Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] Missing size validation when converting firmware input/output virtual addresses to CPU pointers allows out-of-bounds memory access. -- --- Patch [1]: [PATCH] drm/panthor: fix firmware control interface bounds checks --- commit e72a8d4004139e04a7daa2f7b2eeb68178eeaccf Author: Osama Abdelkader <[email protected]> drm/panthor: fix firmware control interface bounds checks This commit fixes firmware control interface bounds checks by using 64-bit arithmetic for computed offsets. It also correctly validates against the actual firmware control interface structure sizes. > diff --git a/drivers/gpu/drm/panthor/panthor_fw.c > b/drivers/gpu/drm/panthor/panthor_fw.c > index 90f59d782a809..f9da7e4ab7368 100644 > --- a/drivers/gpu/drm/panthor/panthor_fw.c > +++ b/drivers/gpu/drm/panthor/panthor_fw.c [ ... ] > @@ -878,14 +878,15 @@ static int panthor_init_cs_iface(struct panthor_device > *ptdev, > struct panthor_fw_csg_iface *csg_iface = > panthor_fw_get_csg_iface(ptdev, csg_idx); > struct panthor_fw_cs_iface *cs_iface = > &ptdev->fw->iface.streams[csg_idx][cs_idx]; > u64 shared_section_sz = > panthor_kernel_bo_size(ptdev->fw->shared_section->mem); > - u32 iface_offset = CSF_GROUP_CONTROL_OFFSET + > - (csg_idx * glb_iface->control->group_stride) + > + u64 iface_offset = CSF_GROUP_CONTROL_OFFSET + > + ((u64)csg_idx * glb_iface->control->group_stride) + > CSF_STREAM_CONTROL_OFFSET + > - (cs_idx * csg_iface->control->stream_stride); > + ((u64)cs_idx * csg_iface->control->stream_stride); > struct panthor_fw_cs_iface *first_cs_iface = > panthor_fw_get_cs_iface(ptdev, 0, 0); > > - if (iface_offset + sizeof(*cs_iface) >= shared_section_sz) > + if (iface_offset > shared_section_sz || > + sizeof(*cs_iface->control) > shared_section_sz - iface_offset) > return -EINVAL; > > spin_lock_init(&cs_iface->lock); [Severity: High] This is a pre-existing issue, but while you are adding bounds checking for the control structures, does this code also need size validation for the input and output structures? Looking at panthor_init_cs_iface() right below this bounds check: cs_iface->input = iface_fw_to_cpu_addr(ptdev, cs_iface->control->input_va); cs_iface->output = iface_fw_to_cpu_addr(ptdev, cs_iface->control->output_va); The iface_fw_to_cpu_addr() function only verifies that the starting virtual address is within the shared_section bounds: if (mcu_va < shared_mem_start || mcu_va >= shared_mem_end) return NULL; If a malformed firmware provides an input_va or output_va near the very end of the shared section (for example, shared_mem_end - 1), wouldn't the driver read or write past the mapped memory boundary since the structure sizes aren't validated to fit within the remaining space? This same pattern appears to exist for the group input/output pointers in panthor_init_csg_iface() and the global input/output pointers in panthor_fw_init_ifaces(). -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
