[PATCH v3] drm/gem: Fix and enable CHANGE_HANDLE

[PATCH v3] drm/gem: Fix and enable CHANGE_HANDLE

[David Francis]

Make the following changes to CHANGE_HANDLE
- reject new_handle = old_handle with -EINVAL
- idr_preload before idr_alloc
- reject new_handle = 0 with -EINVAL
and re-enable it.

These changes are prerequisities for the enabling of this ioctl, which was
disabled over security concerns and the lack of IGT tests.

For a summary of the saga so far, see
https://lists.freedesktop.org/archives/dri-devel/2026-June/571890.html

I hope these changes leave the ioctl in a spot where it is safe to
re-enable. There are now igt tests covering all corner case behaviour
and race conditions between change_handle and itself, create, free,
import, and export.

The new_handle = 0 and new_handle = old_handle corner cases
are now -EINVAL as recommended; these are covered in the igt tests.

idr_preload is now correctly called before idr_alloc.

With these changes the TODO can be removed and the ioctl can be
re-enabled.

Accompanying IGT patch on igt-dev is
tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE

v2: new_handle = old_handle is EINVAL
v3: More detailed commit message and some cleanup

cc: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
cc: Christian Koenig <christian.koenig@amd.com>
cc: Simona Vetter <simona.vetter@ffwll.ch>
Signed-off-by: David Francis <David.Francis@amd.com>
---
 drivers/gpu/drm/drm_gem.c   | 30 ++++++++++--------------------
 drivers/gpu/drm/drm_ioctl.c |  3 +--
 2 files changed, 11 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 6a86bd2a0343e..bb55715e8d170 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1018,19 +1018,6 @@ drm_gem_open_ioctl(struct drm_device *dev, void *data,
 	return ret;
 }
 
-/*
- * This ioctl is disabled for security reasons but also it failed
- * to follow process in terms of adding testing in igt and verifying
- * all the corner cases which made fixing security bugs in it even
- * harder than necessary.
- *
- * To re-enable this ioctl
- * 1. land working IGT tests in igt-gpu-tools that cover
- *    all corner cases and race conditions.
- * 2. handle idr_preload
- * 3. handle == 0
- * 4. handle == new_handle semantics definition.
- */
 int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data,
 				struct drm_file *file_priv)
 {
@@ -1041,21 +1028,22 @@ 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)
-		return -EINVAL;
 	new_handle = args->new_handle;
 
-	if (args->handle == new_handle)
-		return 0;
+	/* idr_alloc() limitations. */
+	if (new_handle == 0 || new_handle > INT_MAX ||
+	    args->handle == new_handle)
+		return -EINVAL;
 
 	mutex_lock(&file_priv->prime.lock);
+	idr_preload(GFP_KERNEL);
 	spin_lock(&file_priv->table_lock);
-	ret = idr_alloc(&file_priv->object_idr, NULL, new_handle, new_handle + 1,
-			GFP_NOWAIT);
+	ret = idr_alloc(&file_priv->object_idr, NULL, new_handle,
+			new_handle + 1, GFP_NOWAIT);
 
 	if (ret < 0) {
 		spin_unlock(&file_priv->table_lock);
+		idr_preload_end();
 		goto out_unlock;
 	}
 
@@ -1063,10 +1051,12 @@ int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data,
 	if (IS_ERR_OR_NULL(obj)) {
 		idr_remove(&file_priv->object_idr, new_handle);
 		spin_unlock(&file_priv->table_lock);
+		idr_preload_end();
 		ret = -ENOENT;
 		goto out_unlock;
 	}
 	spin_unlock(&file_priv->table_lock);
