Re: [PATCH v4 10/12] drm/panthor: Support FW init of protm

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] drm: kernel crash on FW reload due to mapping protected PROT section
- [High] drm: data race on protm_section->mem causes ERR_PTR dereference
- [Medium] drm: mixing goto-based cleanup and guard() in panthor_fw_protm_init
- [Medium] drm/panthor: missing VA reservation for PROT section allows auto-VA clash
- [Low] drm: data race in panthor_ioctl_dev_query copying PROTM_INFO
--

commit 6a74417abb2feb49f589b33202b41b8e8a067ce4
Author: Boris Brezillon <boris.brezillon@collabora.com>
Subject: drm/panthor: Support FW init of protm

This commit adds support for initializing the Mali GPU firmware's protected
mode (protm) memory section. It introduces user queries for protm state and a
new ioctl for user-initiated protected mode initialization.

> diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
> index 51a2e5358cd51..cc1c050ea3806 100644
> --- a/drivers/gpu/drm/panthor/panthor_drv.c
> +++ b/drivers/gpu/drm/panthor/panthor_drv.c
[ ... ]
> @@ -991,6 +997,9 @@ static int panthor_ioctl_dev_query(struct drm_device *ddev, void *data, struct d
>  	case DRM_PANTHOR_DEV_QUERY_MMU_INFO:
>  		return PANTHOR_UOBJ_SET(args->pointer, args->size, ptdev->mmu_info);
>  
> +	case DRM_PANTHOR_DEV_QUERY_PROTM_INFO:
> +		return PANTHOR_UOBJ_SET(args->pointer, args->size, ptdev->protm.info);

[Severity: Low]
Is it safe to directly pass ptdev->protm.info to PANTHOR_UOBJ_SET() here?

Since panthor_fw_protm_init() updates the state field concurrently using
smp_store_release(), this byte/word copy in copy_to_user() might race
and copy torn state to userspace.

Should this create a local copy using smp_load_acquire() before passing it
to copy_to_user(), similar to how timestamp_info is handled?

> +
>  	default:
>  		return -EINVAL;
>  	}
> diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
> index fc07796fc9ef7..0af6addfd55a5 100644
> --- a/drivers/gpu/drm/panthor/panthor_fw.c
> +++ b/drivers/gpu/drm/panthor/panthor_fw.c
[ ... ]
> @@ -462,6 +470,9 @@ static void panthor_fw_init_section_mem(struct panthor_device *ptdev,
>  	    !(section->flags & CSF_FW_BINARY_IFACE_ENTRY_ZERO))
>  		return;
>  
> +	if (!section->mem)
> +		return;

[Severity: High]
Does this check properly handle the error pointer state?

If panthor_fw_protm_init() is currently executing concurrently,
section->mem might briefly contain an ERR_PTR if
panthor_kernel_bo_create_from_gem() fails.

Since the reset worker reads section->mem without holding the
protm_init_lock, could it observe the intermediate ERR_PTR, bypass this
check, and crash when panthor_kernel_bo_vmap() dereferences it?

> +
>  	ret = panthor_kernel_bo_vmap(section->mem);
>  	if (drm_WARN_ON(&ptdev->base, ret))
>  		return;
>  
>  	memcpy(section->mem->kmap, section->data.buf, section->data.size);

[Severity: Critical]
Will this cause a secure fault (SError) or kernel panic during a GPU reset?

If PROTM_INIT was successfully called by userspace, section->mem now
holds protected memory. During a slow reset, panthor_reload_fw_sections()
iterates over all firmware sections and unconditionally calls
panthor_fw_init_section_mem().

If it processes the PROT section, it will attempt to vmap and
memset/memcpy the protected memory from the CPU non-secure world. Should
this function or the reload loop explicitly skip the
CSF_FW_BINARY_IFACE_ENTRY_PROT section to avoid a secure fault?

[ ... ]
> @@ -678,7 +683,22 @@ static int panthor_fw_load_section_entry(struct panthor_device *ptdev,
>  	section->size = section_size;
>  	section->va = hdr.va.start;
>  
> -	if (section_size) {
> +	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;
> +		}
> +
> +		ptdev->protm.info.fw_protected_sections_size =
> +			ALIGN(section->size, vm_pgsz);
> +		ptdev->fw->protm_section = section;
> +	}
> +
> +	/* 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)) {

[Severity: Medium]
Does skipping the memory allocation here also unintentionally skip reserving
the virtual address in the firmware VM?

If the firmware binary places the protected mode (PROT) section within the
kernel_auto_va range, and we skip panthor_kernel_bo_create() (which
allocates the VA), could an unprivileged user allocating queues or suspend
buffers steal this unreserved VA?

This would later cause panthor_fw_protm_init() to fail with -ENOSPC when
it attempts to reserve the exact VA. Should the VA still be reserved early,
even if memory allocation is deferred?

>  		u32 vm_map_flags = section_vm_map_flags(section);
>  
>  		section->mem = panthor_kernel_bo_create(
[ ... ]
> @@ -1585,6 +1610,70 @@ int panthor_fw_protm_exit(struct panthor_device *ptdev, u32 timeout_ms)
>  	return panthor_fw_protm_exit_wait(ptdev, timeout_ms);
>  }
>  
> +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: Medium]
This isn't a bug, but this function mixes scope-based cleanup
(guard(mutex)) with traditional goto cleanup labels (like
goto out_gem_put).

According to the cleanup helpers documentation, goto and scope-based
cleanup should not be mixed in the same function. Could all resources in
this function be converted to use scope-based cleanup to avoid ownership
confusion?

> +
> +	/* Thread safe access so other readers do not need protm_init_lock */
> +	if (smp_load_acquire(&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_create_from_gem(
> +		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;

[Severity: High]
Is it safe to assign directly to the globally visible protm_section->mem
before checking for errors?

If this allocation fails, protm_section->mem briefly holds an ERR_PTR
until the error check clears it. A concurrent GPU reset worker, which reads
section->mem without holding protm_init_lock, could observe this
intermediate state and crash when trying to vmap it.

Should the result be stored in a local variable and only assigned to
protm_section->mem upon success?

> +		goto out_gem_put;
> +	}
> +
> +	/* Make sure all above is completed before we update state */
> +	smp_store_release(&ptdev->protm.info.state,
> +			  DRM_PANTHOR_PROTM_INITIALIZED);
> +
> +out_gem_put:
> +	drm_gem_object_put(obj);
> +
> +out_dev_exit:
> +	drm_dev_exit(cookie);
> +	return ret;
> +}

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