Re: [PATCH v4 11/12] drm/panthor: Protm support for groups

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/panthor: missing validation of protected memory for PROTM suspend buffers
- [Low] drm: dead macro GROUP_CREATE_FLAGS referencing undefined identifier
--

commit a26512a2f35a2a18c407c42a57bf8210da106e39
Author: Boris Brezillon <boris.brezillon@collabora.com>

drm/panthor: Protm support for groups

This commit extends struct drm_panthor_group_create to allow a user-provided
protm suspend buffer. It imports the user-supplied suspend buffer and allows
the PROT_REGION instruction for groups that provide it. It also stores the
suspend buffer sizes provided by the FW.

> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 13c12a24e2f45..bfdabd7ee7beb 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c

[ ... ]

> @@ -4074,6 +4079,7 @@ static void add_group_kbo_sizes(struct panthor_device *ptdev,
>  }
>  
>  #define MAX_GROUPS_PER_POOL		128
> +#define GROUP_CREATE_FLAGS DRM_PANTHOR_GROUP_CREATE_PROTECTED

[Severity: Low]
Is this macro needed? The identifier DRM_PANTHOR_GROUP_CREATE_PROTECTED is not
defined anywhere in the codebase, and GROUP_CREATE_FLAGS is never used. 

While it has no runtime effect, it is a compilation hazard if the macro is ever
evaluated and constitutes dead code.

>  
>  int panthor_group_create(struct drm_file *file,
>  			 const struct drm_panthor_group_create *group_args,

[ ... ]

> @@ -4136,14 +4138,51 @@ int panthor_group_create(struct drm_file *file,
>  		goto err_put_group;
>  	}
>  
> -	suspend_size = csg_iface->control->suspend_size;
> -	group->suspend_buf = panthor_fw_alloc_suspend_buf_mem(ptdev, suspend_size);
> +	group->suspend_buf = panthor_fw_alloc_suspend_buf_mem(
> +		ptdev, panthor_fw_suspend_size(ptdev));
>  	if (IS_ERR(group->suspend_buf)) {
>  		ret = PTR_ERR(group->suspend_buf);
>  		group->suspend_buf = NULL;
>  		goto err_put_group;
>  	}
>  
> +	if (group_args->protected_suspend_bo_handle) {
> +		struct drm_gem_object *obj;
> +
> +		/* Read a correct snapshot of the state */
> +		if (!(smp_load_acquire(&ptdev->protm.info.state) &
> +		      DRM_PANTHOR_PROTM_INITIALIZED)) {
> +			ret = -EINVAL;
> +			goto err_put_group;
> +		}
> +
> +		obj = drm_gem_object_lookup(
> +			file, group_args->protected_suspend_bo_handle);
> +		if (!obj) {
> +			ret = -ENOENT;
> +			goto err_put_group;
> +		}
> +
> +		if (obj->size <
> +		    ptdev->protm.info.group_protected_suspend_buf_size) {
> +			drm_gem_object_put(obj);
> +			ret = -EINVAL;
> +			goto err_put_group;
> +		}
> +
> +		group->protm_suspend_buf = panthor_kernel_bo_create_from_gem(
> +			ptdev, panthor_fw_vm(ptdev), to_panthor_bo(obj),
> +			DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC,
> +			PANTHOR_VM_KERNEL_AUTO_VA,
> +			panthor_fw_protm_suspend_size(ptdev));

[Severity: High]
Does this code map the user-provided BO into the firmware VM without validating
that it actually corresponds to a protected DMA-BUF heap?

The uAPI comments state this BO must be allocated from a protected DMA-BUF
heap, but there doesn't appear to be any verification of its security properties
or whether it's actually backed by protected memory.

If the Mali firmware writes protected state into this suspend buffer during
context switches, could a malicious userspace application supply a standard
shmem-backed BO to read the protected state from normal RAM?

Even if the GPU hardware traps non-secure accesses to secure physical addresses,
should the kernel verify the user-provided memory handle is actually secure
before using it for firmware structures?

(Note that a similar missing validation seems to exist for the firmware
protected sections initialization in panthor_fw_protm_init(), though that path
is protected by CAP_SYS_MODULE).

> +		drm_gem_object_put(obj);
> +		if (IS_ERR(group->protm_suspend_buf)) {
> +			ret = PTR_ERR(group->protm_suspend_buf);
> +			group->protm_suspend_buf = NULL;
> +			goto err_put_group;
> +		}
> +	}
> +
>  	group->syncobjs = panthor_kernel_bo_create(ptdev, group->vm,
>  						   group_args->queues.count *
>  						   sizeof(struct panthor_syncobj_64b),

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