+	idr_preload_end();
 
 	if (obj->dma_buf) {
 		ret = drm_prime_add_buf_handle(&file_priv->prime, obj->dma_buf,
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index e2df4becce629..ff193155129e7 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -660,8 +660,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
 	DRM_IOCTL_DEF(DRM_IOCTL_GEM_CLOSE, drm_gem_close_ioctl, DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF(DRM_IOCTL_GEM_FLINK, drm_gem_flink_ioctl, DRM_AUTH),
 	DRM_IOCTL_DEF(DRM_IOCTL_GEM_OPEN, drm_gem_open_ioctl, DRM_AUTH),
-	/* see drm_gem.c:drm_gem_change_handle_ioctl for why this is invalid */
-	DRM_IOCTL_DEF(DRM_IOCTL_GEM_CHANGE_HANDLE, drm_invalid_op, DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF(DRM_IOCTL_GEM_CHANGE_HANDLE, drm_gem_change_handle_ioctl, DRM_RENDER_ALLOW),
 
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETRESOURCES, drm_mode_getresources, 0),
 
-- 
2.34.1

Re: [PATCH v3] drm/gem: Fix and enable CHANGE_HANDLE

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/gem: signed int truncation in drm_gem_change_handle_ioctl()
- [Medium] drm/gem: broken PRIME handling in drm_gem_change_handle_ioctl()
--

commit 69ca138819f06e34ad7cfcd2fb35830a526fdf7a
Author: David Francis <David.Francis@amd.com>

drm/gem: Fix and enable CHANGE_HANDLE

This commit updates drm_gem_change_handle_ioctl() to reject a new_handle of 0
or equal to the existing handle, adds idr_preload() around idr_alloc(), and
re-enables DRM_IOCTL_GEM_CHANGE_HANDLE in the core DRM ioctl table.

> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 3ce38df7b8b85..6973bf69dd439 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
[ ... ]
> @@ -1040,21 +1027,22 @@ 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)
> -		return -EINVAL;
>  	new_handle = args->new_handle;
>  
> -	if (args->handle == new_handle)
> -		return 0;
> +	/* idr_alloc() limitations. */
> +	if (new_handle == 0 || new_handle > INT_MAX ||
> +	    args->handle == new_handle)
> +		return -EINVAL;
>  
>  	mutex_lock(&file_priv->prime.lock);
> +	idr_preload(GFP_KERNEL);
>  	spin_lock(&file_priv->table_lock);
> -	ret = idr_alloc(&file_priv->object_idr, NULL, new_handle, new_handle + 1,
> -			GFP_NOWAIT);
> +	ret = idr_alloc(&file_priv->object_idr, NULL, new_handle,
> +			new_handle + 1, GFP_NOWAIT);

[Severity: High]
Does moving new_handle = args->new_handle ahead of the INT_MAX check allow
negative values to bypass validation and trigger a warning in idr_alloc()?

In struct drm_gem_change_handle, args->new_handle is __u32, whereas the local
variable new_handle in drm_gem_change_handle_ioctl() is a signed int. Moving
the assignment before the bounds check makes new_handle > INT_MAX
tautologically false.

When userspace passes args->new_handle >= 0x80000000 (with args->handle !=
args->new_handle), new_handle wraps to a negative value, passes the check,
and is passed as start to idr_alloc():

lib/idr.c:idr_alloc() {
	if (WARN_ON_ONCE(start < 0))
		return -EINVAL;
}

This reintroduces the warning fixed by commit 12f15d52d38a, and evaluating
new_handle + 1 when args->new_handle == INT_MAX also overflows signed int.
Should new_handle be declared as u32 (or checked against args->new_handle
before assignment)?

>  
>  	if (ret < 0) {
>  		spin_unlock(&file_priv->table_lock);
> +		idr_preload_end();
>  		goto out_unlock;
>  	}
>  
> @@ -1062,10 +1050,12 @@ int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data,
>  	if (IS_ERR_OR_NULL(obj)) {
>  		idr_remove(&file_priv->object_idr, new_handle);
>  		spin_unlock(&file_priv->table_lock);
> +		idr_preload_end();
>  		ret = -ENOENT;
>  		goto out_unlock;
>  	}
>  	spin_unlock(&file_priv->table_lock);
> +	idr_preload_end();
>  
>  	if (obj->dma_buf) {
>  		ret = drm_prime_add_buf_handle(&file_priv->prime, obj->dma_buf,

[Severity: Medium]
With DRM_IOCTL_GEM_CHANGE_HANDLE now enabled in drm_ioctls[], is it safe for
drm_gem_change_handle_ioctl() to read obj->dma_buf and update file_priv->prime
here?

First, struct drm_gem_object documents that obj->dma_buf is protected by
dev->object_name_lock:

drivers/gpu/drm/drm_prime.c:export_and_register_object() {
	...
	obj->dma_buf = dmabuf;
	get_dma_buf(obj->dma_buf);
}

Reading obj->dma_buf under only file_priv->prime.lock can race with another
drm_file exporting the shared object under dev->object_name_lock.

Second, checking if (obj->dma_buf) tests whether the underlying GEM object has
an associated dma_buf rather than whether args->handle is registered in this
file's PRIME cache (via drm_prime_lookup_buf_by_handle(&file_priv->prime,
args->handle)). If a client holds a PRIME handle to obj and a second non-PRIME
handle to the same obj (for example via GEM_OPEN or MODE_GETFB2), changing the
non-PRIME handle will insert a duplicate entry for obj->dma_buf into
file_priv->prime.dmabufs while drm_prime_remove_buf_handle(&file_priv->prime,
args->handle) will be a no-op.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103155411.3305376-1-David.Francis@amd.com?part=1

Re: [PATCH v3] drm/gem: Fix and enable CHANGE_HANDLE

[Francis, David]

> Does moving new_handle = args->new_handle ahead of the INT_MAX check allow
> negative values to bypass validation and trigger a warning in idr_alloc()?
Whoops, that's why it was two stages before. Changing the local to u32 should fix this

>Reading obj->dma_buf under only file_priv->prime.lock can race with another
>drm_file exporting the shared object under dev->object_name_lock.
Fairly certain this can't happen as the object is not in the idr at this stage, so
it cannot be exported.

>Second, checking if (obj->dma_buf) tests whether the underlying GEM object has
>an associated dma_buf rather than whether args->handle is registered in this
>file's PRIME cache (via drm_prime_lookup_buf_by_handle(&file_priv->prime,
>args->handle)). If a client holds a PRIME handle to obj and a second non-PRIME
>handle to the same obj (for example via GEM_OPEN or MODE_GETFB2), changing the
>non-PRIME handle will insert a duplicate entry for obj->dma_buf into
>file_priv->prime.dmabufs while drm_prime_remove_buf_handle(&file_priv->prime,
>args->handle) will be a no-op.

