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

[sashiko-bot@kernel.org]

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

New issues:
- [High] The firmware file fallback for APU VBIOS discovery does not validate the loaded firmware with `check_atom_bios()`, potentially leading to a kernel panic or out-of-bounds reads.

Pre-existing issues:
- [High] Pre-existing off-by-one boundary checks in `check_atom_bios()` allow out-of-bounds memory reads when evaluating truncated or malformed VBIOS images.
--

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

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

This patch adds a fifth fallback path for APU VBIOS discovery using a
firmware file loaded via request_firmware(). This allows a VBIOS image
extracted from running hardware to be used when other methods like VFCT
or ROM BAR fail.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
> index aa039e148a5ee..491f88f495a66 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c

[ ... ]

> @@ -457,6 +458,24 @@ static bool amdgpu_get_bios_apu(struct amdgpu_device *adev)
>  		goto success;
>  	}
>  
> +	{
> +		const struct firmware *fw;
> +		char fw_name[32];
> +
> +		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);
> +			adev->bios_size = fw->size;
> +			release_firmware(fw);
> +			if (adev->bios) {

[Severity: High]
Does this new firmware fallback path need to validate the fetched memory using
check_atom_bios()?

All other VBIOS discovery paths appear to validate the fetched memory before
accepting the BIOS image. If the provided firmware file is empty (size 0),
kmemdup() returns ZERO_SIZE_PTR (which evaluates to true). The driver will
later dereference this pointer during ATOM BIOS parsing, which could cause a
kernel page fault.

Additionally, if the firmware is malformed or undersized, the ATOM BIOS parser
could read out-of-bounds.

[Severity: High]
This is a pre-existing issue, but check_atom_bios() appears to have off-by-one
boundary checks that can allow out-of-bounds memory reads when evaluating
malformed or truncated VBIOS images:

drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c:check_atom_bios() {
    ...
	if (!bios || size < 0x49) {
		dev_dbg(adev->dev, "VBIOS mem is null or mem size is wrong\n");
		return false;
	}

	...

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

If size is exactly 0x49 (73 bytes), the check size < 0x49 passes, but the code
then accesses bios[0x49], which is the 74th byte, resulting in a 1-byte
out-of-bounds read.

There is another similar boundary check later in the same function:

drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c:check_atom_bios() {
    ...
	tmp = bios_header_start + 4;
	if (size < tmp) {
		dev_dbg(adev->dev, "VBIOS header is broken\n");
		return false;
	}

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

If the buffer size is exactly equal to tmp, size < tmp passes, but the
subsequent memcmp() reads 4 bytes starting at tmp, which is completely past
the end of the allocated buffer. Can these boundary checks be updated to
prevent KASAN out-of-bounds warnings when parsing VBIOS blobs?

> +				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/20260621173211.28443-1-oz@shift-computing.de?part=1