Re: [PATCH v5 09/10] 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:

New issues:
- [Critical] Logic inversion in fallback condition allows VM_BIND contexts to hijack the global shared GPU VM, while breaking legacy contexts on older hardware.

Pre-existing issues:
- [High] Missing lock protection when reading and updating `ctx->sysprof`, leading to refcount and pm_runtime leaks.
--

--- Patch [9]: [PATCH v5 09/10] drm/msm: Fix per-process-pgtables check ---
commit 439295368270634dcd942a5c715dc37574234eb9
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.

> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index 84101e534ebef..3370cd44382fe 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -513,7 +513,7 @@ int adreno_set_param(struct msm_gpu *gpu, struct msm_context *ctx,

[Severity: High]
This is a pre-existing issue, but does the MSM_PARAM_SYSPROF ioctl handler
in this function need to hold any context-specific locks before calling
msm_context_set_sysprof()?

drivers/gpu/drm/msm/msm_submitqueue.c:msm_context_set_sysprof() {
...
	/* unwind old value: */
	switch (ctx->sysprof) {
	case 2:
		pm_runtime_put_autosuspend(&gpu->pdev->dev);
		fallthrough;
	case 1:
		refcount_dec(&gpu->sysprof_active);
		fallthrough;
	case 0:
		break;
	}

	/* Some gpu families require additional setup for sysprof */
	if (gpu->funcs->sysprof_setup)
		gpu->funcs->sysprof_setup(gpu, false);

	ctx->sysprof = sysprof;
...
}

If two threads execute the MSM_PARAM_SYSPROF ioctl concurrently on the same
context, they both enter msm_context_set_sysprof() without any locks.
Both could increment the global refcounts based on the new value, but read
the same old ctx->sysprof value (e.g., 0) during the unwind phase.

Neither thread decrements the old refcounts, but both update ctx->sysprof.
When the context is closed, the refcounts are only decremented once,
leaving a permanent leak that prevents the GPU from powering down.

>  		guard(rwsem_read)(&ctx->ctxlock);
>  
>  		/* We can only support VM_BIND with per-process pgtables: */
> -		if (ctx->vm == gpu->vm)
> +		if (!gpu->funcs->create_private_vm)
>  			return UERR(EINVAL, drm, "requires per-process pgtables");

[Severity: Critical]
While looking at the VM_BIND changes, does the fallback logic in
msm_gpu_create_private_vm() accidentally invert its condition?

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

If private VM creation fails or is unsupported for a VM_BIND context
(where kernel_managed is false), this condition evaluates to true,
and the context falls back to the global shared GPU VM.

Could this allow userspace to hijack the global shared VM and modify
its pagetables?

Additionally, this logic inversion prevents legacy contexts (where
kernel_managed is true) from falling back to the global VM on older hardware,
causing initialization failures.

>  		/*

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