This is new to me. Can a single obj be in two different files?
I guess no particular downside to having this do a
drm_prime_lookup_buf_handle() on the old handle
and confirm it isn't ENOENT

David Francis

________________________________________
From: sashiko-bot@kernel.org <sashiko-bot@kernel.org>
Sent: Tuesday, November 3, 2026 11:13 AM
To: Francis, David; dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v3] drm/gem: Fix and enable CHANGE_HANDLE

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/gem: signed int truncation in drm_gem_change_handle_ioctl()
- [Medium] drm/gem: broken PRIME handling in drm_gem_change_handle_ioctl()
--

commit 69ca138819f06e34ad7cfcd2fb35830a526fdf7a
Author: David Francis <David.Francis@amd.com>

drm/gem: Fix and enable CHANGE_HANDLE

This commit updates drm_gem_change_handle_ioctl() to reject a new_handle of 0
or equal to the existing handle, adds idr_preload() around idr_alloc(), and
re-enables DRM_IOCTL_GEM_CHANGE_HANDLE in the core DRM ioctl table.

> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 3ce38df7b8b85..6973bf69dd439 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
[ ... ]
> @@ -1040,21 +1027,22 @@ 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)
> -             return -EINVAL;
>       new_handle = args->new_handle;
>
> -     if (args->handle == new_handle)
> -             return 0;
> +     /* idr_alloc() limitations. */
> +     if (new_handle == 0 || new_handle > INT_MAX ||
> +         args->handle == new_handle)
> +             return -EINVAL;
>
>       mutex_lock(&file_priv->prime.lock);
> +     idr_preload(GFP_KERNEL);
>       spin_lock(&file_priv->table_lock);
> -     ret = idr_alloc(&file_priv->object_idr, NULL, new_handle, new_handle + 1,
> -                     GFP_NOWAIT);
> +     ret = idr_alloc(&file_priv->object_idr, NULL, new_handle,
> +                     new_handle + 1, GFP_NOWAIT);

[Severity: High]
Does moving new_handle = args->new_handle ahead of the INT_MAX check allow
negative values to bypass validation and trigger a warning in idr_alloc()?

In struct drm_gem_change_handle, args->new_handle is __u32, whereas the local
variable new_handle in drm_gem_change_handle_ioctl() is a signed int. Moving
the assignment before the bounds check makes new_handle > INT_MAX
tautologically false.

When userspace passes args->new_handle >= 0x80000000 (with args->handle !=
args->new_handle), new_handle wraps to a negative value, passes the check,
and is passed as start to idr_alloc():

lib/idr.c:idr_alloc() {
        if (WARN_ON_ONCE(start < 0))
                return -EINVAL;
}

This reintroduces the warning fixed by commit 12f15d52d38a, and evaluating
new_handle + 1 when args->new_handle == INT_MAX also overflows signed int.
Should new_handle be declared as u32 (or checked against args->new_handle
before assignment)?

>
>       if (ret < 0) {
>               spin_unlock(&file_priv->table_lock);
> +             idr_preload_end();
>               goto out_unlock;
>       }
>
> @@ -1062,10 +1050,12 @@ int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data,
>       if (IS_ERR_OR_NULL(obj)) {
>               idr_remove(&file_priv->object_idr, new_handle);
>               spin_unlock(&file_priv->table_lock);
> +             idr_preload_end();
>               ret = -ENOENT;
>               goto out_unlock;
>       }
>       spin_unlock(&file_priv->table_lock);
> +     idr_preload_end();
>
>       if (obj->dma_buf) {
>               ret = drm_prime_add_buf_handle(&file_priv->prime, obj->dma_buf,

[Severity: Medium]
With DRM_IOCTL_GEM_CHANGE_HANDLE now enabled in drm_ioctls[], is it safe for
drm_gem_change_handle_ioctl() to read obj->dma_buf and update file_priv->prime
here?

First, struct drm_gem_object documents that obj->dma_buf is protected by
dev->object_name_lock:

drivers/gpu/drm/drm_prime.c:export_and_register_object() {
        ...
        obj->dma_buf = dmabuf;
        get_dma_buf(obj->dma_buf);
}

Reading obj->dma_buf under only file_priv->prime.lock can race with another
drm_file exporting the shared object under dev->object_name_lock.

Second, checking if (obj->dma_buf) tests whether the underlying GEM object has
an associated dma_buf rather than whether args->handle is registered in this
file's PRIME cache (via drm_prime_lookup_buf_by_handle(&file_priv->prime,
args->handle)). If a client holds a PRIME handle to obj and a second non-PRIME
handle to the same obj (for example via GEM_OPEN or MODE_GETFB2), changing the
non-PRIME handle will insert a duplicate entry for obj->dma_buf into
file_priv->prime.dmabufs while drm_prime_remove_buf_handle(&file_priv->prime,
args->handle) will be a no-op.

