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 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Out-of-bounds reads in `check_atom_bios` when validating VBIOS headers, made directly reachable via the new user-supplied firmware file fallback.
--

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

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

This commit adds a fifth fallback method to discover an APU VBIOS by
requesting a firmware file when all other methods fail. The fallback uses
request_firmware() and allocates the VBIOS buffer using kmemdup() with the
exact firmware size.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
> index aa039e148a5ee..86064c753b099 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
[ ... ]
> @@ -457,6 +458,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 allocating the buffer to the
exact firmware size via kmemdup() exposes existing out-of-bounds reads in
check_atom_bios().

Looking at check_atom_bios(), the initial size check is:

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

But then it reads bios[0x49]:

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

If fw_size is exactly 0x49, bios[0x49] reads 1 byte past the end of the
kmemdup() buffer.

Similarly, check_atom_bios() later checks size against tmp before reading
4 bytes for the ATOM signature:

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

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

If size is exactly tmp (or up to tmp + 3), memcmp() will read up to 4 bytes
past the end of the allocation. 

Could a malformed firmware file loaded through this new path trigger KASAN
slab-out-of-bounds warnings or page faults due to these bounds checks?

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