Re: [PATCH v7 8/8] drm/panthor: Expose the panthor perf ioctls

[sashiko-bot@kernel.org]

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 <lukas.zapolskas@arm.com>

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/20261011-panthor-performance-counters-v7-0-12c32495062f@arm.com?part=8