Re: [PATCH v6 01/16] drm/msm: Fix barriers accessing ctx vm

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Direct access to ctx->vm before lazy initialization causes a NULL pointer dereference.
- [High] TOCTOU race in MSM_PARAM_EN_VM_BIND allows invalid VM configuration.
--

--- Patch [1]: [PATCH v6 01/16] drm/msm: Fix barriers accessing ctx vm ---
commit ca8b3357d768cc8c8e48035f7adbc82abd4eb8c7
Author: Rob Clark <robin.clark@oss.qualcomm.com>

    drm/msm: Fix barriers accessing ctx vm
    
    Don't rely on store ordering to protect us from caller seeing a
    partially initialized vm.

> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 79f78e7e16e9d..3c8b5083a1fbe 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -225,18 +225,19 @@ 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);
>  
>  	/* Once ctx->vm is created it is valid for the lifetime of the context: */
> -	if (ctx->vm)
> -		return ctx->vm;
> +	if (vm)
> +		return vm;
> +
> +	guard(mutex)(&init_lock);
>  
> -	mutex_lock(&init_lock);
>  	if (!ctx->vm) {
> -		ctx->vm = msm_gpu_create_private_vm(
> +		vm = msm_gpu_create_private_vm(
>  			priv->gpu, current, !ctx->userspace_managed_vm);
> -
> +		smp_store_release(&ctx->vm, vm);
>  	}
> -	mutex_unlock(&init_lock);
>  
>  	return ctx->vm;
>  }

[Severity: Critical]
This is a pre-existing issue, but I noticed that ctx->vm can be accessed 
directly before lazy initialization. If an unprivileged user opens the DRM 
device and immediately calls the MSM_GEM_SUBMIT IOCTL, it evaluates 
to_msm_vm(ctx->vm)->unusable without ensuring the VM has been initialized via 
msm_context_vm():

drivers/gpu/drm/msm/msm_gem_submit.c:msm_ioctl_gem_submit() {
    ...
	if (to_msm_vm(ctx->vm)->unusable)
		return UERR(EPIPE, dev, "context is unusable");
    ...
}

Since ctx->vm is NULL, the to_msm_vm() macro computes an invalid pointer and 
dereferences its unusable field, causing a panic.

Similarly, calling the MSM_GEM_NEW IOCTL with the MSM_BO_NO_SHARE flag 
executes drm_gpuvm_resv_obj(ctx->vm) which also dereferences NULL:

drivers/gpu/drm/msm/msm_gem.c:msm_gem_new_handle() {
    ...
	if (flags & MSM_BO_NO_SHARE) {
		struct msm_context *ctx = file->driver_priv;
		struct drm_gem_object *r_obj = drm_gpuvm_resv_obj(ctx->vm);
    ...
}

Can these paths be updated to ensure the VM is initialized via 
msm_context_vm() before accessing it?


[Severity: High]
This isn't a bug introduced by this patch, but I found a potential TOCTOU 
race in MSM_PARAM_EN_VM_BIND that could allow an invalid VM configuration. 

In adreno_gpu_set_param(), it locklessly checks if (ctx->vm):

drivers/gpu/drm/msm/adreno/adreno_gpu.c:adreno_gpu_set_param() {
    ...
	case MSM_PARAM_EN_VM_BIND:
        ...
		if (ctx->vm)
			return UERR(EBUSY, drm, "VM already created");

		ctx->userspace_managed_vm = value;
    ...
}

If Thread A calls the MSM_SET_PARAM IOCTL, checks ctx->vm and sees NULL, and 
concurrently Thread B calls an IOCTL like MSM_INFO_GET_IOVA which calls 
msm_context_vm(), Thread B will create the VM as kernel-managed. Thread A 
then resumes and sets ctx->userspace_managed_vm = 1. 

Does this result in an inconsistent state where the context claims userspace 
management but the underlying VM is kernel-managed?

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