Re: [PATCH v6 03/16] drm/msm: Synchronize VM creation on ctxlock

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] `msm_context_vm()` propagates errors as `ERR_PTR` or `NULL` without callers checking, leading to wild pointer dereferences.
- [Critical] Direct access to uninitialized `ctx->vm` in `msm_ioctl_gem_submit()` causes a guaranteed NULL pointer dereference.
- [High] Hardware capability check in `adreno_set_param()` fails due to lazy VM initialization, bypassing restrictions.
--

--- Patch [3]: [PATCH v6 03/16] drm/msm: Synchronize VM creation on ctxlock ---
commit 78d3ed784b530d2c180edb9b49939bea521dccd3
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Synchronize VM creation on ctxlock
    
This commit serializes the setting of EN_VM_BIND against VM creation
by synchronizing it on ctxlock.

> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index c62c45bb0ddbb..0d54141ff089c 100644
> --- 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)

[Severity: High]
This isn't a bug introduced by this patch, but does this hardware capability
check properly account for lazy VM initialization?

Since VMs are lazily initialized, ctx->vm will be NULL at this stage. This
means the check evaluates NULL == gpu->vm, which is always false. Does this
allow all hardware, even those without per-process pagetables, to bypass this 
restriction?

>  			return UERR(EINVAL, drm, "requires per-process pgtables");

[ ... ]

> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 380b3e2bf734e..20c5a6cc82019 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -223,7 +223,6 @@ static void load_gpu(struct drm_device *dev)
>   */
>  struct drm_gpuvm *msm_context_vm(struct drm_device *dev, struct msm_context *ctx)
>  {
> -	static DEFINE_MUTEX(init_lock);
>  	struct msm_drm_private *priv = dev->dev_private;
>  	struct drm_gpuvm *vm = smp_load_acquire(&ctx->vm);
>  
> @@ -231,7 +230,7 @@ struct drm_gpuvm *msm_context_vm(struct drm_device *dev, struct msm_context *ctx
>  	if (vm)
>  		return vm;
>  
> -	guard(mutex)(&init_lock);
> +	guard(rwsem_write)(&ctx->ctxlock);
>  
>  	if (!ctx->vm) {
>  		vm = msm_gpu_create_private_vm(

[Severity: Critical]
This is a pre-existing issue, but does msm_context_vm() propagate an
unhandled error pointer or NULL on failure?

If msm_gpu_create_private_vm() returns ERR_PTR or NULL (e.g., during an OOM
condition or on platforms without per-process pagetables), it appears
msm_context_vm() stores and returns this directly. 

Callers like msm_submitqueue_create() in msm_submitqueue.c blindly assume
success:

    struct drm_gpu_scheduler *sched = &to_msm_vm(msm_context_vm(drm, ctx))->sched;

If an error pointer or NULL is returned, to_msm_vm() yields an invalid
address. Could this lead to an immediate kernel crash when it is
dereferenced in drm_sched_entity_init()?


[Severity: Critical]
This is another pre-existing issue, but does direct access to ctx->vm cause a
NULL pointer dereference?

If a user issues a DRM_IOCTL_MSM_GEM_SUBMIT ioctl before executing any
commands that lazily initialize the VM, ctx->vm is still NULL.

In msm_ioctl_gem_submit():

    if (to_msm_vm(ctx->vm)->unusable)
        return UERR(EPIPE, dev, "context is unusable");

Because ctx->vm is NULL, the to_msm_vm() macro returns a negative offset
pointer. Does dereferencing ->unusable here immediately crash the kernel
instead of calling msm_context_vm() to lazily initialize it?

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