Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Zero-sized firmware sections result in a NULL `section->mem` pointer which is subsequently dereferenced unconditionally. - [High] Missing bounds checks and an integer overflow in `panthor_fw_read_build_info()` allow out-of-bounds kernel memory reads when parsing metadata. -- --- Patch [1]: [PATCH] drm/panthor: reject firmware sections with oversized data --- commit d0d7589e74bd6422c8113d23148f0b37d9037277 Author: Osama Abdelkader <[email protected]> drm/panthor: reject firmware sections with oversized data 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, this could 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); Reject section entries whose initial data is larger than the section size. Fixes: 2718d91816ee ("drm/panthor: Add the FW logical block") Cc: [email protected] Signed-off-by: Osama Abdelkader <[email protected]> > diff --git a/drivers/gpu/drm/panthor/panthor_fw.c > b/drivers/gpu/drm/panthor/panthor_fw.c > index de8e6689a869a..b73cf054d33ad 100644 > --- a/drivers/gpu/drm/panthor/panthor_fw.c > +++ b/drivers/gpu/drm/panthor/panthor_fw.c [ ... ] > @@ -595,6 +596,13 @@ static int panthor_fw_load_section_entry(struct > panthor_device *ptdev, > return -EINVAL; > } > > + section_size = hdr.va.end - hdr.va.start; > + data_size = hdr.data.end - hdr.data.start; > + if (data_size > section_size) { > + drm_err(&ptdev->base, "Firmware corrupted, section data exceeds > section size\n"); > + return -EINVAL; > + } > + [Severity: High] This is a pre-existing issue, but if a crafted firmware file provides a section entry where hdr.va.end == hdr.va.start, section_size evaluates to 0. Since this patch ensures data_size is not greater than section_size, data_size would also be 0, allowing the check to pass. Later in panthor_fw_load_section_entry(), BO creation is skipped when section_size is 0, leaving section->mem as NULL: if (section_size) { ... section->mem = panthor_kernel_bo_create(...); ... } Since the section was already added to the ptdev->fw->sections list, could iterating over this list later in panthor_reload_fw_sections() trigger a NULL pointer dereference when calling panthor_fw_init_section_mem()? static void panthor_fw_init_section_mem(...) { bool was_mapped = !!section->mem->kmap; // Unconditional dereference [ ... ] Also in drivers/gpu/drm/panthor/panthor_fw.c, looking at the panthor_fw_read_build_info() function: drivers/gpu/drm/panthor/panthor_fw.c:panthor_fw_read_build_info() { ... if (hdr.meta_start > fw->size || hdr.meta_start + hdr.meta_size > fw->size) { drm_err(&ptdev->base, "Firmware build info corrupt\n"); /* We don't need the build info, so continue */ return 0; } [Severity: High] This isn't a bug introduced by this patch, but does the 32-bit addition of hdr.meta_start and hdr.meta_size allow an integer overflow to bypass this bounds check? If bypassed, a crafted firmware could trigger an out-of-bounds read on the subsequent memcmp(), which reads header_len (9) bytes without verifying that the remaining firmware size is at least 9 bytes: if (memcmp(git_sha_header, fw->data + hdr.meta_start, header_len)) { /* Not the expected header, this isn't metadata we understand */ return 0; } Additionally, if hdr.meta_size is 0, could the check for the NULL terminator evaluate to an out-of-bounds index (hdr.meta_start - 1)? /* Check that the git SHA is NULL terminated as expected */ if (fw->data[hdr.meta_start + hdr.meta_size - 1] != '\0') { drm_warn(&ptdev->base, "Firmware's git sha is not NULL terminated\n"); /* Don't treat as fatal */ return 0; } ... } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
