Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues: - [High] TOCTOU race condition in UVD destroy ownership check allows bypassing the security check. - [High] Partial driver state modification during UVD destroy message validation leads to a permanent firmware resource leak. - [Medium] Un-rate-limited DRM_ERROR in user-triggerable path allows kernel log flooding. Pre-existing issues: - [High] Flawed loop logic and lockless design in UVD handle creation allows clients to create duplicate handles and shadow existing sessions. - [High] Integer overflow in amdgpu_uvd_cs_msg_decode allows bypassing buffer size validation, leading to out-of-bounds DMA writes by firmware. -- --- Patch [4]: [PATCH 4/5] drm/amdgpu: enforce UVD handle ownership on destroy --- commit 51bb2295b5cb365884456f0b29a575d54bdae4a7 Author: Junrui Luo <[email protected]> Subject: drm/amdgpu: enforce UVD handle ownership on destroy This patch updates the UVD command submission to validate handle ownership during destroy operations, similar to the decode path. It checks that the file pointer associated with the handle matches the submitting client. The mapping release is also hoisted above the loop to prevent reference leaks on error. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c > index e8b0c62f72be2..8d3e5435cf528 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c > @@ -918,9 +918,19 @@ static int amdgpu_uvd_cs_msg(struct amdgpu_uvd_cs_ctx > *ctx, > > case 2: > /* it's a destroy msg, free the handle */ > - for (i = 0; i < adev->uvd.max_handles; ++i) > - atomic_cmpxchg(&adev->uvd.handles[i], handle, 0); > amdgpu_bo_kunmap(bo); > + > + for (i = 0; i < adev->uvd.max_handles; ++i) { > + if (atomic_read(&adev->uvd.handles[i]) != handle) > + continue; > + > + if (adev->uvd.filp[i] != ctx->parser->filp) { [Severity: High] Does this check introduce a time-of-check to time-of-use race condition? In the case 0 (create) path, the slot is claimed via atomic_cmpxchg before filp[i] is updated: drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c:amdgpu_uvd_cs_msg() { ... if (!atomic_cmpxchg(&adev->uvd.handles[i], 0, handle)) { adev->uvd.filp[i] = ctx->parser->filp; return 0; } ... } If a victim thread is preempted right after the atomic_cmpxchg but before updating filp[i], filp[i] still retains its old value. If an attacker concurrently submits a destroy command and their filp matches that old value, can they pass this new ownership check and destroy the victim's newly allocated session? > + DRM_ERROR("UVD handle collision detected!\n"); [Severity: Medium] Is it possible for a user to flood the kernel log with this error message? Since this path is entirely controlled by user inputs, an attacker could repeatedly submit destroy messages for handles they do not own, triggering this un-rate-limited DRM_ERROR in a tight loop. > + return -EINVAL; > + } > + > + atomic_cmpxchg(&adev->uvd.handles[i], handle, 0); [Severity: High] Can returning -EINVAL here cause a permanent leak of UVD hardware sessions? If multiple slots happen to contain the same handle, this loop will clear the first matching slot using atomic_cmpxchg. When it reaches the second matching slot (owned by a different client), the filp check fails and the function returns -EINVAL. This aborts the command submission before the destroy command is sent to the firmware. However, because the first software slot is already zeroed out, amdgpu_uvd_free_handles() will never clean it up, leaving the hardware session leaked permanently. > + } > return 0; > > default: [Severity: High] This is a pre-existing issue, but does the loop logic in the case 0 (create) path allow clients to create duplicate handles and shadow existing sessions? drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c:amdgpu_uvd_cs_msg() { ... for (i = 0; i < adev->uvd.max_handles; ++i) { if (atomic_read(&adev->uvd.handles[i]) == handle) { ... } if (!atomic_cmpxchg(&adev->uvd.handles[i], 0, handle)) { adev->uvd.filp[i] = ctx->parser->filp; return 0; } } ... } The loop exits early upon finding a free slot, bypassing duplicate checks for the remaining slots. If an attacker creates a handle that is already owned by a victim, and the attacker obtains an earlier slot index, subsequent operations by the victim will hit the attacker's slot first and fail the ownership check. [Severity: High] This is a pre-existing issue, but does the 32-bit multiplication of user-controlled dimensions in amdgpu_uvd_cs_msg_decode() overflow? drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c:amdgpu_uvd_cs_msg_decode() { ... image_size = (ALIGN(width, 16) * ALIGN(height, 16) * 3) / 2; ... } The width and height dimensions read from the UVD message do not seem to be checked against upper bounds. By providing extremely large values, this calculation can overflow 32-bit arithmetic to a very small value, bypassing buffer size validation and allowing the firmware to perform out-of-bounds DMA writes. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
