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