Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] The newly added `pad` field in `struct drm_panfrost_perfcnt_dump` is not validated for being zero, breaking future extendability. - [Medium] The `state` output field in `struct drm_panfrost_perfcnt_dump` is not initialized to zero before being returned to userspace on the success path. --
commit 206ab1a1901fc21e7df0dcb1254dbaded4f7ae19 Author: Adrián Larumbe <[email protected]> drm/panfrost: Bump driver minor to reflect new DUMP IOCTL req field Expose expanded PERFCNT_DUMP IOCTL interface to user mode. > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c > b/drivers/gpu/drm/panfrost/panfrost_drv.c > index 571a26b841269..de9b1c115181b 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c > @@ -808,6 +808,7 @@ static const struct file_operations > panfrost_drm_driver_fops = { > * - 1.6 - adds PANFROST_BO_MAP_WB, PANFROST_IOCTL_SYNC_BO, > * PANFROST_IOCTL_QUERY_BO_INFO and > * DRM_PANFROST_PARAM_SELECTED_COHERENCY > + * - 1.7 - adds PERFCNT_DUMP req state field [Severity: Medium] Since this exposes the new PERFCNT_DUMP fields to userspace, does the IOCTL handler need to validate that the new padding field is zero? In the newly expanded struct drm_panfrost_perfcnt_dump, a pad field was added: include/uapi/drm/panfrost_drm.h: struct drm_panfrost_perfcnt_dump { __u64 buf_ptr; __u32 state; __u32 pad; }; But in panfrost_ioctl_perfcnt_dump(), it doesn't appear this field is checked: drivers/gpu/drm/panfrost/panfrost_perfcnt.c:panfrost_ioctl_perfcnt_dump() { ... ret = panfrost_unstable_ioctl_check(); if (ret) return ret; mutex_lock(&perfcnt->lock); ... } If userspace leaves this uninitialized, the kernel currently accepts non-zero values, which might prevent safely repurposing the pad field for new features in the future. [Severity: Medium] Also regarding the new state field, is req->state guaranteed to be initialized to zero before being returned to userspace? Looking at panfrost_ioctl_perfcnt_dump(), the state field is passed directly: drivers/gpu/drm/panfrost/panfrost_perfcnt.c:panfrost_ioctl_perfcnt_dump() { ... ret = panfrost_perfcnt_dump_locked(pfdev, &req->state); if (ret) goto out; ... } And in panfrost_perfcnt_dump_locked(), *state is only conditionally modified or OR'd: drivers/gpu/drm/panfrost/panfrost_perfcnt.c:panfrost_perfcnt_dump_locked() { ... if (perfcnt->reset_happened) { *state = PANFROST_PERFCNT_SESSION_INTERRUPTED_BY_RESET; perfcnt->reset_happened = false; } ... /* Either sample finished or reset happened */ if (ret > 0) { ... if (perfcnt->reset_happened) *state |= PANFROST_PERFCNT_SESSION_INTERRUPTED_BY_RESET; if (!perfcnt->owns_as_ref) *state |= PANFROST_PERFCNT_SESSION_DEAD; } ... } If a userspace program calls the IOCTL with uninitialized memory for req->state, and the happy path is taken where neither condition hits, does this leave the state field uninitialized on success, returning the user's garbage data back to them and potentially causing them to falsely detect a GPU reset or dead session? [ ... ] -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=16
