[Deepin-Kernel-SIG] [linux 6.18.y] [FROMLIST] [Security] drm/gem: Try to fix change_handle ioctl, attempt 4#1832
Open
Avenger-285714 wants to merge 1 commit into
Conversation
[airlied: just added some comments on how to reenable]
On-list because the cat is out of the bag and we're clearly not good
enough to figure this out in private. The story thus far:
5e28b7b94408 ("drm: Set old handle to NULL before prime swap in
change_handle") tried to fix a race condition between the gem_close and
gem_change_handle ioctls, but got a few things wrong:
- There's a confusion with the local variable handle, which is actually
the new handle, and so the two-stage trick was actually applied to the
wrong idr slot. 7164d78559b0 ("drm/gem: fix race between
change_handle and handle_delete") tried to fix that by adding yet
another code block, but forgot to add the error handling. Which meant
we now have two paths, both kinda wrong.
- dc366607c41c ("drm: Replace old pointer to new idr") tried to apply
another fix, but inconsistently, again because of the handle confusion
- this would be the right fix (kinda, somewhat, it's a mess) if we'd
do the two-stage approach for the new handle. Except that wasn't the
intent of the original fix.
We also didn't have an igt merged for the original ioctl, which is a big
no-go. This was attempted to address off-list in the original bugfix,
and amd QA people claimed the bug was fixed now. Very clearly that's not
the case. Here's my attempt to sort this out:
- Rename the local variable to new_handle, the old aliasing with
args->handle is just too dangerously confusing.
- Merge the gem obj lookup with the two-stage idr_replace so that we
avoid getting ourselves confused there.
- This means we don't have a surplus temporary reference anymore, only
an inherited from the idr. A concurrent gem_close on the new_handle
could steal that. Fix that with the same two-stage approach
create_tail uses. This is a bit overkill as documented in the comment,
but I also don't trust my ability to understand this all correctly, so
go with the established pattern we have from other ioctls instead for
maximum paranoia.
- Adjust error paths. I've tried to make the error and success paths
common, because they are identical except for which handle is removed
and on which we call idr_replace to (re)install the object again. But
that made things messier to read, so I've left it at the more verbose
version, which unfortunately hides the symmetry in the entire code
flow a bit.
- While at it, also replace the 7 space indent with 1 tab.
And finally, because I flat out don't trust my abilities here at all
anymore:
- Disable the ioctl until we have the igt situation and everything else
sorted out on-list and with full consensus.
v2:
Sashiko noticed that I didn't handle the error path for idr_replace
correctly, it must be checked with IS_ERR_OR_NULL like in
gem_handle_delete. So yeah, definitely should just the existing paths
1:1 because this is endless amounts of tricky.
Also add the Fixes: line for the original ioctl, I forgot that too.
Reported-by: DARKNAVY (@DarkNavyOrg) <vr@darknavy.com>
Signed-off-by: Simona Vetter <simona.vetter@ffwll.ch>
Fixes: dc366607c41c ("drm: Replace old pointer to new idr")
Cc: syzbot+d7c9eed171647e421013@syzkaller.appspotmail.com
Cc: stable@vger.kernel.org
Cc: Edward Adam Davis <eadavis@qq.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: 5e28b7b94408 ("drm: Set old handle to NULL before prime swap in change_handle")
Cc: David Francis <David.Francis@amd.com>
Cc: Puttimet Thammasaeng <pwn8official@gmail.com>
Cc: Christian Koenig <Christian.Koenig@amd.com>
Fixes: 7164d78559b0 ("drm/gem: fix race between change_handle and handle_delete")
Cc: Zhenghang Xiao <kipreyyy@gmail.com>
Fixes: 5e28b7b94408 ("drm: Set old handle to NULL before prime swap in change_handle")
Reviewed-by: David Francis <David.Francis@amd.com>
Signed-off-by: Dave Airlie <airlied@redhat.com>
Link: https://patch.msgid.link/20260604194437.1725314-1-simona.vetter@ffwll.ch
[WangYuli: Fix conflicts]
Link: https://gitlab.freedesktop.org/drm/kernel/-/commit/1a4f03d22fb655e5f192244fb2c87d8066fcfca2
Signed-off-by: WangYuli <wangyl5933@chinaunicom.cn>
Reviewer's GuideDisables the insecure DRM GEM change_handle ioctl and rewrites its implementation to use a safer two-stage idr replacement pattern, renaming variables and aligning error handling with other GEM handle paths. Sequence diagram for updated GEM change_handle ioctl dispatchsequenceDiagram
participant Userspace
participant DRM_core
participant drm_invalid_op
Userspace->>DRM_core: DRM_IOCTL_GEM_CHANGE_HANDLE
DRM_core->>drm_invalid_op: drm_invalid_op
drm_invalid_op-->>DRM_core: -EINVAL (invalid ioctl)
DRM_core-->>Userspace: ioctl error (-EINVAL)
Flow diagram for new drm_gem_change_handle_ioctl idr handlingflowchart TD
A[Start drm_gem_change_handle_ioctl] --> B[drm_core_check_feature]
B -->|unsupported| Z[Return -EOPNOTSUPP]
B -->|supported| C[args->handle == new_handle?]
C -->|yes| Z2[Return 0]
C -->|no| D[mutex_lock prime.lock]
D --> E[spin_lock table_lock]
E --> F[idr_alloc object_idr NULL new_handle]
F -->|ret < 0| G[spin_unlock table_lock]
G --> H[goto out_unlock]
F -->|ret >= 0| I[obj = idr_replace object_idr NULL args->handle]
I -->|IS_ERR_OR_NULL obj| J[idr_remove object_idr new_handle]
J --> K[spin_unlock table_lock]
K --> H
I -->|obj valid| L[spin_unlock table_lock]
L --> M[obj->dma_buf?]
M -->|yes| N[drm_prime_add_buf_handle prime obj->dma_buf new_handle]
N -->|ret < 0| O[spin_lock table_lock]
O --> P[idr_remove object_idr new_handle]
P --> Q[spin_unlock table_lock]
Q --> H
M -->|no or success| R[spin_lock table_lock]
R --> S[idr_remove object_idr args->handle]
S --> T[obj = idr_replace object_idr obj new_handle]
T --> U[spin_unlock table_lock]
U --> V[WARN_ON obj != NULL]
V --> H
H[out_unlock] --> W[mutex_unlock prime.lock]
W --> X[Return ret]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="drivers/gpu/drm/drm_gem.c" line_range="1010-1011" />
<code_context>
- ret = -ENOENT;
- goto out_unlock;
- }
+ 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);
</code_context>
<issue_to_address>
**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.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+1010
to
+1011
| obj = idr_replace(&file_priv->object_idr, NULL, args->handle); | ||
| if (IS_ERR_OR_NULL(obj)) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Pull request overview
Disables the DRM GEM CHANGE_HANDLE ioctl at the core ioctl dispatcher due to unresolved security/race concerns, while updating drm_gem_change_handle_ioctl() with clearer variable naming and revised idr/prime bookkeeping in preparation for a future, properly-tested re-enable.
Changes:
- Route
DRM_IOCTL_GEM_CHANGE_HANDLEtodrm_invalid_op(ioctl disabled). - Add a detailed comment block describing why the ioctl is disabled and prerequisites for re-enabling.
- Rework
drm_gem_change_handle_ioctl()internals (variable rename tonew_handle, revised idr_alloc/idr_replace flow, adjusted error paths, indentation fixes).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| drivers/gpu/drm/drm_ioctl.c | Disables DRM_IOCTL_GEM_CHANGE_HANDLE by mapping it to drm_invalid_op and adds a reference comment. |
| drivers/gpu/drm/drm_gem.c | Documents disablement and refactors drm_gem_change_handle_ioctl() handle/idr management logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
1023
to
1027
| 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
992
to
+996
| 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) |
| 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 */ |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
[airlied: just added some comments on how to reenable] On-list because the cat is out of the bag and we're clearly not good enough to figure this out in private. The story thus far:
5e28b7b94408 ("drm: Set old handle to NULL before prime swap in change_handle") tried to fix a race condition between the gem_close and gem_change_handle ioctls, but got a few things wrong:
There's a confusion with the local variable handle, which is actually the new handle, and so the two-stage trick was actually applied to the wrong idr slot. 7164d78559b0 ("drm/gem: fix race between change_handle and handle_delete") tried to fix that by adding yet another code block, but forgot to add the error handling. Which meant we now have two paths, both kinda wrong.
dc366607c41c ("drm: Replace old pointer to new idr") tried to apply another fix, but inconsistently, again because of the handle confusion
We also didn't have an igt merged for the original ioctl, which is a big no-go. This was attempted to address off-list in the original bugfix, and amd QA people claimed the bug was fixed now. Very clearly that's not the case. Here's my attempt to sort this out:
Rename the local variable to new_handle, the old aliasing with args->handle is just too dangerously confusing.
Merge the gem obj lookup with the two-stage idr_replace so that we avoid getting ourselves confused there.
This means we don't have a surplus temporary reference anymore, only an inherited from the idr. A concurrent gem_close on the new_handle could steal that. Fix that with the same two-stage approach create_tail uses. This is a bit overkill as documented in the comment, but I also don't trust my ability to understand this all correctly, so go with the established pattern we have from other ioctls instead for maximum paranoia.
Adjust error paths. I've tried to make the error and success paths common, because they are identical except for which handle is removed and on which we call idr_replace to (re)install the object again. But that made things messier to read, so I've left it at the more verbose version, which unfortunately hides the symmetry in the entire code flow a bit.
While at it, also replace the 7 space indent with 1 tab.
And finally, because I flat out don't trust my abilities here at all anymore:
v2:
Sashiko noticed that I didn't handle the error path for idr_replace correctly, it must be checked with IS_ERR_OR_NULL like in gem_handle_delete. So yeah, definitely should just the existing paths 1:1 because this is endless amounts of tricky.
Also add the Fixes: line for the original ioctl, I forgot that too.
Reported-by: DARKNAVY (@DarkNavyOrg) vr@darknavy.com
Fixes: dc366607c41c ("drm: Replace old pointer to new idr")
Cc: syzbot+d7c9eed171647e421013@syzkaller.appspotmail.com
Cc: stable@vger.kernel.org
Cc: Edward Adam Davis eadavis@qq.com
Cc: Dave Airlie airlied@redhat.com
Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com
Cc: Maxime Ripard mripard@kernel.org
Cc: Thomas Zimmermann tzimmermann@suse.de
Fixes: 5e28b7b94408 ("drm: Set old handle to NULL before prime swap in change_handle")
Cc: David Francis David.Francis@amd.com
Cc: Puttimet Thammasaeng pwn8official@gmail.com
Cc: Christian Koenig Christian.Koenig@amd.com
Fixes: 7164d78559b0 ("drm/gem: fix race between change_handle and handle_delete")
Cc: Zhenghang Xiao kipreyyy@gmail.com
Fixes: 5e28b7b94408 ("drm: Set old handle to NULL before prime swap in change_handle")
Reviewed-by: David Francis David.Francis@amd.com
Link: https://patch.msgid.link/20260604194437.1725314-1-simona.vetter@ffwll.ch
[WangYuli: Fix conflicts]
Link: https://gitlab.freedesktop.org/drm/kernel/-/commit/1a4f03d22fb655e5f192244fb2c87d8066fcfca2
Summary by Sourcery
Disable the drm GEM change_handle ioctl for security and process reasons while tightening its internal handle management logic to prevent races and inconsistencies.
Bug Fixes:
Enhancements: