Re: [PATCH v3 6/6] drm/panthor: Expose protected rendering features

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] An unprivileged userspace process can trigger a full GPU reset by omitting the protected suspend buffer during group creation and then submitting commands that switch to protected mode.
- [High] Slow reset path triggers a NULL pointer dereference on the uninitialized protected firmware section in `panthor_fw_init_section_mem`.
- [Low] Data race on the `ptdev->protm.info.state` flag.
- [Low] Mixing `guard()` and `goto`-based cleanup in `panthor_fw_protm_init` violates subsystem cleanup guidelines.
--

commit b7ec40f4bb5c3ae17349c8d0c7a683e90abc16c4
Author: Boris Brezillon <boris.brezillon@collabora.com>
Subject: drm/panthor: Expose protected rendering features

This commit introduces extensions to the Panthor uAPI to support protected
rendering. It adds a new IOCTL for providing protected firmware memory and
extends group creation to allow userspace to provide a protected suspend
buffer.

> diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
> index 96770ce34da84..4b44baf82a037 100644
> --- a/drivers/gpu/drm/panthor/panthor_fw.c
> +++ b/drivers/gpu/drm/panthor/panthor_fw.c

[ ... ]

> @@ -644,35 +677,36 @@ static int panthor_fw_load_section_entry(struct panthor_device *ptdev,
>  		section->name = name;
>  	}
>  
> -	if (section_size) {
> -		u32 cache_mode = hdr.flags & CSF_FW_BINARY_IFACE_ENTRY_CACHE_MODE_MASK;
> -		u32 vm_map_flags = 0;
> -		u64 va = hdr.va.start;
> +	section->size = section_size;
> +	section->va = hdr.va.start;
>  
> -		if (!(hdr.flags & CSF_FW_BINARY_IFACE_ENTRY_WR))
> -			vm_map_flags |= DRM_PANTHOR_VM_BIND_OP_MAP_READONLY;
> +	if (hdr.flags & CSF_FW_BINARY_IFACE_ENTRY_PROT) {
> +		if (ptdev->fw->protm_section) {
> +			drm_err(&ptdev->base,
> +				"Only one protected section supported\n");
> +			return -EINVAL;
> +		}
>  
> -		if (!(hdr.flags & CSF_FW_BINARY_IFACE_ENTRY_EX))
> -			vm_map_flags |= DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC;
> +		ptdev->protm.info.fw_protected_sections_size =
> +			ALIGN(section->size, vm_pgsz);
> +		ptdev->fw->protm_section = section;
> +	}
>  
> -		/* TODO: CSF_FW_BINARY_IFACE_ENTRY_CACHE_MODE_*_COHERENT are mapped to
> -		 * non-cacheable for now. We might want to introduce a new
> -		 * IOMMU_xxx flag (or abuse IOMMU_MMIO, which maps to device
> -		 * memory and is currently not used by our driver) for
> -		 * AS_MEMATTR_AARCH64_SHARED memory, so we can take benefit
> -		 * of IO-coherent systems.
> -		 */
> -		if (cache_mode != CSF_FW_BINARY_IFACE_ENTRY_CACHE_MODE_CACHED)
> -			vm_map_flags |= DRM_PANTHOR_VM_BIND_OP_MAP_UNCACHED;
> +	/* Defer the section->mem creation if this is a protected entry.
> +	 * This will be populated when DRM_IOCTL_PANTHOR_PROTM_INIT is called.
> +	 */
> +	if (section->size && !(hdr.flags & CSF_FW_BINARY_IFACE_ENTRY_PROT)) {
> +		u32 vm_map_flags = section_vm_map_flags(section);

[Severity: High]
Can this cause a kernel panic if a GPU hang occurs before the protected mode
is initialized?

Because section->mem is intentionally left as NULL here for protected
sections, if an unprivileged user triggers a GPU hang before calling the
PROTM_INIT ioctl, the slow reset path will be invoked. When
panthor_reload_fw_sections() iterates over all sections, it calls
panthor_fw_init_section_mem() which dereferences section->mem without checking
if it is valid (e.g., bool was_mapped = !!section->mem->kmap). 

Would it make sense to skip uninitialized protected sections during reset?

[ ... ]

> @@ -1566,6 +1605,66 @@ int panthor_fw_protm_exit(struct panthor_device *ptdev, u32 timeout_ms)
>  	return ret;
>  }
>  
> +int panthor_fw_protm_init(struct drm_file *file,
> +			  struct drm_panthor_protm_init *args)
> +{
> +	struct panthor_file *pfile = file->driver_priv;
> +	struct panthor_device *ptdev = pfile->ptdev;
> +	struct panthor_fw_section *protm_section = ptdev->fw->protm_section;
> +	struct drm_gem_object *obj;
> +	u32 vm_map_flags;
> +	int cookie, ret = 0;
> +
> +	if (!capable(CAP_SYS_MODULE))
> +		return -EPERM;
> +
> +	if (args->pad)
> +		return -EINVAL;
> +
> +	if (!protm_section || !protm_section->size)
> +		return -EINVAL;
> +
> +	guard(mutex)(&ptdev->fw->protm_init_lock);

[Severity: Low]
Is it safe to mix guard() and goto-based cleanup in the same function?

The cleanup subsystem guidelines suggest avoiding the mixture of scope-based
locks like guard(mutex) with goto labels (such as out_gem_put and
out_dev_exit) in the same routine, as it can lead to confusing ownership
semantics and future maintenance bugs.

> +
> +	if (ptdev->protm.info.state & DRM_PANTHOR_PROTM_INITIALIZED)
> +		return 0;
> +
> +	if (!drm_dev_enter(&ptdev->base, &cookie))
> +		return -ENODEV;
> +
> +	obj = drm_gem_object_lookup(file,
> +				    args->fw_protected_sections_bo_handle);
> +	if (!obj) {
> +		ret = -ENOENT;
> +		goto out_dev_exit;
> +	}
> +
> +	if (obj->size < ptdev->protm.info.fw_protected_sections_size) {
> +		ret = -EINVAL;
> +		goto out_gem_put;
> +	}
> +
> +	vm_map_flags = section_vm_map_flags(protm_section);
> +
> +	protm_section->mem = panthor_kernel_bo_import(
> +		ptdev, panthor_fw_vm(ptdev), to_panthor_bo(obj), vm_map_flags,
> +		protm_section->va, protm_section->size);
> +	if (IS_ERR(protm_section->mem)) {
> +		ret = PTR_ERR(protm_section->mem);
> +		protm_section->mem = NULL;
> +		goto out_gem_put;
> +	}
> +
> +	ptdev->protm.info.state |= DRM_PANTHOR_PROTM_INITIALIZED;

[Severity: Low]
Could this non-atomic modification cause torn reads or compiler reordering
issues?

This updates ptdev->protm.info.state using a bitwise OR, but the same state
flag is read concurrently in cs_slot_process_protm_pending_event_locked()
without locks or READ_ONCE().

> +
> +out_gem_put:
> +	drm_gem_object_put(obj);
> +
> +out_dev_exit:
> +	drm_dev_exit(cookie);
> +	return ret;
> +}

[ ... ]

> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 1fe77e5c41995..d73fc2f6633a6 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c

[ ... ]

> @@ -1429,10 +1429,15 @@ cs_slot_process_protm_pending_event_locked(struct panthor_device *ptdev,
>  	if (!group)
>  		return;
>  
> -	/* Do not allow user space work to switch into protected mode, as we
> -	 * do not fully support this quite yet.
> +	/* Do not allow user space work to switch into protected mode if we
> +	 * do not support protected mode on this device.
> +	 * User space should query (and init) this support before attempting
> +	 * to use such GPU instructions.
>  	 */
> -	atomic_or(BIT(cs_id), &group->fatal_queues);
> +	if (!(ptdev->protm.info.state & DRM_PANTHOR_PROTM_INITIALIZED))
> +		atomic_or(BIT(cs_id), &group->fatal_queues);
> +	else
> +		atomic_or(BIT(cs_id), &group->protm_pending_queues);
>  
>  	sched_queue_delayed_work(sched, tick, 0);
>  }

[Severity: High]
Does this allow an unprivileged userspace process to trigger a full
system-wide GPU reset?

The logic here approves a transition to protected mode by only checking the
global DRM_PANTHOR_PROTM_INITIALIZED flag. However, if a user creates a
group with protected_suspend_bo_handle set to 0 (resulting in
group->protm_suspend_buf being NULL), they can still submit a PROT_REGION
command. 

Because the scheduler does not verify if the specific group provided a valid
suspend buffer, the hardware interface gets programmed with a zero address
for the suspend buffer in panthor_group_bind_locked(). When the firmware
later attempts to suspend the group, it writes to address 0, causing an
unrecoverable exception and Denial of Service.

Should this also check if group->protm_suspend_buf is valid?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911114014.79139-1-ketil.johnsen@arm.com?part=6