Re: [PATCH] drm/panthor: return error on truncated firmware

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Missing length validation between firmware section data size and virtual address size causes a heap buffer overflow.
- [High] panthor_fw_read_build_info() suffers from integer overflows and insufficient bounds checks, enabling out-of-bounds memory reads via a malformed firmware file.
- [High] Empty firmware sections bypass allocation but are added to the section list, causing NULL pointer dereferences during initialization and resets.
- [High] Missing structure size validation when converting firmware MCU virtual addresses allows out-of-bounds kernel reads and writes.
--

--- Patch [1]: [PATCH] drm/panthor: return error on truncated firmware ---
commit e5c6b49a119e7aa698d6e5f3a9176615e1f26472
Author: Osama Abdelkader <osama.abdelkader@gmail.com>

drm/panthor: return error on truncated firmware

panthor_fw_load() detects truncated firmware images, but jumps to the
common cleanup path without setting ret. Set ret to -EINVAL before
leaving the truncated-image path.

Cc: stable@vger.kernel.org

> diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
> index de8e6689a869a..90f59d782a809 100644
> --- a/drivers/gpu/drm/panthor/panthor_fw.c
> +++ b/drivers/gpu/drm/panthor/panthor_fw.c
> @@ -829,6 +829,7 @@ static int panthor_fw_load(struct panthor_device *ptdev)
>  	}
>  
>  	if (hdr.size > iter.size) {
> +		ret = -EINVAL;
>  		drm_err(&ptdev->base, "Firmware image is truncated\n");
>  		goto out;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the firmware
loading code, I noticed a potential integer overflow in
panthor_fw_read_build_info() when parsing firmware headers:

    if (hdr.meta_start > fw->size ||
        hdr.meta_start + hdr.meta_size > fw->size) {

Could this bounds check be bypassed if a malformed firmware provides values
for hdr.meta_start and hdr.meta_size such that their 32-bit sum wraps to 0?

If the check is bypassed, could the memcmp call read out-of-bounds?

    if (memcmp(git_sha_header, fw->data + hdr.meta_start, header_len)) {

And could this array access evaluate to an extremely large index (0xFFFFFFFF)
if the sum evaluates to 0?

    if (fw->data[hdr.meta_start + hdr.meta_size - 1] != '\0') {

[Severity: High]
This is a pre-existing issue, but does the driver mishandle zero-sized
firmware sections?

In panthor_fw_load_section_entry(), if hdr.va.end == hdr.va.start, the size
evaluates to 0:

    section_size = hdr.va.end - hdr.va.start;
    if (section_size) {

Zero-sized sections skip section->mem allocation but are still added to the
FW sections list.

Later, panthor_reload_fw_sections() iterates through all sections and calls
panthor_fw_init_section_mem():

    panthor_fw_init_section_mem(ptdev, section);

Would this cause a NULL pointer dereference when panthor_fw_init_section_mem()
unconditionally dereferences section->mem?

    bool was_mapped = !!section->mem->kmap;

[Severity: Critical]
This is a pre-existing issue, but is there a missing bounds check between the
section data size and the allocated section size?

In panthor_fw_load_section_entry(), the data size to copy is calculated
without validating it against the allocated section_size:

    section->data.size = hdr.data.end - hdr.data.start;

If a crafted firmware sets data.size larger than the allocated memory,
would this cause a heap buffer overflow in panthor_fw_init_section_mem()?

    memcpy(section->mem->kmap, section->data.buf, section->data.size);

Additionally, if the section->data.size exceeds the BO size, could this
memset underflow the size calculation, leading to a massive out-of-bounds
zeroing of kernel memory?

    memset(section->mem->kmap + section->data.size, 0,
           panthor_kernel_bo_size(section->mem) - section->data.size);

[Severity: High]
This isn't a bug introduced by this patch, but does the firmware MCU virtual
address conversion miss a structure size validation?

In iface_fw_to_cpu_addr(), it validates that the start address falls within
the shared memory region:

    if (mcu_va < shared_mem_start || mcu_va >= shared_mem_end)
        return NULL;

However, in panthor_init_cs_iface(), this pointer is cast to an interface
structure without verifying there is enough space left in the region:

    cs_iface->input = iface_fw_to_cpu_addr(ptdev, cs_iface->control->input_va);

If a malformed firmware provides an offset pointing to the very end of the
shared memory region, could subsequent reads or writes to the structure's
fields access unallocated or unrelated kernel memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714163056.22329-1-osama.abdelkader@gmail.com?part=1