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

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

[David Francis]

Make the following changes to CHANGE_HANDLE
- Changing a non-existent handle to itself is ENOENT
- idr_preload before idr_alloc
- reject new_handle = 0 with EINVAL

This patch will not be merged until the relevant igt-tests
(https://gitlab.freedesktop.org/fdavid-amd/igt-gpu-tools)
are reviewed, merged, and run to the satisfaction of everyone
involved.

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, 15 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index e3ed684ddcf2..c8f3bf27b7de 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1015,19 +1015,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)
 {
@@ -1039,20 +1026,29 @@ int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data,
 		return -EOPNOTSUPP;
 
 	/* idr_alloc() limitation. */
-	if (args->new_handle > INT_MAX)
+	if (args->new_handle == 0 || args->new_handle > INT_MAX)
 		return -EINVAL;
 	new_handle = args->new_handle;
 
-	if (args->handle == new_handle)
-		return 0;
+	if (args->handle == new_handle) {
+		spin_lock(&file_priv->table_lock);
+		if (idr_find(&file_priv->object_idr, args->handle))
+			ret = 0;
+		else
+			ret = -ENOENT;
+		spin_unlock(&file_priv->table_lock);
+		return ret;
+	}
 
 	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);
 
 	if (ret < 0) {
 		spin_unlock(&file_priv->table_lock);
+		idr_preload_end();
 		goto out_unlock;
 	}
 
@@ -1060,10 +1056,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

[PATCH v2] 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

See also the igt patch
tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE
on igt-dev

v2: new_handle = old_handle is EINVAL

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

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 6a86bd2a0343..81aee038e3b2 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)
 {
@@ -1042,20 +1029,22 @@ int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data,
 		return -EOPNOTSUPP;
 
 	/* idr_alloc() limitation. */
-	if (args->new_handle > INT_MAX)
+	if (args->new_handle == 0 || args->new_handle > INT_MAX)
 		return -EINVAL;
 	new_handle = args->new_handle;
 
 	if (args->handle == new_handle)
-		return 0;
+		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);
 
 	if (ret < 0) {
 		spin_unlock(&file_priv->table_lock);
+		idr_preload_end();
 		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 v2] drm/gem: Fix and enable CHANGE_HANDLE

[Sebastian Brzezinka]

Hi David,

On Wed Oct 7, 2026 at 4:10 PM CEST, 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
>
> See also the igt patch
> tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE
> on igt-dev
>
> v2: new_handle = old_handle is EINVAL
>
> 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>
> ---
I had a brief look at this series and it looks pretty good to me.
I would improve the commit messages so they describe not only what was
done, but also why. From my side, I ran the new tests on the try bot,
and they all pass.

https://patchwork.freedesktop.org/series/177021/#rev1
(re run doesn't pick up the new tests for some reason, soplease look
at rev1)

New tests have been introduced between CI_DRM_19403_full and Trybot_177021v1_full:
New IGT tests (11)

    igt@gem_change_handle@change_prime:
        Statuses : 6 pass(s)
        Exec time: [0.01, 0.04] s

    igt@gem_change_handle@invalid_handle:
        Statuses : 6 pass(s)
        Exec time: [0.01, 0.05] s

    igt@gem_change_handle@new_handle_equals_old_handle:
        Statuses : 4 pass(s)
        Exec time: [0.01, 0.03] s

    igt@gem_change_handle@new_handle_exceeds_int_max:
        Statuses : 7 pass(s)
        Exec time: [0.01, 0.04] s

    igt@gem_change_handle@new_handle_taken:
        Statuses : 6 pass(s)
        Exec time: [0.01, 0.03] s

    igt@gem_change_handle@new_handle_zero:
        Statuses : 6 pass(s)
        Exec time: [0.01, 0.04] s

    igt@gem_change_handle@race_change_change:
        Statuses : 6 pass(s)
        Exec time: [1.01, 1.04] s

    igt@gem_change_handle@race_change_create_close:
        Statuses : 7 pass(s)
        Exec time: [1.01, 1.04] s

    igt@gem_change_handle@race_change_export_import:
        Statuses : 6 pass(s)
        Exec time: [1.01, 1.03] s

    igt@gem_change_handle@sanity:
        Statuses : 6 pass(s)
        Exec time: [0.01, 0.03] s

    igt@kms_cursor_crc@cursor-onscreen-64x64@pipe-c-hdmi-a-2:
        Statuses : 1 pass(s)
        Exec time: [2.23] s

-- 
Best regards,
Sebastian