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 <[email protected]> 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/[email protected]?part=1
