Re: [PATCH AUTOSEL 6.18-6.12] drm/amdgpu: validate and share PSP fw_pri_buf copies via psp_copy_fw

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Unvalidated `pos` parameter in `amdgpu_psp_vbflash_write` allows out-of-bounds heap write.
- [High] Concurrent sysfs read and write operations on `psp_vbflash` can cause a Use-After-Free on `vbflash_tmp_buf`.
- [High] Uninitialized kernel heap memory can be leaked to SPI ROM due to lack of buffer zeroing.
--

commit 0f415f32d1c8f6a02dd5dfbd6b22a7a6a71cda35
Author: Candice Li <candice.li@amd.com>
Subject: drm/amdgpu: validate and share PSP fw_pri_buf copies via psp_copy_fw

Change psp_copy_fw from void to int: return -ENODEV when drm_dev_enter
fails, and -EINVAL when the image size is zero or larger than the
1 MiB PSP private buffer. Replace open-coded memset/memcpy into fw_pri_buf
with psp_copy_fw.

> 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
[ ... ]

While reviewing the changes in this file, I noticed a few pre-existing issues
in the psp_vbflash sysfs handlers that might need attention:

drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c:amdgpu_psp_vbflash_write() {
	...
	if (!adev->psp.vbflash_tmp_buf) {
		adev->psp.vbflash_tmp_buf = kvmalloc(AMD_VBIOS_FILE_MAX_SIZE_B,
						     GFP_KERNEL);
		if (!adev->psp.vbflash_tmp_buf)
			return -ENOMEM;
	}

	mutex_lock(&adev->psp.mutex);
	memcpy(adev->psp.vbflash_tmp_buf + pos, buffer, count);
	...
}

[Severity: Critical]
This is a pre-existing issue, but does this code overflow vbflash_tmp_buf?

Because the size is set to 0 in psp_vbflash_bin_attr, the sysfs core does
not restrict the pos offset. The function checks if vbflash_image_size
exceeds the maximum size, but it does not validate if pos + count is larger
than AMD_VBIOS_FILE_MAX_SIZE_B.

If userspace calls pwrite on the psp_vbflash sysfs file with a large pos,
could this write arbitrarily far past the allocated heap buffer?


drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c:amdgpu_psp_vbflash_read() {
	...
rel_buf:
	kvfree(adev->psp.vbflash_tmp_buf);
	adev->psp.vbflash_tmp_buf = NULL;
	...
}

[Severity: High]
This is a pre-existing issue, but could this cause a use-after-free on
vbflash_tmp_buf?

The sysfs handlers amdgpu_psp_vbflash_read() and amdgpu_psp_vbflash_write()
can be called concurrently. In both amdgpu_psp_vbflash_read() and the error
path of amdgpu_psp_vbflash_write(), kvfree() is called without holding
adev->psp.mutex.

If a concurrent write is executing memcpy() to the buffer while holding the
mutex, and another thread frees it outside the lock here, could this result
in memory corruption?


drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c:amdgpu_psp_vbflash_write() {
	...
	if (!adev->psp.vbflash_tmp_buf) {
		adev->psp.vbflash_tmp_buf = kvmalloc(AMD_VBIOS_FILE_MAX_SIZE_B,
						     GFP_KERNEL);
	...
}

drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c:amdgpu_psp_vbflash_read() {
	...
	memcpy_toio(fw_pri_cpu_addr, adev->psp.vbflash_tmp_buf,
		    adev->psp.vbflash_image_size);
	...
}

[Severity: High]
This is a pre-existing issue, but does this leak uninitialized kernel memory?

In amdgpu_psp_vbflash_write(), the buffer is allocated using kvmalloc()
which does not zero the memory. If userspace writes a small amount of data
at a non-zero offset, the beginning of the buffer remains uninitialized.

When amdgpu_psp_vbflash_read() later copies vbflash_image_size bytes from
the start of this buffer to the GPU VRAM via memcpy_toio(), could this
expose uninitialized kernel heap data to the hardware?

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