[PATCH v2 0/3] drm/msm: A couple lazy-vm fixes

[PATCH v2 0/3] drm/msm: A couple lazy-vm fixes

[Rob Clark]

The VM is created lazily to give userspace a chance to opt-in to
VM_BIND.  But Sashiko noticed a couple paths that were not handling
this properly.

Rob Clark (3):
  drm/msm: Fix barriers accessing ctx vm
  drm/msm: Validate lazy VM is created in GEM_SUBMIT
  drm/msm: Validate lazy VM in GEM_NEW

 drivers/gpu/drm/msm/msm_drv.c        | 13 +++++++------
 drivers/gpu/drm/msm/msm_gem.c        |  7 ++++++-
 drivers/gpu/drm/msm/msm_gem_submit.c |  9 +++++----
 3 files changed, 18 insertions(+), 11 deletions(-)

-- 
2.54.0

[PATCH v2 1/3] drm/msm: Fix barriers accessing ctx vm

[Rob Clark]

Don't rely on store ordering to protect us from caller seeing a
partially initialized vm.

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>
---
 drivers/gpu/drm/msm/msm_drv.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 32d5ebea2596..ec88155e0ed7 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -224,18 +224,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;
 }
-- 
2.54.0

Re: [PATCH v2 1/3] drm/msm: Fix barriers accessing ctx vm

[sashiko-bot]

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

Pre-existing issues:
- [High] Direct access to uninitialized `ctx->vm` in `adreno_set_param` bypasses validation, allowing `VM_BIND` on global VMs.
--

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

drm/msm: Fix barriers accessing ctx vm

This commit adds load-acquire and store-release barriers when accessing
and lazily initializing ctx->vm in msm_context_vm(). This prevents callers
from seeing a partially initialized virtual memory state.

> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 32d5ebea2596f..ec88155e0ed72 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -224,18 +224,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;

[Severity: High]
This isn't a bug introduced by this patch, but seeing the fixes to ctx->vm
lazy initialization here points to a related problem in adreno_set_param().

Looking at adreno_set_param() in drivers/gpu/drm/msm/adreno/adreno_gpu.c,
there is this check when enabling VM_BIND:

    if (ctx->vm == gpu->vm)
        return UERR(EINVAL, drm, "requires per-process pgtables");

Since ctx->vm starts as NULL and is lazily initialized, couldn't this
check fail to catch global VMs (since NULL != gpu->vm) if msm_context_vm()
hasn't been called yet?

If unprivileged userspace opts into VM_BIND mode before the VM is
initialized, could this bypass the validation check on platforms without
per-process pagetables, potentially allowing arbitrary mapping in the
global GPU address space? 

Should this be using msm_context_vm(dev, ctx) instead to ensure the
VM is instantiated before checking?

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

[PATCH v2 2/3] drm/msm: Validate lazy VM is created in GEM_SUBMIT

[Rob Clark]