--
Sashiko AI review · https://sashiko.dev/#/patchset/20261103155411.3305376-1-David.Francis@amd.com?part=1

Re: [PATCH v3] drm/gem: Fix and enable CHANGE_HANDLE

[Tvrtko Ursulin]

On 03/11/2026 15:54, David Francis wrote:
> Make the following changes to CHANGE_HANDLE
> - reject new_handle = old_handle with -EINVAL
> - idr_preload before idr_alloc
> - reject new_handle = 0 with -EINVAL
> and re-enable it.
> 
> These changes are prerequisities for the enabling of this ioctl, which was
> disabled over security concerns and the lack of IGT tests.
> 
> For a summary of the saga so far, see
> https://lists.freedesktop.org/archives/dri-devel/2026-June/571890.html
> 
> I hope these changes leave the ioctl in a spot where it is safe to
> re-enable. There are now igt tests covering all corner case behaviour
> and race conditions between change_handle and itself, create, free,
> import, and export.
> 
> The new_handle = 0 and new_handle = old_handle corner cases
> are now -EINVAL as recommended; these are covered in the igt tests.
> 
> idr_preload is now correctly called before idr_alloc.
> 
> With these changes the TODO can be removed and the ioctl can be
> re-enabled.
> 
> Accompanying IGT patch on igt-dev is
> tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE
> 
> v2: new_handle = old_handle is EINVAL
> v3: More detailed commit message and some cleanup
> 
> cc: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> cc: Christian Koenig <christian.koenig@amd.com>
> cc: Simona Vetter <simona.vetter@ffwll.ch>
> Signed-off-by: David Francis <David.Francis@amd.com>
> ---
>   drivers/gpu/drm/drm_gem.c   | 30 ++++++++++--------------------
>   drivers/gpu/drm/drm_ioctl.c |  3 +--
>   2 files changed, 11 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 6a86bd2a0343e..bb55715e8d170 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1018,19 +1018,6 @@ drm_gem_open_ioctl(struct drm_device *dev, void *data,
>   	return ret;
>   }
>   
> -/*
> - * This ioctl is disabled for security reasons but also it failed
> - * to follow process in terms of adding testing in igt and verifying
> - * all the corner cases which made fixing security bugs in it even
> - * harder than necessary.
> - *
> - * To re-enable this ioctl
> - * 1. land working IGT tests in igt-gpu-tools that cover
> - *    all corner cases and race conditions.
> - * 2. handle idr_preload
> - * 3. handle == 0
> - * 4. handle == new_handle semantics definition.
> - */
>   int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data,
>   				struct drm_file *file_priv)
>   {
> @@ -1041,21 +1028,22 @@ 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)
> -		return -EINVAL;
>   	new_handle = args->new_handle;
>   
> -	if (args->handle == new_handle)
> -		return 0;
> +	/* idr_alloc() limitations. */

Nit- it's now more than idr_alloc limitations.

> +	if (new_handle == 0 || new_handle > INT_MAX ||
> +	    args->handle == new_handle)
> +		return -EINVAL;

I'd like to open the same discussion point that I opened in the 
corresponding IGT. Should we or should we not check for INT_MAX here. 
There I wrote this:

"""
About this one I am not sure. Is it not implementation details as long 
as the uapi for handle is u32? Let say someone converts to XArray one 
day and then we have to artificially limit that to s32 just so 
gem_change_handle can keep working.
"""

>   
>   	mutex_lock(&file_priv->prime.lock);
> +	idr_preload(GFP_KERNEL);
>   	spin_lock(&file_priv->table_lock);
> -	ret = idr_alloc(&file_priv->object_idr, NULL, new_handle, new_handle + 1,
> -			GFP_NOWAIT);
> +	ret = idr_alloc(&file_priv->object_idr, NULL, new_handle,
> +			new_handle + 1, GFP_NOWAIT);

And here I opened a point about what happens when target handle is taken 
(again in IGT review):

