Re: [PATCH v2] drm/amd/amdgpu: add firmware file fallback for APU VBIOS discovery

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Insufficient size bounds checks in `check_atom_bios()` lead to out-of-bounds memory reads when validating the VBIOS image.
--

commit f277c5eb3a5ff47c43a2c9dc08a427a7be71788e
Author: Oz Tiram <oz@shift-computing.de>

drm/amd/amdgpu: add firmware file fallback for APU VBIOS discovery

This patch adds a fallback method to load the APU VBIOS from a firmware file
when all other discovery methods (VFCT, VRAM BAR, ROM BAR, platform BIOS)
fail. This allows hybrid graphics systems with incorrect ACPI metadata to
initialize successfully by placing an extracted VBIOS in the firmware path.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
> index 3ebdd792feec1..64b226cb17e3a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c

[ ... ]

> @@ -463,6 +464,28 @@ static bool amdgpu_get_bios_apu(struct amdgpu_device *adev)
>  		goto success;
>  	}
>  
> +	{
> +		const struct firmware *fw;
> +		char fw_name[32];
> +		size_t fw_size;
> +
> +		snprintf(fw_name, sizeof(fw_name), "amdgpu/%04x_%04x.bin",
> +			 adev->pdev->vendor, adev->pdev->device);
> +		if (request_firmware(&fw, fw_name, adev->dev) == 0) {
> +			adev->bios = kmemdup(fw->data, fw->size, GFP_KERNEL);
> +			fw_size = fw->size;
> +			release_firmware(fw);
> +			if (!adev->bios || !check_atom_bios(adev, fw_size)) {
                                                           ^^^^^^^^^^^^^^^^^^

[Severity: High]
This isn't a bug introduced by this patch, but passing an arbitrary firmware
file size to check_atom_bios() can trigger out-of-bounds reads due to
insufficient bounds checks in that function.

If the firmware file size is exactly 73 bytes (size == 0x49), the initial
check in check_atom_bios() passes:

check_atom_bios() {
	...
	if (!bios || size < 0x49) {
		...
		return false;
	}
	...

However, the subsequent header read accesses bios[0x49], which is 1 byte
past the end of a 73-byte buffer:

	...
	bios_header_start = bios[0x48] | (bios[0x49] << 8);
	...

Similarly, if the firmware size is equal to tmp, the length check passes:

	...
	tmp = bios_header_start + 4;
	if (size < tmp) {
		...
		return false;
	}
	...

But the memcmp() that follows will read 4 bytes starting exactly at the
end of the allocated buffer:

	...
	if (!memcmp(bios + tmp, "ATOM", 4) ||
	    !memcmp(bios + tmp, "MOTA", 4)) {
	...

Could this lead to memory safety issues if a malformed firmware file is
provided? Should the checks in check_atom_bios() be updated to ensure size
is strictly greater than 0x49, and that size >= tmp + 4 before the memcmp()
evaluation?

> +				amdgpu_bios_release(adev);
> +			} else {
> +				adev->bios_size = fw_size;
> +				dev_info(adev->dev, "Fetched VBIOS from firmware file %s\n",
> +					 fw_name);
> +				goto success;
> +			}
> +		}
> +	}
> +
>  	dev_err(adev->dev, "Unable to locate a BIOS ROM\n");
>  	return false;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260705100436.6877-1-oz@shift-computing.de?part=1