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