Re: [PATCH v7 5/8] drm/panthor: Introduce sampling sessions to handle userspace clients

[sashiko-bot@kernel.org]

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

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