Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] The patch prevents TOCTOU vulnerabilities during firmware interface initialization by using READ_ONCE() for `group_num` and `stream_num`, but fails to apply this protection to `panthor_sched_init()`, which re-reads these exact same fields directly from the volatile firmware-shared memory. This can lead to an initialization mismatch and a subsequent NULL pointer dereference. - [High] Userspace queue creation validates `group_args->queues.count` against the hardcoded `MAX_CS_PER_CSG`, but fails to validate it against the firmware-reported and initialized `cs_slot_count`, allowing a NULL pointer dereference. - [Medium] The patch adds `READ_ONCE()` for `group_num` and `stream_num` but leaves `group_stride` and `stream_stride` unprotected, allowing potential out-of-bounds memory accesses if the compiler rematerializes the reads. -- --- Patch [1]: [PATCH v3] drm/panthor: use local variables for firmware interface counts --- commit 49abfdca3f9a1c66d7525434ea16162686b42354 Author: Osama Abdelkader <[email protected]> drm/panthor: use local variables for firmware interface counts The firmware exposes the global group count and per-group stream count in the shared control interface. These values are validated before being used as loop bounds. > diff --git a/drivers/gpu/drm/panthor/panthor_fw.c > b/drivers/gpu/drm/panthor/panthor_fw.c > index 10b7e12c1f1ed..fbc4d10aed1fb 100644 > --- a/drivers/gpu/drm/panthor/panthor_fw.c > +++ b/drivers/gpu/drm/panthor/panthor_fw.c > @@ -955,6 +955,7 @@ static int panthor_init_csg_iface(struct panthor_device > *ptdev, > u64 shared_section_sz = > panthor_kernel_bo_size(ptdev->fw->shared_section->mem); > u64 iface_offset = CSF_GROUP_CONTROL_OFFSET + > ((u64)csg_idx * glb_iface->control->group_stride); [Severity: Medium] This isn't a bug introduced by this patch, but since we are preventing TOCTOU vulnerabilities with READ_ONCE(), should group_stride (and stream_stride in panthor_init_cs_iface) also be protected? If the compiler rematerializes the reads for the strides after the bounds checks, could a malicious firmware modify the stride concurrently and cause out-of-bounds pointer arithmetic when setting up the interface pointers? > + u32 stream_num; > unsigned int i; > > if (iface_offset > shared_section_sz || [ ... ] > @@ -1036,13 +1043,18 @@ static int panthor_fw_init_ifaces(struct > panthor_device *ptdev) > return -EINVAL; > } > > - if (glb_iface->control->group_num > MAX_CSGS || > - glb_iface->control->group_num < MIN_CSGS) { > + /* > + * To protect against self-modifying control sections > + * take a single snapshot from the control section so validation and > + * iteration use the same value. > + */ > + group_num = READ_ONCE(glb_iface->control->group_num); > + if (group_num > MAX_CSGS || group_num < MIN_CSGS) { > drm_err(&ptdev->base, "Invalid number of control groups"); > return -EINVAL; > } [Severity: High] This is a pre-existing issue, but does the scheduler also need to use these protected values? In drivers/gpu/drm/panthor/panthor_sched.c:panthor_sched_init(), the code re-reads these fields directly from the volatile firmware memory: sched->csg_slot_count = num_groups; sched->cs_slot_count = csg_iface->control->stream_num; If the firmware increases stream_num after the bounds checks here in panthor_fw_init_ifaces(), could the scheduler configure cs_slot_count to a larger value than the initialized structures in ptdev->fw->iface? Later, when iterating up to these counts in tick_ctx_apply() or csg_slot_prog_locked(), could the driver retrieve a zero-initialized interface structure and dereference NULL pointers? [Severity: High] This is also a pre-existing issue, but is the userspace queue count properly bounded by the initialized firmware slot count? In drivers/gpu/drm/panthor/panthor_drv.c:panthor_ioctl_group_create(), the queue count is validated against the hardcoded maximum: if (!args->queues.count || args->queues.count > MAX_CS_PER_CSG) return -EINVAL; However, if the firmware reports fewer slots than MAX_CS_PER_CSG, panthor_fw_init_ifaces() will only initialize that smaller number of stream interfaces. If unprivileged userspace requests more queues than the firmware reported (but less than MAX_CS_PER_CSG), could cs_slot_prog_locked() fetch an uninitialized stream interface and crash when dereferencing cs_iface->input->ringbuf_base? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
