Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] drm/panthor: panthor_perf_bo_get() drops iosys_map is_iomem flag - [High] drm/panthor: integer overflow in panthor_perf_session_create() ringbuffer size check - [Medium] drm/panthor: dead code in session state checks causes incorrect error returns --
commit 7bd124a87dfe342eb277b597b5fdc104cfbe8dd7 Author: Lukas Zapolskas <[email protected]> drm/panthor: Introduce sampling sessions to handle userspace clients This commit introduces a sampling session intermediary layer between the HW/FW interfaces and userspace for the Panthor DRM driver. It enables manual sampling sessions with operations to start, stop, and sample hardware performance counters, while multiple sessions and periodic sampling are left for future extensions. > diff --git a/drivers/gpu/drm/panthor/panthor_perf.c > b/drivers/gpu/drm/panthor/panthor_perf.c > index ba466e22c50d0..76c3469811508 100644 > --- a/drivers/gpu/drm/panthor/panthor_perf.c > +++ b/drivers/gpu/drm/panthor/panthor_perf.c [ ... ] > +static int > +panthor_perf_bo_get(struct drm_file *file, u32 handle, size_t expected_size, > + struct panthor_perf_bo *bo) > +{ > + struct drm_device *ddev = file->minor->dev; > + struct drm_gem_object *obj; > + struct iosys_map map; > + int ret; > + > + if (WARN_ON_ONCE(!bo)) > + return -EINVAL; > + > + obj = drm_gem_object_lookup(file, handle); > + if (!obj) { > + drm_err(ddev, "Could not find handle %d!\n", handle); > + return -EINVAL; > + } > + > + if (expected_size && obj->size != PFN_ALIGN(expected_size)) { [Severity: High] Does this expected_size check handle integer overflow correctly? In panthor_perf_session_create(), expected_size is calculated as: user_sample_size = ptdev->perf_info.sample_size * slots; Since slots is controlled by userspace and size_t is 32-bit on 32-bit architectures, a maliciously large power-of-two value for sample_slots can overflow the multiplication to exactly 0. Because of the "if (expected_size && ...)" check, an expected_size of 0 bypasses the size validation completely, allowing a small buffer to be mapped. When sampling occurs later, the massive slots count could lead to out-of-bounds writes into kernel memory. > + drm_err(ddev, > + "Incorrect BO size from userspace: user %zu vs kernel > %lu\n", > + obj->size, PFN_ALIGN(expected_size)); > + ret = -ENOMEM; > + goto err_put; > + } > + > + ret = drm_gem_vmap(obj, &map); > + if (ret) > + goto err_put; > + > + bo->obj = obj; > + bo->vaddr = map.vaddr; [Severity: High] Is it safe to discard the map.is_iomem flag here? These BOs might be imported DMA-BUFs backed by device IOMEM. By dropping the is_iomem flag and treating the mapping as a standard CPU pointer, the driver may trigger architecture faults (such as alignment faults or unsupported access types) when interacting with IOMEM memory later on, such as via SMP memory barriers. > + > + return 0; > + > +err_put: > + drm_gem_object_put(obj); > + return ret; > +} > + > +static void panthor_perf_bo_put(struct panthor_perf_bo *bo) > +{ > + if (!bo->obj) > + return; > + > + if (bo->vaddr) { > + struct iosys_map map = IOSYS_MAP_INIT_VADDR(bo->vaddr); > + > + drm_gem_vunmap(bo->obj, &map); [Severity: High] Is it correct to hardcode is_iomem to false during vunmap? Since the original mapping might have been IOMEM, reconstructing a fake iosys_map with IOSYS_MAP_INIT_VADDR() sets is_iomem = false. Passing this to drm_gem_vunmap() violates the API contract for imported objects and could corrupt the exporter's teardown flow because the unmap parameters do not match the original map parameters. > + } > + > + drm_gem_object_put(bo->obj); > + > + memset(bo, 0, sizeof(*bo)); > +} [ ... ] > +int panthor_perf_session_create(struct drm_file *file, > + struct drm_panthor_perf_session_create > *create_args, > + unsigned long *sid) > +{ [ ... ] > + session->control = control.vaddr; > + *session->control = (struct drm_panthor_perf_ringbuf_control) { 0 }; [Severity: High] Can this direct assignment fault if the memory is IOMEM? Since the vaddr came from panthor_perf_bo_get() which discarded the is_iomem flag, this performs a direct CPU assignment on memory that could be IOMEM. [ ... ] > +static int session_try_stop(struct panthor_perf_session *session, u64 > user_data) > +{ > + guard(mutex)(&session->state_lock); > + > + if (!session_is_active(session)) > + return 0; > + > + if (session_is_starting(session)) > + return -EBUSY; > + > + if (session_is_stopping(session)) > + return 1; [Severity: Medium] Are the session_is_starting() and session_is_stopping() checks unreachable here? If a session is in PANTHOR_PERF_SESSION_STARTING or PANTHOR_PERF_SESSION_STOPPING state, session_is_active() evaluates to false. The !session_is_active() check will immediately return 0, bypassing the later checks. This causes the kernel to incorrectly return success (0) instead of -EBUSY for a starting session, and 0 instead of 1 for a stopping session. This may prevent session_stop from properly waiting for a stopping session to finish syncing. > + > + session_set_state(session, PANTHOR_PERF_SESSION_STOPPING); > + session->user_data = user_data; > + > + return 1; > +} [ ... ] > +static int session_try_sample(struct panthor_perf_session *session, u64 > user_data) > +{ > + guard(mutex)(&session->state_lock); > + > + if (!session_is_active(session)) > + return -EINVAL; > + > + if (session_is_stopping(session)) > + return -EBUSY; [Severity: Medium] Is the session_is_stopping() check unreachable dead code? Similar to session_try_stop(), if the session is stopping, session_is_active() is false, causing the function to return -EINVAL immediately instead of the expected -EBUSY. > + > + if (session_has_overflow(session)) > + return -EACCES; > + > + session->user_data = user_data; > + > + return 1; > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
