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