"""
Hmm.. this is actually leaking the wrong implementation detail of 
idr_alloc to ABI. ENOSPC is "no free handles". It feels it would be 
better if gem_change_handle returned EBUSY or EEXIST for this case so it 
is not conflated. Thoughts?
""'

>   
>   	if (ret < 0) {
>   		spin_unlock(&file_priv->table_lock);
> +		idr_preload_end();
>   		goto out_unlock;
>   	}
>   
> @@ -1063,10 +1051,12 @@ int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data,
>   	if (IS_ERR_OR_NULL(obj)) {
>   		idr_remove(&file_priv->object_idr, new_handle);
>   		spin_unlock(&file_priv->table_lock);
> +		idr_preload_end();
>   		ret = -ENOENT;
>   		goto out_unlock;
>   	}
>   	spin_unlock(&file_priv->table_lock);
> +	idr_preload_end();
>   
>   	if (obj->dma_buf) {
>   		ret = drm_prime_add_buf_handle(&file_priv->prime, obj->dma_buf,
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index e2df4becce629..ff193155129e7 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -660,8 +660,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
>   	DRM_IOCTL_DEF(DRM_IOCTL_GEM_CLOSE, drm_gem_close_ioctl, DRM_RENDER_ALLOW),
>   	DRM_IOCTL_DEF(DRM_IOCTL_GEM_FLINK, drm_gem_flink_ioctl, DRM_AUTH),
>   	DRM_IOCTL_DEF(DRM_IOCTL_GEM_OPEN, drm_gem_open_ioctl, DRM_AUTH),
> -	/* see drm_gem.c:drm_gem_change_handle_ioctl for why this is invalid */
> -	DRM_IOCTL_DEF(DRM_IOCTL_GEM_CHANGE_HANDLE, drm_invalid_op, DRM_RENDER_ALLOW),
> +	DRM_IOCTL_DEF(DRM_IOCTL_GEM_CHANGE_HANDLE, drm_gem_change_handle_ioctl, DRM_RENDER_ALLOW),
>   
>   	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETRESOURCES, drm_mode_getresources, 0),
>   

The rest looks good to me.

Regards,

Tvrtko

Re: [PATCH v3] drm/gem: Fix and enable CHANGE_HANDLE

[Francis, David]

>> +     if (new_handle == 0 || new_handle > INT_MAX ||
>> +         args->handle == new_handle)
>> +             return -EINVAL;
>
>I'd like to open the same discussion point that I opened in the
>corresponding IGT. Should we or should we not check for INT_MAX here.
>There I wrote this:
>
>"""
>About this one I am not sure. Is it not implementation details as long
>as the uapi for handle is u32? Let say someone converts to XArray one
>day and then we have to artificially limit that to s32 just so
>gem_change_handle can keep working.
>"""

uapi is u32 but idr only does handles from 0 to INT_MAX. I think it's
correct for this to be an error now; if the interface one day changes
such that INT_MAX < handle < UINT_MAX is now valid, I think we're okay to have
some cases that were once errors now work (that's what happens whenever an
ioctl adds new flags / replaces padding with new inputs).

>> +     ret = idr_alloc(&file_priv->object_idr, NULL, new_handle,
>> +                     new_handle + 1, GFP_NOWAIT);
>
>And here I opened a point about what happens when target handle is taken
>(again in IGT review):
>
>"""
>Hmm.. this is actually leaking the wrong implementation detail of
>idr_alloc to ABI. ENOSPC is "no free handles". It feels it would be
>better if gem_change_handle returned EBUSY or EEXIST for this case so it
>is not conflated. Thoughts?
>""'
On balance EBUSY is probably a more accurate error code for this than
ENOSPC, I can make that change. Neither is perfect? "Busy" to me implies
a temporary use of a resource, but this is about a handle which is persistently
being used. EEXIST has the error message "File exists"; I don't know whether it's
used for cases like this is practice.

The definition of these error codes seems pretty loose, as long as it's distinct from
the other error codes this can return

David Francis

________________________________________
From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Sent: Thursday, November 5, 2026 6:12 AM
To: Francis, David; dri-devel@lists.freedesktop.org
Cc: Koenig, Christian; Simona Vetter
Subject: Re: [PATCH v3] drm/gem: Fix and enable CHANGE_HANDLE


