Hi all,
Thanks to Sashiko AI for the review. To clarify, the two issues reported (the missing idr_preload and the critical UAF race condition) are pre-existing bugs in drm_gem_change_handle_ioctl() and are orthogonal to the integer overflow fix addressed in my current v2 patch. The UAF race condition is particularly alarming. I will keep my current v2 patch focused purely on fixing the integer overflow as it currently stands. I will investigate these two pre-existing issues separately and submit a separate patch series to address them. Thanks, Mingyu At 2026-06-05 22:38:59, [email protected] wrote: >Thank you for your contribution! Sashiko AI review found 2 potential issue(s) >to consider: > >Pre-existing issues: >- [Critical] A race condition exists between `drm_gem_change_handle_ioctl` and >`DRM_IOCTL_GEM_CLOSE` due to a lockless window, allowing a Use-After-Free, >unbalanced handle counts, and IDR corruption. >- [Medium] Calling `idr_alloc` with `GFP_NOWAIT` inside a spinlock without a >preceding `idr_preload` can cause spurious memory allocation failures. >-- > >commit b765b42ca862b5e1577465e07a11a3f29e24c848 >Author: Mingyu Wang <[email protected]> > >drm/gem: fix signed integer overflow in idr_alloc end parameter > >This commit fixes a signed integer overflow vulnerability in >drm_gem_change_handle_ioctl(). It tightens the limit check to >= INT_MAX to >prevent overflow and explicitly rejects 0 as a handle, as 0 is universally >treated as an invalid handle in the DRM subsystem. > >> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c >> index ac751a42db281..b42241d6802f9 100644 >> --- a/drivers/gpu/drm/drm_gem.c >> +++ b/drivers/gpu/drm/drm_gem.c >> @@ -1025,8 +1025,12 @@ int drm_gem_change_handle_ioctl(struct drm_device >> *dev, void *data, >> if (!drm_core_check_feature(dev, DRIVER_GEM)) >> return -EOPNOTSUPP; >> >> - /* idr_alloc() limitation. */ >> - if (args->new_handle > INT_MAX) >> + /* >> + * idr_alloc() limitation. >> + * Reject handle 0 (invalid in DRM) and strictly bound >> + * to < INT_MAX to avoid signed integer overflow in handle + 1. >> + */ >> + if (args->new_handle == 0 || args->new_handle >= INT_MAX) >> return -EINVAL; >> handle = args->new_handle; >> > >[Severity: Medium] >This is a pre-existing issue, but can calling idr_alloc() with GFP_NOWAIT >inside a spinlock without a preceding idr_preload() cause spurious memory >allocation failures? > >drivers/gpu/drm/drm_gem.c:drm_gem_change_handle_ioctl() { > ... > spin_lock(&file_priv->table_lock); > ... > ret = idr_alloc(&file_priv->object_idr, obj, handle, handle + 1, > GFP_NOWAIT); > ... >} > >Because handle is user-controlled and can be up to INT_MAX - 1, the IDR radix >tree may need to allocate multiple deep tree nodes. Without preloading, is it >possible for this GFP_NOWAIT allocation to fail under memory pressure, causing >the ioctl to spuriously return -ENOMEM? > >[Severity: Critical] >This is a pre-existing issue, but does a race condition exist between >drm_gem_change_handle_ioctl() and DRM_IOCTL_GEM_CLOSE due to a lockless >window? > >drivers/gpu/drm/drm_gem.c:drm_gem_change_handle_ioctl() { > ... > idr_replace(&file_priv->object_idr, NULL, args->handle); > spin_unlock(&file_priv->table_lock); > ... >} > >drm_gem_change_handle_ioctl() looks up args->handle locklessly, gaining a >temporary kref. Between this lookup and acquiring table_lock, could another >thread call DRM_IOCTL_GEM_CLOSE on args->handle, removing it from the IDR and >releasing its handle reference? > >When drm_gem_change_handle_ioctl() resumes, it ignores the return value of >idr_replace(..., NULL, args->handle) and unconditionally associates >args->new_handle with obj. > >Since the handle reference was dropped by the concurrent close, and this >function doesn't acquire a new one for the new handle, could obj be left in >the IDR without a corresponding handle reference? > >When the temporary lookup kref is dropped via drm_gem_object_put(obj) at the >end of the function, the object is freed, but args->new_handle still points to >it in the IDR. Could subsequent access to this handle trigger an exploitable >use-after-free? > >-- >Sashiko AI review ยท >https://sashiko.dev/#/patchset/[email protected]?part=1
