Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] drm: panthor: leak of SRCU read lock on error paths in perf session ioctls - [High] drm: panthor: integer overflow in sample size calculation allows out-of-bounds write - [High] drm: missing drm_dev_exit() on error paths causes SRCU lock leak - [Low] drm/panthor: missing bounds validation for perf block_set --
commit 67ed39f94766c6f1ca32b076cb6cf69b5aaa9f5d Author: Lukas Zapolskas <[email protected]> drm/panthor: Expose the panthor perf ioctls This patch implements the PANTHOR_PERF_SESSION ioctl series. > diff --git a/drivers/gpu/drm/panthor/panthor_drv.c > b/drivers/gpu/drm/panthor/panthor_drv.c > --- a/drivers/gpu/drm/panthor/panthor_drv.c > +++ b/drivers/gpu/drm/panthor/panthor_drv.c [ ... ] > +static int panthor_ioctl_perf_session_create(struct drm_device *ddev, void > *data, > + struct drm_file *file) > +{ > + int ret, cookie; > + unsigned long sid = 0; > + struct drm_panthor_perf_session_create *args = data; > + > + if (!drm_dev_enter(file->minor->dev, &cookie)) > + return -ENODEV; > + > + if (!mem_is_zero(args->pad, sizeof(args->pad))) > + return -EINVAL; [Severity: High] Does this error path leak the SRCU read lock acquired by drm_dev_enter()? Returning directly without calling drm_dev_exit() leaves the lock held, which can cause a system deadlock when drm_dev_unplug() waits indefinitely in synchronize_srcu(). > + > + ret = panthor_perf_session_create(file, args, &sid); [Severity: High] When calling panthor_perf_session_create(), does the calculation of user_sample_size overflow on 32-bit systems? Inside panthor_perf_session_create() in panthor_perf.c: user_sample_size = ptdev->perf_info.sample_size * slots; Since size_t is 32-bit on 32-bit architectures, and slots is derived from the userspace-provided sample_slots, a large power-of-two value can cause this multiplication to wrap to exactly 0. If user_sample_size wraps to 0, panthor_perf_bo_get() will skip its bounds validation. This would allow userspace to map a small buffer object while retaining a massive ringbuf_slots value, which could result in out-of-bounds writes into adjacent kernel virtual memory during sample collection. Should this use size_mul() to saturate on overflow? [Severity: Low] Is the block_set parameter from userspace bounds-checked before it is passed to panthor_perf_session_add() here? Inside panthor_perf_session_add() in panthor_perf.c, the unvalidated set is stored directly: perf->set_config = set; Later in panthor_perf_fw_write_config(), it gets truncated by the hardware masking macro GLB_PERFCNT_CONFIG_SET: perfcnt_config |= GLB_PERFCNT_CONFIG_SET(perf->set_config); Should this be validated against the maximum number of blocks to return an -EINVAL error for non-existent sets, as documented in the uAPI? > + if (ret) > + return ret; [Severity: High] Does this error path also leak the SRCU read lock by returning directly without calling drm_dev_exit(cookie)? > + > + drm_dev_exit(cookie); > + return sid; > +} > + > +static int panthor_ioctl_perf_session_start(struct drm_device *ddev, void > *data, > + struct drm_file *file) > +{ > + int cookie, ret; > + struct panthor_file *pfile = file->driver_priv; > + struct drm_panthor_perf_session_start *args = data; > + > + if (!drm_dev_enter(file->minor->dev, &cookie)) > + return -ENODEV; > + > + if (args->pad0) > + return -EINVAL; [Severity: High] Can this early return leak the SRCU lock? We need drm_dev_exit(cookie) here before returning. > + > + ret = panthor_perf_session_start(pfile, args->handle, args->user_data); > + > + drm_dev_exit(cookie); > + return ret; > +} > + > +static int panthor_ioctl_perf_session_sample(struct drm_device *ddev, void > *data, > + struct drm_file *file) > +{ > + int cookie, ret; > + struct panthor_file *pfile = file->driver_priv; > + struct drm_panthor_perf_session_sample *args = data; > + > + if (!drm_dev_enter(file->minor->dev, &cookie)) > + return -ENODEV; > + > + if (args->pad0) > + return -EINVAL; [Severity: High] Can this early return leak the SRCU lock? We need drm_dev_exit(cookie) here before returning. > + > + ret = panthor_perf_session_sample(pfile, args->handle, > args->user_data); > + > + drm_dev_exit(cookie); > + return ret; > +} > + > +static int panthor_ioctl_perf_session_stop(struct drm_device *ddev, void > *data, > + struct drm_file *file) > +{ > + int cookie, ret; > + struct panthor_file *pfile = file->driver_priv; > + struct drm_panthor_perf_session_stop *args = data; > + > + if (!drm_dev_enter(file->minor->dev, &cookie)) > + return -ENODEV; > + > + if (args->pad0) > + return -EINVAL; [Severity: High] Can this early return leak the SRCU lock? We need drm_dev_exit(cookie) here before returning. > + > + ret = panthor_perf_session_stop(pfile, args->handle, args->user_data); > + > + drm_dev_exit(cookie); > + return ret; > +} > + > +static int panthor_ioctl_perf_session_destroy(struct drm_device *ddev, void > *data, > + struct drm_file *file) > +{ > + int cookie, ret; > + struct panthor_file *pfile = file->driver_priv; > + struct drm_panthor_perf_session_destroy *args = data; > + > + if (!drm_dev_enter(file->minor->dev, &cookie)) > + return -ENODEV; > + > + if (args->pad0) > + return -EINVAL; [Severity: High] Can this early return leak the SRCU lock? We need drm_dev_exit(cookie) here before returning. > + > + ret = panthor_perf_session_destroy(pfile, args->handle); > + > + drm_dev_exit(cookie); > + return ret; > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=8
