Skip to content

[Deepin-Kernel-SIG] [linux 6.18.y] [FROMLIST] [Security] drm/gem: Try to fix change_handle ioctl, attempt 4 #1832

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Avenger-285714 wants to merge 1 commit into
base: linux-6.18.y
Choose a base branch
from
+36 −37
Open

[Deepin-Kernel-SIG] [linux 6.18.y] [FROMLIST] [Security] drm/gem: Try to fix change_handle ioctl, attempt 4 #1832

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 34 additions & 36 deletions drivers/gpu/drm/drm_gem.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -965,64 +965,64 @@ 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, *idrobj;
int handle, ret;
struct drm_gem_object *obj;
int new_handle, ret;

if (!drm_core_check_feature(dev, DRIVER_GEM))
return -EOPNOTSUPP;

/* idr_alloc() limitation. */
if (args->new_handle > INT_MAX)
return -EINVAL;
handle = args->new_handle;
new_handle = args->new_handle;

obj = drm_gem_object_lookup(file_priv, args->handle);
if (!obj)
return -ENOENT;

if (args->handle == handle) {
ret = 0;
goto out;
}
if (args->handle == new_handle)
Comment on lines 992 to +996
return 0;

mutex_lock(&file_priv->prime.lock);

spin_lock(&file_priv->table_lock);

/* When create_tail allocs an obj idr, it needs to first alloc as NULL,
* then later replace with the correct object. This is not necessary
* here, because the only operations that could race are drm_prime
* bookkeeping, and we hold the prime lock.
*/
ret = idr_alloc(&file_priv->object_idr, obj, handle, handle + 1,
ret = idr_alloc(&file_priv->object_idr, NULL, new_handle, new_handle + 1,
GFP_NOWAIT);

if (ret < 0) {
spin_unlock(&file_priv->table_lock);
goto out_unlock;
}
if (ret < 0) {
spin_unlock(&file_priv->table_lock);
goto out_unlock;
}

idrobj = idr_replace(&file_priv->object_idr, NULL, handle);
if (idrobj != obj) {
idr_replace(&file_priv->object_idr, idrobj, handle);
idr_remove(&file_priv->object_idr, args->new_handle);
spin_unlock(&file_priv->table_lock);
ret = -ENOENT;
goto out_unlock;
}
obj = idr_replace(&file_priv->object_idr, NULL, args->handle);
if (IS_ERR_OR_NULL(obj)) {
Comment on lines +1010 to +1011

@sourcery-ai sourcery-ai Bot Jun 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (bug_risk): New handle change path can leave the old handle mapped to NULL if drm_prime_add_buf_handle() fails.

On failure, the new_handle entry is removed but the original args->handle mapping is not restored, leaving a NULL entry in the idr and dropping its GEM association. This changes behavior from the prior implementation, which preserved the old mapping until prime handle setup succeeded. Please either restore the original obj at args->handle on failure or reorder operations so args->handle is only replaced after drm_prime_add_buf_handle() succeeds.

idr_remove(&file_priv->object_idr, new_handle);
spin_unlock(&file_priv->table_lock);
ret = -ENOENT;
goto out_unlock;
}

spin_unlock(&file_priv->table_lock);

if (obj->dma_buf) {
ret = drm_prime_add_buf_handle(&file_priv->prime, obj->dma_buf,
handle);
new_handle);
if (ret < 0) {
spin_lock(&file_priv->table_lock);
idr_remove(&file_priv->object_idr, handle);
idr_remove(&file_priv->object_idr, new_handle);
spin_unlock(&file_priv->table_lock);
goto out_unlock;
Comment on lines 1023 to 1027
}
Expand All @@ -1034,14 +1034,12 @@ int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data,

spin_lock(&file_priv->table_lock);
idr_remove(&file_priv->object_idr, args->handle);
idrobj = idr_replace(&file_priv->object_idr, obj, handle);
obj = idr_replace(&file_priv->object_idr, obj, new_handle);
spin_unlock(&file_priv->table_lock);
WARN_ON(idrobj != NULL);
WARN_ON(obj != NULL);

out_unlock:
mutex_unlock(&file_priv->prime.lock);
out:
drm_gem_object_put(obj);

return ret;
}
Expand Down
3 changes: 2 additions & 1 deletion drivers/gpu/drm/drm_ioctl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -653,7 +653,8 @@ 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),
DRM_IOCTL_DEF(DRM_IOCTL_GEM_CHANGE_HANDLE, drm_gem_change_handle_ioctl, DRM_RENDER_ALLOW),
/* 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_MODE_GETRESOURCES, drm_mode_getresources, 0),

Expand Down
Loading