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

[PATCH v4] 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.

And the new handle being taken is now EBUSY instead of ENOENT.

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

Accompanying IGT patch on igt-dev (currently at v4) 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
v4: New handle taken is now EBUSY instead of ENOSPC

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   | 33 ++++++++++++---------------------
 drivers/gpu/drm/drm_ioctl.c |  3 +--
 2 files changed, 13 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 6a86bd2a0343..0c64170f7d61 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1018,44 +1018,33 @@ 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)
 {
 	struct drm_gem_change_handle *args = data;
 	struct drm_gem_object *obj;
-	int new_handle, ret;
+	int ret;
+	u32 new_handle;
 
 	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;
+	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();
+		ret = -EBUSY;
 		goto out_unlock;
 	}
 
@@ -1063,10 +1052,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 e2df4becce62..ff193155129e 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 v4] drm/gem: Fix and enable CHANGE_HANDLE

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] drm/gem: broken PRIME handling in
  drm_gem_change_handle_ioctl()
- [Medium] drm/gem: stale i915 VMA LUT cache in
  drm_gem_change_handle_ioctl()
- [Low] drm/gem: idr_alloc() -ENOMEM masked in
  drm_gem_change_handle_ioctl()
--

commit 7986c0e82ecacec73e2cd689a660b9387bc87689
Author: David Francis <David.Francis@amd.com>

drm/gem: Fix and enable CHANGE_HANDLE

This commit updates drm_gem_change_handle_ioctl() to reject invalid or
unchanged handles, preload the IDR before allocation, return -EBUSY when
new_handle is already in use, and re-enables DRM_IOCTL_GEM_CHANGE_HANDLE.

> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 3ce38df7b8b85..66f69cf41aaf0 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1017,44 +1017,33 @@ 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)
>  {
>  	struct drm_gem_change_handle *args = data;
>  	struct drm_gem_object *obj;
> -	int new_handle, ret;
> +	int ret;
> +	u32 new_handle;
>  
>  	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;
> +	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();
> +		ret = -EBUSY;
>  		goto out_unlock;
>  	}

[Severity: Low]
Does this mask -ENOMEM from idr_alloc()?  If idr_preload() fails to
allocate a radix tree node under memory pressure, idr_alloc() returns
-ENOMEM rather than -ENOSPC, so only -ENOSPC should be translated to
-EBUSY.

>  	obj = idr_replace(&file_priv->object_idr, NULL, args->handle);
>  	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) {

[Severity: Medium]
Can checking obj->dma_buf here race with concurrent PRIME export and
corrupt file_priv->prime?

obj->dma_buf is a device-wide field protected by dev->object_name_lock,
whereas drm_gem_change_handle_ioctl() only holds the per-file
file_priv->prime.lock:

CPU0                                  CPU1
drm_gem_change_handle_ioctl()         drm_gem_prime_handle_to_fd()
  mutex_lock(&file_priv1->prime.lock)   mutex_lock(&file_priv2->prime.lock)
                                        mutex_lock(&dev->object_name_lock)
                                        export_and_register_object()
  if (obj->dma_buf)                       obj->dma_buf = dmabuf;
    <-- data race

Keying the file_priv->prime update on obj->dma_buf rather than checking
whether args->handle is present in file_priv->prime via
drm_prime_lookup_buf_by_handle() also mismanages the per-file PRIME cache:

1. If obj->dma_buf was set by exporting on another drm_file (or via
   another handle H1 on the same file_priv), changing a non-PRIME handle
   H2 (e.g. from DRM_IOCTL_GEM_OPEN or DRM_IOCTL_MODE_GETFB2) to H3 calls
   drm_prime_add_buf_handle() for H3 while drm_prime_remove_buf_handle()
   for H2 is a no-op.  When H1 is already in file_priv->prime, this inserts
   a duplicate dma_buf entry into file_priv->prime.dmabufs.

2. Conversely, when a PRIME-imported object (obj->import_attach != NULL)
   has obj->dma_buf == NULL (for example, if a handle is re-obtained via
   DRM_IOCTL_MODE_GETFB2 after handle_count previously dropped to zero)
   and is exported via drm_gem_prime_handle_to_dmabuf(), the
   (obj->import_attach->dmabuf, args->handle) entry is added to
   file_priv->prime without setting obj->dma_buf:

drivers/gpu/drm/drm_prime.c:drm_gem_prime_handle_to_dmabuf() {
    ...
	if (obj->import_attach) {
		dmabuf = obj->import_attach->dmabuf;
		get_dma_buf(dmabuf);
		goto out_have_obj;
	}
    ...
out_have_obj:
    ...
	ret = drm_prime_add_buf_handle(&file_priv->prime,
				       dmabuf, handle);
}

   Changing args->handle then skips the if (obj->dma_buf) block and leaves
   args->handle orphaned in file_priv->prime, triggering the WARN_ON() in
   drm_prime_destroy_file_private() and leaking the drm_prime_member and
   its dma_buf reference when the file is closed.

> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 9039a39c43243..bb350895625ce 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -672,8 +672,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),

[Severity: Medium]
Does re-enabling DRM_IOCTL_GEM_CHANGE_HANDLE leave stale entries in i915's
per-context VMA lookup cache (eb->gem_context->handles_vma)?

In i915, eb_lookup_vma() caches (handle -> vma) in
eb->gem_context->handles_vma and checks that radix tree before consulting
eb->file->object_idr:

drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:eb_lookup_vma() {
    ...
	if (likely(!eb->lut_size)) {
		rcu_read_lock();
		vma = radix_tree_lookup(&eb->gem_context->handles_vma, handle);
		if (likely(vma))
			vma = i915_vma_tryget(vma);
		rcu_read_unlock();
		if (likely(vma))
			return vma;
	}
    ...
}

Entries are only removed from ctx->handles_vma when the context is closed
or when obj->funcs->close (i915_gem_close_object()) runs.  Because
drm_gem_change_handle_ioctl() moves the handle in file_priv->object_idr
without invoking obj->funcs->close or notifying the driver, subsequent
DRM_IOCTL_I915_GEM_EXECBUFFER2 calls using the old handle (even after a
newly created object is assigned that handle ID) will resolve to the old
object's VMA, and repeated handle changes accumulate i915_lut_handle
entries on obj->lut_list.

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