On 03/11/2026 15:54, David Francis wrote:
> Make the following changes to CHANGE_HANDLE
> - reject new_handle = old_handle with -EINVAL
> - idr_preload before idr_alloc
> - reject new_handle = 0 with -EINVAL
> and re-enable it.
>
> These changes are prerequisities for the enabling of this ioctl, which was
> disabled over security concerns and the lack of IGT tests.
>
> For a summary of the saga so far, see
> https://lists.freedesktop.org/archives/dri-devel/2026-June/571890.html
>
> I hope these changes leave the ioctl in a spot where it is safe to
> re-enable. There are now igt tests covering all corner case behaviour
> and race conditions between change_handle and itself, create, free,
> import, and export.
>
> The new_handle = 0 and new_handle = old_handle corner cases
> are now -EINVAL as recommended; these are covered in the igt tests.
>
> idr_preload is now correctly called before idr_alloc.
>
> With these changes the TODO can be removed and the ioctl can be
> re-enabled.
>
> Accompanying IGT patch on igt-dev is
> tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE
>
> v2: new_handle = old_handle is EINVAL
> v3: More detailed commit message and some cleanup
>
> cc: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> cc: Christian Koenig <christian.koenig@amd.com>
> cc: Simona Vetter <simona.vetter@ffwll.ch>
> Signed-off-by: David Francis <David.Francis@amd.com>
> ---
>   drivers/gpu/drm/drm_gem.c   | 30 ++++++++++--------------------
>   drivers/gpu/drm/drm_ioctl.c |  3 +--
>   2 files changed, 11 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 6a86bd2a0343e..bb55715e8d170 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1018,19 +1018,6 @@ drm_gem_open_ioctl(struct drm_device *dev, void *data,
>       return ret;
>   }
>
> -/*
> - * This ioctl is disabled for security reasons but also it failed
> - * to follow process in terms of adding testing in igt and verifying
> - * all the corner cases which made fixing security bugs in it even
> - * harder than necessary.
> - *
> - * To re-enable this ioctl
> - * 1. land working IGT tests in igt-gpu-tools that cover
> - *    all corner cases and race conditions.
> - * 2. handle idr_preload
> - * 3. handle == 0
> - * 4. handle == new_handle semantics definition.
> - */
>   int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data,
>                               struct drm_file *file_priv)
>   {
> @@ -1041,21 +1028,22 @@ 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)
> -             return -EINVAL;
>       new_handle = args->new_handle;
>
> -     if (args->handle == new_handle)
> -             return 0;
> +     /* idr_alloc() limitations. */

Nit- it's now more than idr_alloc limitations.

> +     if (new_handle == 0 || new_handle > INT_MAX ||
> +         args->handle == new_handle)
> +             return -EINVAL;

I'd like to open the same discussion point that I opened in the
corresponding IGT. Should we or should we not check for INT_MAX here.
There I wrote this:

"""
About this one I am not sure. Is it not implementation details as long
as the uapi for handle is u32? Let say someone converts to XArray one
day and then we have to artificially limit that to s32 just so
gem_change_handle can keep working.
"""

>
>       mutex_lock(&file_priv->prime.lock);
> +     idr_preload(GFP_KERNEL);
>       spin_lock(&file_priv->table_lock);
> -     ret = idr_alloc(&file_priv->object_idr, NULL, new_handle, new_handle + 1,
> -                     GFP_NOWAIT);
> +     ret = idr_alloc(&file_priv->object_idr, NULL, new_handle,
> +                     new_handle + 1, GFP_NOWAIT);

And here I opened a point about what happens when target handle is taken
(again in IGT review):

