Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [Critical] Time-of-Check to Time-of-Use (TOCTOU) vulnerability in firmware interface initialization allows kernel memory corruption. - [High] Integer overflow and incorrect structure size in firmware bounds checks on 32-bit architectures. - [High] NULL pointer dereference when firmware provides a 0-sized shared section. -- --- Patch [1]: [PATCH] drm/panthor: validate firmware interface structure sizes --- commit a126d5ded05975c89ebf69bcb6a208082b1f032d Author: Osama Abdelkader <[email protected]> drm/panthor: validate firmware interface structure sizes iface_fw_to_cpu_addr() only checks that the firmware-provided MCU virtual address points inside the shared section. The returned pointer is later used as a full firmware interface structure, so accepting an address near the end of the shared section can still lead to out-of-bounds accesses. Pass the expected object size to iface_fw_to_cpu_addr() and reject ranges that do not fit entirely in the shared section. > diff --git a/drivers/gpu/drm/panthor/panthor_fw.c > b/drivers/gpu/drm/panthor/panthor_fw.c > index 90f59d782a809..1697305c7b0e9 100644 > --- a/drivers/gpu/drm/panthor/panthor_fw.c > +++ b/drivers/gpu/drm/panthor/panthor_fw.c [ ... ] > @@ -890,8 +896,10 @@ static int panthor_init_cs_iface(struct panthor_device > *ptdev, > > spin_lock_init(&cs_iface->lock); [Severity: High] This is a pre-existing issue, but could the bounds check earlier in panthor_init_cs_iface() be vulnerable to a 32-bit integer overflow? panthor_fw.c:panthor_init_cs_iface() { ... if (iface_offset + sizeof(*cs_iface) >= shared_section_sz) return -EINVAL; ... } If a malicious firmware provides crafted strides, could iface_offset wrap around near U32_MAX on 32-bit architectures, allowing it to bypass this check while still extending beyond shared_section_sz? Also, does this check incorrectly use the size of the host wrapper struct sizeof(*cs_iface) instead of the actual firmware struct size, allowing an invalid iface_offset to pass the check if the host struct is smaller? > cs_iface->control = ptdev->fw->shared_section->mem->kmap + iface_offset; > - 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); > + cs_iface->input = iface_fw_to_cpu_addr(ptdev, > cs_iface->control->input_va, > + sizeof(*cs_iface->input)); > + cs_iface->output = iface_fw_to_cpu_addr(ptdev, > cs_iface->control->output_va, > + sizeof(*cs_iface->output)); [ ... ] > @@ -941,8 +949,10 @@ static int panthor_init_csg_iface(struct panthor_device > *ptdev, > > spin_lock_init(&csg_iface->lock); > csg_iface->control = ptdev->fw->shared_section->mem->kmap + > iface_offset; > - csg_iface->input = iface_fw_to_cpu_addr(ptdev, > csg_iface->control->input_va); > - csg_iface->output = iface_fw_to_cpu_addr(ptdev, > csg_iface->control->output_va); > + csg_iface->input = iface_fw_to_cpu_addr(ptdev, > csg_iface->control->input_va, > + sizeof(*csg_iface->input)); > + csg_iface->output = iface_fw_to_cpu_addr(ptdev, > csg_iface->control->output_va, > + sizeof(*csg_iface->output)); > > if (csg_iface->control->stream_num < MIN_CS_PER_CSG || > csg_iface->control->stream_num > MAX_CS_PER_CSG) [Severity: Critical] This is a pre-existing issue, but is there a Time-of-Check to Time-of-Use (TOCTOU) vulnerability here and in panthor_fw_init_ifaces()? Since panthor_fw_start() starts the GPU MCU before panthor_fw_init_ifaces() is called, could compromised GPU firmware mutate stream_num (or group_num) directly in shared memory after this bounds check? If the compiler reloads the limit from memory on each iteration of the subsequent loop: for (i = 0; i < csg_iface->control->stream_num; i++) { int ret = panthor_init_cs_iface(ptdev, csg_idx, i); ... } could the loop overrun and index out of bounds, overwriting adjacent kernel structures? [ ... ] > @@ -999,8 +1009,10 @@ static int panthor_fw_init_ifaces(struct panthor_device > *ptdev) > return -EINVAL; > } > > - glb_iface->input = iface_fw_to_cpu_addr(ptdev, > glb_iface->control->input_va); > - glb_iface->output = iface_fw_to_cpu_addr(ptdev, > glb_iface->control->output_va); [Severity: High] This isn't a bug introduced by this patch, but earlier in this function could we dereference a NULL pointer if the firmware provides a 0-sized shared section? In panthor_fw_load_section_entry(), if the firmware provides a CSF_MCU_SHARED_REGION_START section header where hdr.va.end == hdr.va.start, the memory allocation is skipped: panthor_fw.c:panthor_fw_load_section_entry() { ... section_size = hdr.va.end - hdr.va.start; if (section_size) { ... } ptdev->fw->shared_section = section; ... } This leaves section->mem as NULL. When panthor_fw_init_ifaces() runs, it checks: if (!ptdev->fw->shared_section->mem->kmap) Will this unconditionally dereference mem when it is NULL, causing a kernel panic during device probe? > + glb_iface->input = iface_fw_to_cpu_addr(ptdev, > glb_iface->control->input_va, > + sizeof(*glb_iface->input)); > + glb_iface->output = iface_fw_to_cpu_addr(ptdev, > glb_iface->control->output_va, > + sizeof(*glb_iface->output)); > if (!glb_iface->input || !glb_iface->output) { > drm_err(&ptdev->base, "Invalid global control interface > input/output VA"); > return -EINVAL; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
