Re: [PATCH 4/5] drm/amdgpu: enforce UVD handle ownership on destroy

[sashiko-bot@kernel.org]

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 <moonafterrain@outlook.com>
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/20260811-amdgpu-fixes-v1-0-4954a417b8ff@outlook.com?part=4