"""
Hmm.. this is actually leaking the wrong implementation detail of
idr_alloc to ABI. ENOSPC is "no free handles". It feels it would be
better if gem_change_handle returned EBUSY or EEXIST for this case so it
is not conflated. Thoughts?
""'

>
>       if (ret < 0) {
>               spin_unlock(&file_priv->table_lock);
> +             idr_preload_end();
>               goto out_unlock;
>       }
>
> @@ -1063,10 +1051,12 @@ int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data,
>       if (IS_ERR_OR_NULL(obj)) {
>               idr_remove(&file_priv->object_idr, new_handle);
>               spin_unlock(&file_priv->table_lock);
> +             idr_preload_end();
>               ret = -ENOENT;
>               goto out_unlock;
>       }
>       spin_unlock(&file_priv->table_lock);
> +     idr_preload_end();
>
>       if (obj->dma_buf) {
>               ret = drm_prime_add_buf_handle(&file_priv->prime, obj->dma_buf,
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index e2df4becce629..ff193155129e7 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -660,8 +660,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
>       DRM_IOCTL_DEF(DRM_IOCTL_GEM_CLOSE, drm_gem_close_ioctl, DRM_RENDER_ALLOW),
>       DRM_IOCTL_DEF(DRM_IOCTL_GEM_FLINK, drm_gem_flink_ioctl, DRM_AUTH),
>       DRM_IOCTL_DEF(DRM_IOCTL_GEM_OPEN, drm_gem_open_ioctl, DRM_AUTH),
> -     /* see drm_gem.c:drm_gem_change_handle_ioctl for why this is invalid */
> -     DRM_IOCTL_DEF(DRM_IOCTL_GEM_CHANGE_HANDLE, drm_invalid_op, DRM_RENDER_ALLOW),
> +     DRM_IOCTL_DEF(DRM_IOCTL_GEM_CHANGE_HANDLE, drm_gem_change_handle_ioctl, DRM_RENDER_ALLOW),
>
>       DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETRESOURCES, drm_mode_getresources, 0),
>

The rest looks good to me.

Regards,

Tvrtko

Re: [PATCH v3] drm/gem: Fix and enable CHANGE_HANDLE

[Tvrtko Ursulin]

On 05/11/2026 19:49, Francis, David wrote:
>>> +     if (new_handle == 0 || new_handle > INT_MAX ||
>>> +         args->handle == new_handle)
>>> +             return -EINVAL;
>>
>> I'd like to open the same discussion point that I opened in the
>> corresponding IGT. Should we or should we not check for INT_MAX here.
>> There I wrote this:
>>
>> """
>> About this one I am not sure. Is it not implementation details as long
>> as the uapi for handle is u32? Let say someone converts to XArray one
>> day and then we have to artificially limit that to s32 just so
>> gem_change_handle can keep working.
>> """
> 
> uapi is u32 but idr only does handles from 0 to INT_MAX. I think it's
> correct for this to be an error now; if the interface one day changes
> such that INT_MAX < handle < UINT_MAX is now valid, I think we're okay to have
> some cases that were once errors now work (that's what happens whenever an
> ioctl adds new flags / replaces padding with new inputs).

Okay I get it. Output handle from BO creation is u32, so rename is u32, 
but internals cannot handle it and we want to avoid silent wrap. You 
have to somehow handle it.. hm.. I guess we have to live with it. If we 
replace IDR with XArray one day we can always artificially keep the same 
s32 limit.

>>> +     ret = idr_alloc(&file_priv->object_idr, NULL, new_handle,
>>> +                     new_handle + 1, GFP_NOWAIT);
>>
>> And here I opened a point about what happens when target handle is taken
>> (again in IGT review):
>>
>> """
>> Hmm.. this is actually leaking the wrong implementation detail of
>> idr_alloc to ABI. ENOSPC is "no free handles". It feels it would be
>> better if gem_change_handle returned EBUSY or EEXIST for this case so it
>> is not conflated. Thoughts?
>> ""'
> On balance EBUSY is probably a more accurate error code for this than
> ENOSPC, I can make that change. Neither is perfect? "Busy" to me implies
> a temporary use of a resource, but this is about a handle which is persistently
> being used. EEXIST has the error message "File exists"; I don't know whether it's
> used for cases like this is practice.
> 
> The definition of these error codes seems pretty loose, as long as it's distinct from
> the other error codes this can return

I suppose all three could be argued for.

ENOSPC - No space in the target range, just that the target range is a 
single id in the case of rename.

EBUSY - Target handle is busy.

EEXIST - Target handle already exists.

There is precedent in DRM for EBUSY and EXIST judging by a quick grep. 
EBUSY sounds the weakest though.

Don't know. If no one else provides their 2c in due time keep it as it is.

Regards,

Tvrtko

