Re: [PATCH AUTOSEL 6.18-6.6] drm/amdgpu: fix buffer overflow during vBIOS update

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Incorrect accumulation of vbflash_image_size allows out-of-bounds reads and uninitialized memory disclosure.
- [High] Missing mutex locking during vbflash_tmp_buf allocation, access, and freeing leads to Use-After-Free, double frees, and memory leaks.
- [High] TOCTOU race condition on vbflash_image_size leads to an out-of-bounds write into VRAM.
- [Medium] Unhandled cleanup of vbflash_tmp_buf causes a 16MB memory leak on driver teardown.
--

commit 8e4215e84ef10c9caba9352b46954259d8b626c5
Author: Shiwu Zhang <shiwu.zhang@amd.com>

drm/amdgpu: fix buffer overflow during vBIOS update

This commit clamps the buffer position for writing by setting the bin
attribute to the maximum buffer size. This allows the VFS layer to block
out-of-bounds access.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> @@ -4272,7 +4272,7 @@ static ssize_t amdgpu_psp_vbflash_read(struct file *filp, struct kobject *kobj,
>   */
>  static const struct bin_attribute psp_vbflash_bin_attr = {
>  	.attr = {.name = "psp_vbflash", .mode = 0660},
> -	.size = 0,
> +	.size = AMD_VBIOS_FILE_MAX_SIZE_B,
>  	.write = amdgpu_psp_vbflash_write,
>  	.read = amdgpu_psp_vbflash_read,
>  };

While reviewing this fix, I noticed a few pre-existing issues in the functions
referenced by this sysfs attribute.

[Severity: High]
This isn't a bug introduced by this patch, but does amdgpu_psp_vbflash_write()
incorrectly accumulate the byte count instead of tracking a high-water mark?

If a user with access to the sysfs node writes 16MB twice at pos 0, the first
write passes the VFS checks and sets the size to 16MB. The second write also
passes because 16MB > 16MB is false, setting the accumulated size to 32MB:

amdgpu_psp_vbflash_write() {
        ...
        memcpy(adev->psp.vbflash_tmp_buf + pos, buffer, count);
        adev->psp.vbflash_image_size += count;
        ...
}

Later, when reading, it uses this oversized vbflash_image_size:

amdgpu_psp_vbflash_read() {
        ...
        memcpy_toio(fw_pri_cpu_addr, adev->psp.vbflash_tmp_buf,
                    adev->psp.vbflash_image_size);
        ...
}

Could this result in a heap out-of-bounds read that copies sensitive kernel
data into the VRAM buffer?

[Severity: High]
This is a pre-existing issue, but is there a missing mutex around the buffer
allocation, access, and freeing?

Sysfs write and read functions can execute concurrently. In
amdgpu_psp_vbflash_write(), the buffer is allocated and freed outside of
adev->psp.mutex:

amdgpu_psp_vbflash_write() {
        if (pos >= AMD_VBIOS_FILE_MAX_SIZE_B) {
                kvfree(adev->psp.vbflash_tmp_buf);
                ...
        }
        ...
        if (!adev->psp.vbflash_tmp_buf) {
                adev->psp.vbflash_tmp_buf = kvmalloc(...);
        ...
}

Meanwhile, concurrent threads might be executing memcpy in write or
memcpy_toio in read. Could one thread trigger a size-limit error and free the
buffer while another is actively copying to or from it, leading to a
use-after-free?

[Severity: High]
This isn't a bug introduced by this patch, but is there a TOCTOU race
condition on vbflash_image_size in amdgpu_psp_vbflash_read()?

The function performs lockless reads of vbflash_image_size. First it sizes
the VRAM buffer:

amdgpu_psp_vbflash_read() {
        ...
        ret = amdgpu_bo_create_kernel(adev, adev->psp.vbflash_image_size,
                                      ... &fw_pri_cpu_addr);
        ...
}

And then it uses vbflash_image_size again for the copy:

amdgpu_psp_vbflash_read() {
        ...
        memcpy_toio(fw_pri_cpu_addr, adev->psp.vbflash_tmp_buf,
                    adev->psp.vbflash_image_size);
        ...
}

If a concurrent sysfs write increments vbflash_image_size between these two
reads, will memcpy_toio copy more bytes than the BO can hold and write past
the end of the allocated VRAM buffer?

[Severity: Medium]
This is a pre-existing issue, but does this code leak the 16MB buffer on
driver teardown?

If a user stages a firmware update by writing to the sysfs node but never
triggers the read, the buffer remains allocated. When the driver is unbound,
teardown functions like psp_sw_fini() do not appear to check for or free
vbflash_tmp_buf:

psp_sw_fini() {
        ...
        /* missing kvfree(adev->psp.vbflash_tmp_buf) here? */
        ...
}

Can this result in a memory leak every time the device is unbound?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831133314.4125787-406-sashal@kernel.org?part=1