[PATCH v2] drm/gem: fix signed integer overflow in idr_alloc end parameter

[PATCH v2] drm/gem: fix signed integer overflow in idr_alloc end parameter

[w15303746062]

From: Mingyu Wang <25181214217@stu.xidian.edu.cn>

During code review of drm_gem_change_handle_ioctl(), a signed integer
overflow vulnerability was identified.

The function currently checks if args->new_handle is strictly greater
than INT_MAX. However, if args->new_handle is exactly INT_MAX, the
subsequent call to idr_alloc() uses 'handle + 1' as the end limit.

Since 'handle' is a signed int, passing INT_MAX results in a signed
integer overflow (INT_MAX + 1 = -2147483648). While idr_alloc() handles
negative end values by gracefully falling back to INT_MAX, this breaks
the exact-ID allocation semantics intended by the caller and relies on
undefined behavior.

Additionally, explicitly reject args->new_handle == 0. In the DRM
subsystem, 0 is universally treated as an invalid handle. Allowing an
object to be aliased to handle 0 breaks core DRM assumptions (e.g.,
drm_gem_object_lookup returns NULL for 0).

Fix this by tightening the limit check to >= INT_MAX to prevent the
overflow, and explicitly reject 0.

Signed-off-by: Mingyu Wang <25181214217@stu.xidian.edu.cn>
---
v2:
 - Add explicit check to reject handle 0, which is invalid in DRM.
 - Remove redundant (u32) cast; rely on standard C integer promotion.
 - Shift focus entirely to fixing the handle + 1 signed integer overflow (Thomas Zimmermann).

 drivers/gpu/drm/drm_gem.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index e12cdf91f4dc..1ad30a700068 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;
 
-- 
2.34.1

Re: [PATCH v2] drm/gem: fix signed integer overflow in idr_alloc end parameter

[sashiko-bot]

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 <25181214217@stu.xidian.edu.cn>

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/20260605142644.1205922-1-w15303746062@163.com?part=1

Re:Re: [PATCH v2] drm/gem: fix signed integer overflow in idr_alloc end parameter

[w15303746062]

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, sashiko-bot@kernel.org 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 <25181214217@stu.xidian.edu.cn>
>
>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/20260605142644.1205922-1-w15303746062@163.com?part=1

Re:[PATCH v2] drm/gem: fix signed integer overflow in idr_alloc end parameter

[w15303746062]

Hi maintainers,

Gentle ping on this v2 patch. 

As a quick note regarding the automated review from Sashiko AI: it correctly flagged two pre-existing issues in this function (a missing idr_preload and a critical UAF race condition during DRM_IOCTL_GEM_CLOSE). 

I want to clarify that those are orthogonal to the integer overflow fixed in this v2 patch. I am keeping this patch purely focused on the signed integer overflow issue so it can be evaluated on its own merits. 

I am currently investigating the UAF race condition separately and will submit a new patch series to address the pre-existing bugs flagged by the bot.

Please let me know if any further revisions are needed for this v2 overflow fix.

Thanks,
Mingyu

At 2026-06-05 22:26:44, w15303746062@163.com wrote:
>From: Mingyu Wang <25181214217@stu.xidian.edu.cn>
>
>During code review of drm_gem_change_handle_ioctl(), a signed integer
>overflow vulnerability was identified.
>
>The function currently checks if args->new_handle is strictly greater
>than INT_MAX. However, if args->new_handle is exactly INT_MAX, the
>subsequent call to idr_alloc() uses 'handle + 1' as the end limit.
>
>Since 'handle' is a signed int, passing INT_MAX results in a signed
>integer overflow (INT_MAX + 1 = -2147483648). While idr_alloc() handles
>negative end values by gracefully falling back to INT_MAX, this breaks
>the exact-ID allocation semantics intended by the caller and relies on
>undefined behavior.
>
>Additionally, explicitly reject args->new_handle == 0. In the DRM
>subsystem, 0 is universally treated as an invalid handle. Allowing an
>object to be aliased to handle 0 breaks core DRM assumptions (e.g.,
>drm_gem_object_lookup returns NULL for 0).
>
>Fix this by tightening the limit check to >= INT_MAX to prevent the
>overflow, and explicitly reject 0.
>
>Signed-off-by: Mingyu Wang <25181214217@stu.xidian.edu.cn>
>---
>v2:
> - Add explicit check to reject handle 0, which is invalid in DRM.
> - Remove redundant (u32) cast; rely on standard C integer promotion.
> - Shift focus entirely to fixing the handle + 1 signed integer overflow (Thomas Zimmermann).
>
> drivers/gpu/drm/drm_gem.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
>index e12cdf91f4dc..1ad30a700068 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;
> 
>-- 
>2.34.1

Re:Re:[PATCH v2] drm/gem: fix signed integer overflow in idr_alloc end parameter

[w15303746062]

>Gentle ping on this v2 patch. 
>
>As a quick note regarding the automated review from Sashiko AI: it correctly flagged two pre-existing issues in this function (a missing idr_preload and a critical UAF race condition during DRM_IOCTL_GEM_CLOSE). 
>
>I want to clarify that those are orthogonal to the integer overflow fixed in this v2 patch. I am keeping this patch purely focused on the signed integer overflow issue so it can be evaluated on its own merits. 
>
>I am currently investigating the UAF race condition separately and will submit a new patch series to address the pre-existing bugs flagged by the bot.
>
>Please let me know if any further revisions are needed for this v2 overflow fix.

Hi all,

Following up on my previous email, I just noticed that
drm_gem_change_handle_ioctl has recently been disabled in mainline due to
security and testing concerns, and a TODO list has been added for its
potential re-enablement.

Given this upstream status, I will drop my v2 patch submission. A standalone
fix for the integer overflow no longer applies cleanly without addressing
the full TODO list (including writing the missing IGT tests and handling
idr_preload).

For the mailing list record, and for whoever takes on the re-enablement
task in the future, I want to leave my local findings here regarding the
edge cases:

1. Signed Integer Overflow in idr_alloc:
The original `args->new_handle > INT_MAX` check is insufficient. If
`new_handle` is exactly `INT_MAX`, the subsequent call to `idr_alloc()`
using `handle + 1` as the end limit will result in a signed integer overflow
(-2147483648). The boundary check must be tightened to `>= INT_MAX`.

2. The "0" Handle Semantics (TODO #3):
The original code did not reject handle 0. In the DRM subsystem, 0 is
universally treated as an invalid handle (e.g., `drm_gem_object_lookup`
returns NULL). Allowing an object to be aliased to handle 0 breaks core
DRM state assumptions. An explicit `args->new_handle == 0` check is
mandatory.

Thank you Thomas and the reviewers for your time and the insightful
discussions. I will move on to other security research targets.

Thanks,
Mingyu