Re: [PATCH v3 4/4] drm/msm: Fix per-process-pgtables check

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Insufficient capability check and silent fallback to the global GPU VM allows userspace to manage the global VM via VM_BIND.
- [High] A time-of-check to time-of-use (TOCTOU) data race when opting into VM_BIND mode allows a state mismatch between `ctx->userspace_managed_vm` and the underlying IOMMU page table configuration.
--

commit 29eb04f2a503cc0bd049bafc825248eb1ad0a013
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Fix per-process-pgtables check

ctx->vm should not be inialized yet (or if it has, an error is returned
immediately following this check), so this isn't a valid way to check
for per-process-pgtable support. Instead just check if create_private_vm()
is supported.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Fixes: feb8ef4636a4 ("drm/msm: Add opt-in for VM_BIND")
Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>

> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index c62c45bb0ddbb..bff2a773c0d4e 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -505,7 +505,7 @@ int adreno_set_param(struct msm_gpu *gpu, struct msm_context *ctx,
>  		return msm_context_set_sysprof(ctx, gpu, value);
>  	case MSM_PARAM_EN_VM_BIND:
>  		/* We can only support VM_BIND with per-process pgtables: */
> -		if (ctx->vm == gpu->vm)
> +		if (!gpu->funcs->create_private_vm)

[Severity: Critical]
This is a pre-existing issue, but does relying on a static function
pointer here rather than verifying actual VM creation success allow an
invalid configuration to proceed?

If dynamic creation of the private VM fails in msm_gpu_create_private_vm()
(e.g., due to memory allocation failure or missing IOMMU support returning
-ENODEV), it silently falls back to returning the global, kernel-managed
gpu->vm:

drivers/gpu/drm/msm/msm_gpu.c:msm_gpu_create_private_vm() {
	...
	if (IS_ERR_OR_NULL(vm))
		vm = drm_gpuvm_get(gpu->vm);
	...
}

Because this check allows userspace to set ctx->userspace_managed_vm to
true, could a malicious process then use DRM_IOCTL_MSM_GEM_VM_BIND to issue
unmap and map operations against the global VM?

This seems to allow unmapping kernel structures like the memptrs ringbuffer
and mapping arbitrary memory in its place, potentially hijacking the GPU
ringbuffer and compromising the kernel. Furthermore, passing offset != 0 or
handle == 0 could trigger a BUG_ON() causing a kernel panic.

>  			return UERR(EINVAL, drm, "requires per-process pgtables");
>  
>  		/*
>  		 * We can only swtich to VM_BIND mode if the VM has not yet
>  		 * been created:
>  		 */
>  		if (ctx->vm)
>  			return UERR(EBUSY, drm, "VM already created");
>  
>  		ctx->userspace_managed_vm = value;

[Severity: High]
This isn't a bug introduced by this patch, but is there a race condition
here because the check of ctx->vm and assignment to ctx->userspace_managed_vm
are done without synchronization?

Could a concurrent ioctl (like MSM_GEM_SUBMIT) invoke msm_context_vm(),
which reads ctx->userspace_managed_vm before it is set to true here, causing
it to lazily create a legacy kernel-managed private VM?

drivers/gpu/drm/msm/msm_drv.c:msm_context_vm() {
	...
	vm = msm_gpu_create_private_vm(
		priv->gpu, current, !ctx->userspace_managed_vm);
	...
}

If adreno_set_param() then overwrites ctx->userspace_managed_vm to true,
and userspace subsequently issues DRM_IOCTL_MSM_GEM_VM_BIND, would the
ioctl proceed on the kernel-managed VM?

During the bind operation, wouldn't msm_gem_vma_new() see
vm->managed == true and execute BUG_ON() if offset != 0 or handle == 0,
triggering a kernel panic?

>  
>  		return 0;
>  	default:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625222744.22254-1-robin.clark@oss.qualcomm.com?part=4