Re: [PATCH v3] drm/panthor: use local variables for firmware interface counts

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