Otherwise a GEM_SUBMIT ioctl before any BOs are mapped could cause a
NPE.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Fixes: 6a4d287a1ae6 ("drm/msm: Mark VM as unusable on GPU hangs")
Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/msm_gem_submit.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index 3c6bc90c3d48..56929e821200 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -30,7 +30,7 @@
  */
 
 static struct msm_gem_submit *submit_create(struct drm_device *dev,
-		struct msm_gpu *gpu,
+		struct msm_gpu *gpu, struct drm_gpuvm *vm,
 		struct msm_gpu_submitqueue *queue, uint32_t nr_bos,
 		uint32_t nr_cmds, u64 drm_client_id)
 {
@@ -66,7 +66,7 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev,
 
 	kref_init(&submit->ref);
 	submit->dev = dev;
-	submit->vm = msm_context_vm(dev, queue->ctx);
+	submit->vm = vm;
 	submit->gpu = gpu;
 	submit->cmd = (void *)&submit->bos[nr_bos];
 	submit->queue = queue;
@@ -554,6 +554,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
 	struct msm_drm_private *priv = dev->dev_private;
 	struct drm_msm_gem_submit *args = data;
 	struct msm_context *ctx = file->driver_priv;
+	struct drm_gpuvm *vm = msm_context_vm(dev, ctx);
 	struct msm_gem_submit *submit = NULL;
 	struct msm_gpu *gpu = priv->gpu;
 	struct msm_gpu_submitqueue *queue;
@@ -572,7 +573,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
 	if (args->pad)
 		return -EINVAL;
 
-	if (to_msm_vm(ctx->vm)->unusable)
+	if (vm && to_msm_vm(vm)->unusable)
 		return UERR(EPIPE, dev, "context is unusable");
 
 	/* for now, we just have 3d pipe.. eventually this would need to
@@ -609,7 +610,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
 		}
 	}
 
-	submit = submit_create(dev, gpu, queue, args->nr_bos, args->nr_cmds,
+	submit = submit_create(dev, gpu, vm, queue, args->nr_bos, args->nr_cmds,
 			       file->client_id);
 	if (IS_ERR(submit)) {
 		ret = PTR_ERR(submit);
-- 
2.54.0

Re: [PATCH v2 2/3] drm/msm: Validate lazy VM is created in GEM_SUBMIT

[sashiko-bot]

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

Pre-existing issues:
- [Critical] The MSM_PARAM_EN_VM_BIND ioctl validation compares an uninitialized context VM against the global VM, failing to prevent VM_BIND enablement on GPUs without per-process page tables.
--

commit 0fcb5ea6b90013aec15c2f186ee0bbf02e4e4818
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Validate lazy VM is created in GEM_SUBMIT
    
Otherwise a GEM_SUBMIT ioctl before any BOs are mapped could cause a
NPE.

> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> index 3c6bc90c3d486..56929e8212008 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
[ ... ]
> @@ -572,7 +573,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
>  	if (args->pad)
>  		return -EINVAL;
>  
> -	if (to_msm_vm(ctx->vm)->unusable)
> +	if (vm && to_msm_vm(vm)->unusable)
>  		return UERR(EPIPE, dev, "context is unusable");

[Severity: Critical]
This is a pre-existing issue, but since we are fixing lazy VM access
here, is there a similar issue in adreno_set_param() when handling
MSM_PARAM_EN_VM_BIND?

Looking at drivers/gpu/drm/msm/adreno/adreno_gpu.c:

adreno_set_param()
{
    ...
	case MSM_PARAM_EN_VM_BIND:
		/* We can only support VM_BIND with per-process pgtables: */
		if (ctx->vm == gpu->vm)
			return UERR(EINVAL, drm, "requires per-process pgtables");

Because ctx->vm can be NULL prior to lazy initialization, this check
evaluates to false, allowing the restriction to be bypassed.

Could this allow an unprivileged user to enable userspace management
(VM_BIND) on older GPUs that fall back to the globally shared GPU VM,
and overwrite page tables shared by other processes?

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

[PATCH v2 3/3] drm/msm: Validate lazy VM in GEM_NEW

[Rob Clark]

Otherwise creating a _NO_SHARE BO before any BOs are mapped could cause
a NPE.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Fixes: b58e12a66e47 ("drm/msm: Add _NO_SHARE flag")
Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/msm_gem.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index efd3d3c9a449..733a094354e1 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -1147,7 +1147,12 @@ int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
 
 	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);
+		struct drm_gpuvm *vm = msm_context_vm(dev, ctx);
+
+		if (!vm)
+			return UERR(EINVAL, dev, "not supported with shared VM");
+
+		struct drm_gem_object *r_obj = drm_gpuvm_resv_obj(vm);
 
 		drm_gem_object_get(r_obj);
 
-- 
2.54.0

Re: [PATCH v2 3/3] drm/msm: Validate lazy VM in GEM_NEW

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The newly allocated GEM object 'obj' is leaked on the error path if the VM is not found or unsupported.
--

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

drm/msm: Validate lazy VM in GEM_NEW

Otherwise creating a _NO_SHARE BO before any BOs are mapped could cause
a NPE.

> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index efd3d3c9a4490..733a094354e1d 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -1147,7 +1147,12 @@ int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
>  
>  	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);
> +		struct drm_gpuvm *vm = msm_context_vm(dev, ctx);
> +
> +		if (!vm)
> +			return UERR(EINVAL, dev, "not supported with shared VM");
                        ^^^^

[Severity: High]
Does this code leak the newly allocated obj?

Looking earlier in msm_gem_new_handle(), obj is allocated:

	obj = msm_gem_new(dev, size, flags);

	if (IS_ERR(obj))
		return PTR_ERR(obj);

If vm is NULL, we return directly here without calling drm_gem_object_put(obj)
to release the reference.

> +
> +		struct drm_gem_object *r_obj = drm_gpuvm_resv_obj(vm);
>  
>  		drm_gem_object_get(r_obj);
>

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