Re: [PATCH v5 03/10] drm/msm: Synchronize VM creation on ctxlock

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Uninitialized pointer dereference in msm_ioctl_gem_submit bypassing lazy VM initialization
- [High] Privilege escalation via bypassed VM_BIND hardware capability check
--

--- Patch [3]: [PATCH v5 03/10] drm/msm: Synchronize VM creation on ctxlock ---
commit 7a10b1ce7cb7f72d046e9af19750da0d0ced94cb
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Synchronize VM creation on ctxlock

This commit synchronizes virtual memory creation using ctxlock rather
than init_lock. It also adds a read lock when setting EN_VM_BIND to
serialize it against VM creation.

> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
[ ... ]
> @@ -503,7 +503,9 @@ int adreno_set_param(struct msm_gpu *gpu, struct msm_context *ctx,
>  		if (!perfmon_capable())
>  			return UERR(EPERM, drm, "invalid permissions");
>  		return msm_context_set_sysprof(ctx, gpu, value);
> -	case MSM_PARAM_EN_VM_BIND:
> +	case MSM_PARAM_EN_VM_BIND: {
> +		guard(rwsem_read)(&ctx->ctxlock);
> +
>  		/* We can only support VM_BIND with per-process pgtables: */
>  		if (ctx->vm == gpu->vm)
>  			return UERR(EINVAL, drm, "requires per-process pgtables");

[Severity: High]
This is a pre-existing issue, but does checking ctx->vm == gpu->vm
properly enforce the per-process pgtable requirement? 

Since the VM is lazily initialized, wouldn't ctx->vm be NULL here?
(This appears to be expected by the subsequent if (ctx->vm) check).

Because ctx->vm is NULL, it will never equal gpu->vm, which seems to
completely bypass this hardware capability check. If a legacy device lacks
create_private_vm, couldn't an unprivileged user bypass this check and gain
userspace-managed control over the global GPU page tables when the VM is
later created?


While reviewing the lazy VM initialization semantics in this subsystem, I
noticed another pre-existing issue in a related file not touched by this
patch:

> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -571,4 +571,5 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
>  
>  	if (args->pad)
>  		return -EINVAL;
>  
>  	if (to_msm_vm(ctx->vm)->unusable)
>  		return UERR(EPIPE, dev, "context is unusable");

[Severity: High]
This is a pre-existing issue, but does msm_ioctl_gem_submit() safely access
the virtual memory context here?

If a user opens the DRM device to create a new context and immediately issues
a DRM_IOCTL_MSM_GEM_SUBMIT ioctl, ctx->vm will still be NULL because it has
not yet been lazily initialized via msm_context_vm().

Could this direct dereference of ctx->vm cause a reliable kernel panic when
evaluated?

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