____________________________________
> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Sent: Thursday, November 5, 2026 6:12 AM
> To: Francis, David; dri-devel@lists.freedesktop.org
> Cc: Koenig, Christian; Simona Vetter
> Subject: Re: [PATCH v3] drm/gem: Fix and enable CHANGE_HANDLE
> 
> 
> On 03/11/2026 15:54, David Francis wrote:
>> Make the following changes to CHANGE_HANDLE
>> - reject new_handle = old_handle with -EINVAL
>> - idr_preload before idr_alloc
>> - reject new_handle = 0 with -EINVAL
>> and re-enable it.
>>
>> These changes are prerequisities for the enabling of this ioctl, which was
>> disabled over security concerns and the lack of IGT tests.
>>
>> For a summary of the saga so far, see
>> https://lists.freedesktop.org/archives/dri-devel/2026-June/571890.html
>>
>> I hope these changes leave the ioctl in a spot where it is safe to
>> re-enable. There are now igt tests covering all corner case behaviour
>> and race conditions between change_handle and itself, create, free,
>> import, and export.
>>
>> The new_handle = 0 and new_handle = old_handle corner cases
>> are now -EINVAL as recommended; these are covered in the igt tests.
>>
>> idr_preload is now correctly called before idr_alloc.
>>
>> With these changes the TODO can be removed and the ioctl can be
>> re-enabled.
>>
>> Accompanying IGT patch on igt-dev is
>> tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE
>>
>> v2: new_handle = old_handle is EINVAL
>> v3: More detailed commit message and some cleanup
>>
>> cc: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> cc: Christian Koenig <christian.koenig@amd.com>
>> cc: Simona Vetter <simona.vetter@ffwll.ch>
>> Signed-off-by: David Francis <David.Francis@amd.com>
>> ---
>>    drivers/gpu/drm/drm_gem.c   | 30 ++++++++++--------------------
>>    drivers/gpu/drm/drm_ioctl.c |  3 +--
>>    2 files changed, 11 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
>> index 6a86bd2a0343e..bb55715e8d170 100644
>> --- a/drivers/gpu/drm/drm_gem.c
>> +++ b/drivers/gpu/drm/drm_gem.c
>> @@ -1018,19 +1018,6 @@ drm_gem_open_ioctl(struct drm_device *dev, void *data,
>>        return ret;
>>    }
>>
>> -/*
>> - * This ioctl is disabled for security reasons but also it failed
>> - * to follow process in terms of adding testing in igt and verifying
>> - * all the corner cases which made fixing security bugs in it even
>> - * harder than necessary.
>> - *
>> - * To re-enable this ioctl
>> - * 1. land working IGT tests in igt-gpu-tools that cover
>> - *    all corner cases and race conditions.
>> - * 2. handle idr_preload
>> - * 3. handle == 0
>> - * 4. handle == new_handle semantics definition.
>> - */
>>    int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data,
>>                                struct drm_file *file_priv)
>>    {
>> @@ -1041,21 +1028,22 @@ 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)
>> -             return -EINVAL;
>>        new_handle = args->new_handle;
>>
>> -     if (args->handle == new_handle)
>> -             return 0;
>> +     /* idr_alloc() limitations. */
> 
> Nit- it's now more than idr_alloc limitations.
> 
>> +     if (new_handle == 0 || new_handle > INT_MAX ||
>> +         args->handle == new_handle)
>> +             return -EINVAL;
> 
> I'd like to open the same discussion point that I opened in the
> corresponding IGT. Should we or should we not check for INT_MAX here.
> There I wrote this:
> 
> """
> About this one I am not sure. Is it not implementation details as long
> as the uapi for handle is u32? Let say someone converts to XArray one
> day and then we have to artificially limit that to s32 just so
> gem_change_handle can keep working.
> """
> 
>>
>>        mutex_lock(&file_priv->prime.lock);
>> +     idr_preload(GFP_KERNEL);
>>        spin_lock(&file_priv->table_lock);
>> -     ret = idr_alloc(&file_priv->object_idr, NULL, new_handle, new_handle + 1,
>> -                     GFP_NOWAIT);
>> +     ret = idr_alloc(&file_priv->object_idr, NULL, new_handle,
>> +                     new_handle + 1, GFP_NOWAIT);
> 
> And here I opened a point about what happens when target handle is taken
> (again in IGT review):
> 
> """
> Hmm.. this is actually leaking the wrong implementation detail of
> idr_alloc to ABI. ENOSPC is "no free handles". It feels it would be
> better if gem_change_handle returned EBUSY or EEXIST for this case so it
> is not conflated. Thoughts?
> ""'
> 
>>
>>        if (ret < 0) {
>>                spin_unlock(&file_priv->table_lock);
>> +             idr_preload_end();
>>                goto out_unlock;
>>        }
>>
>> @@ -1063,10 +1051,12 @@ int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data,
>>        if (IS_ERR_OR_NULL(obj)) {
>>                idr_remove(&file_priv->object_idr, new_handle);
>>                spin_unlock(&file_priv->table_lock);
>> +             idr_preload_end();
>>                ret = -ENOENT;
>>                goto out_unlock;
>>        }
>>        spin_unlock(&file_priv->table_lock);
>> +     idr_preload_end();
>>
>>        if (obj->dma_buf) {
>>                ret = drm_prime_add_buf_handle(&file_priv->prime, obj->dma_buf,
>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
>> index e2df4becce629..ff193155129e7 100644
>> --- a/drivers/gpu/drm/drm_ioctl.c
>> +++ b/drivers/gpu/drm/drm_ioctl.c
>> @@ -660,8 +660,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
>>        DRM_IOCTL_DEF(DRM_IOCTL_GEM_CLOSE, drm_gem_close_ioctl, DRM_RENDER_ALLOW),
>>        DRM_IOCTL_DEF(DRM_IOCTL_GEM_FLINK, drm_gem_flink_ioctl, DRM_AUTH),
>>        DRM_IOCTL_DEF(DRM_IOCTL_GEM_OPEN, drm_gem_open_ioctl, DRM_AUTH),
>> -     /* see drm_gem.c:drm_gem_change_handle_ioctl for why this is invalid */
>> -     DRM_IOCTL_DEF(DRM_IOCTL_GEM_CHANGE_HANDLE, drm_invalid_op, DRM_RENDER_ALLOW),
>> +     DRM_IOCTL_DEF(DRM_IOCTL_GEM_CHANGE_HANDLE, drm_gem_change_handle_ioctl, DRM_RENDER_ALLOW),
>>
>>        DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETRESOURCES, drm_mode_getresources, 0),
>>
> 
> The rest looks good to me.
> 
> Regards,
> 
> Tvrtko
>