[PATCH v4 0/6] drm/msm: A couple lazy-vm fixes

[PATCH v4 0/6] 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 (6):
  drm/msm: Fix barriers accessing ctx vm
  drm/msm: Block VM_BIND for shared VMs
  drm/msm: Validate lazy VM is created in GEM_SUBMIT
  drm/msm: Validate lazy VM in GEM_NEW
  drm/msm: Fix per-process-pgtables check
  drm/msm: Remove invalid overflow check

 drivers/gpu/drm/msm/adreno/adreno_gpu.c |  2 +-
 drivers/gpu/drm/msm/msm_drv.c           | 13 +++++++------
 drivers/gpu/drm/msm/msm_gem.c           | 11 ++++++++++-
 drivers/gpu/drm/msm/msm_gem_submit.c    | 15 ++++++---------
 drivers/gpu/drm/msm/msm_gem_vma.c       | 11 ++++++-----
 5 files changed, 30 insertions(+), 22 deletions(-)

-- 
2.55.0

[PATCH v4 1/6] 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.55.0

Re: [PATCH v4 1/6] drm/msm: Fix barriers accessing ctx vm

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Silent fallback to the global GPU VM when private VM allocation fails breaks memory isolation.
- [High] Direct access to ctx->vm before initialization causes a NULL pointer dereference in IOCTL handlers.
- [High] Missing smp_load_acquire() when reading ctx->vm in msm_gem_close() can lead to accessing partially initialized memory.
--

--- Patch [1]: [PATCH v4 1/6] drm/msm: Fix barriers accessing ctx vm ---
commit 5827c45dcfd83b1ca9d1ff0ea3f3c026fba67fb3
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Fix barriers accessing ctx vm

This commit fixes memory barrier issues when accessing the context VM
by introducing smp_load_acquire and smp_store_release in msm_context_vm.
It prevents callers from seeing a partially initialized virtual machine.

> 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);

[Severity: Critical]
This isn't a bug introduced by this patch, but looking at the VM creation
logic in msm_gpu_create_private_vm() called above, does it safely handle
memory allocation failures?

drivers/gpu/drm/msm/msm_gpu.c:msm_gpu_create_private_vm() {
    ...
    if (gpu->funcs->create_private_vm) {
        vm = gpu->funcs->create_private_vm(gpu, kernel_managed);
        ...
    }

    if (IS_ERR_OR_NULL(vm))
        vm = drm_gpuvm_get(gpu->vm);
    
    return vm;
}

If the private VM allocation fails (returning ERR_PTR(-ENOMEM) due to
system memory exhaustion), this fallback grants the userspace context access
to the shared global GPU VM.

Could an attacker intentionally exhaust memory to force this fallback,
thereby bypassing private VM isolation and gaining unauthorized access to
structures stored in the global VM?

> -
> +		smp_store_release(&ctx->vm, vm);
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but since we are fixing the
barriers for ctx->vm initialization here, does msm_gem_close() also need an
smp_load_acquire() barrier?

Looking at msm_gem_close() in drivers/gpu/drm/msm/msm_gem.c, it reads
ctx->vm using a plain load:

drivers/gpu/drm/msm/msm_gem.c:msm_gem_close() {
    ...
    if (!ctx->vm)
        return;
    ...
}

If msm_gem_close() executes concurrently with VM creation, could it see a
non-NULL pointer without an acquire barrier and access partially
initialized memory?

[Severity: High]
This is a pre-existing issue, but I noticed there are paths that directly
access ctx->vm without calling msm_context_vm() first to ensure it is
created.

For example, in msm_ioctl_gem_submit():

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

And in msm_gem_new_handle():

drivers/gpu/drm/msm/msm_gem.c:msm_gem_new_handle() {
    ...
    struct drm_gem_object *r_obj = drm_gpuvm_resv_obj(ctx->vm);
    ...
}

If userspace calls either of these IOCTLs on a fresh context before the
VM is initialized, could ctx->vm be NULL here, leading to a NULL pointer
dereference when computing the struct offset or dereferencing it?

> -	mutex_unlock(&init_lock);
>  
>  	return ctx->vm;
>  }

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

[PATCH v4 2/6] drm/msm: Block VM_BIND for shared VMs

[Rob Clark]

We already block MSM_PARAM_EN_VM_BIND where per-process VMs are not
supported.  But in error paths when we actually try to create the VM
(allocation failures, etc) we could still end up with a shared-VM.

Since we need to create the VM lazily, for backwards compat, there
isn't really a better place to return an error.  But this isn't to
happen in practice.  Just block VM_BIND ioctls in this case so we
aren't giving userspace a way to manage the shared VM.

Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/msm_gem_vma.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
index 3ed05ab0eeef..06a3f2aa3ef0 100644
--- a/drivers/gpu/drm/msm/msm_gem_vma.c
+++ b/drivers/gpu/drm/msm/msm_gem_vma.c
@@ -954,7 +954,7 @@ msm_gem_vm_close(struct drm_gpuvm *gpuvm)
 
 
 static struct msm_vm_bind_job *
-vm_bind_job_create(struct drm_device *dev, struct drm_file *file,
+vm_bind_job_create(struct drm_device *dev, struct drm_file *file, struct drm_gpuvm *vm,
 		   struct msm_gpu_submitqueue *queue, uint32_t nr_ops)
 {
 	struct msm_vm_bind_job *job;
@@ -971,7 +971,7 @@ vm_bind_job_create(struct drm_device *dev, struct drm_file *file,
 		return ERR_PTR(ret);
 	}
 
-	job->vm = msm_context_vm(dev, queue->ctx);
+	job->vm = vm;
 	job->queue = queue;
 	INIT_LIST_HEAD(&job->vm_ops);
 
@@ -1430,6 +1430,7 @@ msm_ioctl_vm_bind(struct drm_device *dev, void *data, struct drm_file *file)
 	struct msm_drm_private *priv = dev->dev_private;
 	struct drm_msm_vm_bind *args = data;
 	struct msm_context *ctx = file->driver_priv;
+	struct drm_gpuvm *vm = msm_context_vm(dev, ctx);
 	struct msm_vm_bind_job *job = NULL;
 	struct msm_gpu *gpu = priv->gpu;
 	struct msm_gpu_submitqueue *queue;
@@ -1448,7 +1449,7 @@ msm_ioctl_vm_bind(struct drm_device *dev, void *data, struct drm_file *file)
 	 * Maybe we could allow just UNMAP ops?  OTOH userspace should just
 	 * immediately close the device file and all will be torn down.
 	 */
-	if (to_msm_vm(msm_context_vm(dev, ctx))->unusable)
+	if (to_msm_vm(vm)->unusable)
 		return UERR(EPIPE, dev, "context is unusable");
 
 	/*
@@ -1456,7 +1457,7 @@ msm_ioctl_vm_bind(struct drm_device *dev, void *data, struct drm_file *file)
 	 * place, if you haven't opted in to VM_BIND context.  But it is
 	 * cleaner / less confusing, to check this case directly.
 	 */
-	if (!msm_context_is_vmbind(ctx))
+	if (to_msm_vm(vm)->managed)
 		return UERR(EINVAL, dev, "context does not support vmbind");
 
 	if (args->flags & ~MSM_VM_BIND_FLAGS)
@@ -1479,7 +1480,7 @@ msm_ioctl_vm_bind(struct drm_device *dev, void *data, struct drm_file *file)
 		}
 	}
 
-	job = vm_bind_job_create(dev, file, queue, args->nr_ops);
+	job = vm_bind_job_create(dev, file, vm, queue, args->nr_ops);
 	if (IS_ERR(job)) {
 		ret = PTR_ERR(job);
 		goto out_post_unlock;
-- 
2.55.0

Re: [PATCH v4 2/6] drm/msm: Block VM_BIND for shared VMs

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Incomplete fix: Falling back to the shared VM on private VM allocation failure allows EXECBUF isolation bypass, arbitrary kernel memory access, and information leaks.
- [High] Direct access to `ctx->vm` in `msm_ioctl_gem_submit()` before the VM is guaranteed to be initialized causes a NULL pointer dereference.
--

--- Patch [2]: [PATCH v4 2/6] drm/msm: Block VM_BIND for shared VMs ---
commit 14b0c06028a80fcfb812d65ea727ff988063c94e
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Block VM_BIND for shared VMs

We already block MSM_PARAM_EN_VM_BIND where per-process VMs are not
supported.  But in error paths when we actually try to create the VM
(allocation failures, etc) we could still end up with a shared-VM.

Since we need to create the VM lazily, for backwards compat, there
isn't really a better place to return an error.  But this isn't to
happen in practice.  Just block VM_BIND ioctls in this case so we
aren't giving userspace a way to manage the shared VM.

> diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
> index 1a952b171ed7f..c88b4352ad751 100644
> --- a/drivers/gpu/drm/msm/msm_gem_vma.c
> +++ b/drivers/gpu/drm/msm/msm_gem_vma.c
[ ... ]
> @@ -1449,7 +1450,7 @@ msm_ioctl_vm_bind(struct drm_device *dev, void *data, struct drm_file *file)
>  	 * Maybe we could allow just UNMAP ops?  OTOH userspace should just
>  	 * immediately close the device file and all will be torn down.
>  	 */
> -	if (to_msm_vm(msm_context_vm(dev, ctx))->unusable)
> +	if (to_msm_vm(vm)->unusable)
>  		return UERR(EPIPE, dev, "context is unusable");
>  
>  	/*
> @@ -1457,7 +1458,7 @@ msm_ioctl_vm_bind(struct drm_device *dev, void *data, struct drm_file *file)
>  	 * place, if you haven't opted in to VM_BIND context.  But it is
>  	 * cleaner / less confusing, to check this case directly.
>  	 */
> -	if (!msm_context_is_vmbind(ctx))
> +	if (to_msm_vm(vm)->managed)

[Severity: Critical]
This is a pre-existing issue, but does falling back to the shared VM on private
VM allocation failure allow an execbuf isolation bypass?

While this patch blocks vm_bind ioctls for shared VMs, an attacker could still
opt into vm_bind (setting ctx->userspace_managed_vm = true) and intentionally
cause private VM allocation to fail. In this case, msm_gpu_create_private_vm()
falls back to the global kernel VM:

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

If the attacker then creates a normal submitqueue and calls
msm_ioctl_gem_submit(), command validation is skipped because
msm_context_is_vmbind(ctx) still returns true based on user intent rather
than actual VM state:

drivers/gpu/drm/msm/msm_gem_submit.c:msm_ioctl_gem_submit() {
	...
	cmds_to_parse = msm_context_is_vmbind(ctx) ? 0 : args->nr_cmds;
	...
}

This allows the unvalidated user-provided IOVA to be mapped and executed
within the global kernel VM:

drivers/gpu/drm/msm/msm_gem_submit.c:submit_lookup_cmds() {
	...
	submit->cmd[i].iova = submit_cmd.iova;
	...
}

Could this lead to arbitrary kernel memory access, or leak other processes'
data into the devcoredump if a job hangs?

[Severity: High]
This isn't a bug introduced by this patch, but does direct access to ctx->vm
in msm_ioctl_gem_submit() cause a null pointer dereference?

If a user directly calls msm_ioctl_gem_submit() on a fresh context without
performing any prior operation (like msm_ioctl_vm_bind) that initializes the
VM, ctx->vm remains null due to lazy initialization.

Does this check in msm_ioctl_gem_submit() cause an immediate crash when
dereferencing the offset to unusable?

drivers/gpu/drm/msm/msm_gem_submit.c:msm_ioctl_gem_submit() {
	...
	if (to_msm_vm(ctx->vm)->unusable)
	...
}

>  		return UERR(EINVAL, dev, "context does not support vmbind");
>  
>  	if (args->flags & ~MSM_VM_BIND_FLAGS)

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

[PATCH v4 3/6] 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.55.0

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

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Direct access to `ctx->vm` without calling `msm_context_vm()` leads to a NULL pointer dereference in `msm_gem_new_handle()`.
- [Critical] Adding to the saturated result of `struct_size()` causes an integer wrap-around on overflow, defeating overflow protection and leading to a massive heap buffer overflow.
- [High] Validation bypass in `adreno_set_param()` when enabling `MSM_PARAM_EN_VM_BIND` allows userspace to enable VM BIND on unsupported hardware, leading to a system-wide GPU DoS.
--

--- Patch [3]: [PATCH v4 3/6] drm/msm: Validate lazy VM is created in GEM_SUBMIT ---
commit a317c04b26f6bc5a511792869e55bcddc1d9014f
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Validate lazy VM is created in GEM_SUBMIT

This commit fixes a potential NULL pointer dereference in the GEM_SUBMIT
ioctl by ensuring the lazy VM is created before validating it. It passes the
instantiated VM to submit_create() to avoid redundant context lookups.

> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> index 26ea8a28be474..0dc983d155cd2 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)
>  {

[Severity: Critical]
This is a pre-existing issue, but can this size calculation overflow?

submit_create() calculates the size as:

	sz = struct_size(submit, bos, nr_bos) +
			((u64)nr_cmds * sizeof(submit->cmd[0]));

	if (sz > SIZE_MAX)
		return ERR_PTR(-ENOMEM);

If struct_size() saturates to SIZE_MAX, adding to it will wrap around uint64_t.

Also, the sz > SIZE_MAX check is dead code on 64-bit systems because an
unsigned 64-bit integer can never exceed SIZE_MAX. This defeats overflow
protection and could allow an undersized allocation to succeed, potentially
leading to an out-of-bounds heap write when submit->bos and submit->cmd
are processed.

[ ... ]

While reviewing this commit's handling of lazy VM initialization, I noticed two
other pre-existing issues in related code where ctx->vm is accessed without
being instantiated:

[Severity: Critical]
This is a pre-existing issue, but does msm_gem_new_handle() dereference a
NULL pointer if the VM isn't created yet?

In drivers/gpu/drm/msm/msm_gem.c:msm_gem_new_handle():

	struct drm_gem_object *r_obj = drm_gpuvm_resv_obj(ctx->vm);

Since ctx->vm is lazily initialized, it will be NULL if msm_context_vm()
hasn't been called. drm_gpuvm_resv_obj expands to (ctx->vm)->r_obj, which
would dereference NULL. Could an unprivileged userspace program trigger
this by calling DRM_IOCTL_MSM_GEM_NEW with MSM_BO_NO_SHARE immediately
after opening the device?

[Severity: High]
This is a pre-existing issue, but is the VM_BIND validation bypassed in
adreno_set_param()?

In drivers/gpu/drm/msm/adreno/adreno_gpu.c:adreno_set_param():

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

Because ctx->vm is lazily created, it is NULL here. This makes the check
evaluate to NULL == gpu->vm, which is false, bypassing the validation.
Could this allow userspace to enable VM BIND on unsupported hardware and
cause lock contention during job submission?

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

[PATCH v4 4/6] 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 | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index efd3d3c9a449..4442cfa55b00 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -1146,8 +1146,16 @@ int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
 		msm_gem_object_set_name(obj, "%s", name);
 
 	if (flags & MSM_BO_NO_SHARE) {
+		struct msm_drm_private *priv = dev->dev_private;
 		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 (!priv->gpu || (vm == priv->gpu->vm)) {
+			ret = UERR(EINVAL, dev, "not supported with shared VM");
+			goto out_put;
+		}
+
+		struct drm_gem_object *r_obj = drm_gpuvm_resv_obj(vm);
 
 		drm_gem_object_get(r_obj);
 
@@ -1157,6 +1165,7 @@ int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
 	ret = drm_gem_handle_create(file, obj, handle);
 
 	/* drop reference from allocate - handle holds it now */
+out_put:
 	drm_gem_object_put(obj);
 
 	return ret;
-- 
2.55.0

[PATCH v4 5/6] drm/msm: Fix per-process-pgtables check

[Rob Clark]

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.

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/adreno/adreno_gpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index c62c45bb0ddb..bff2a773c0d4 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -505,7 +505,7 @@ int adreno_set_param(struct msm_gpu *gpu, struct msm_context *ctx,
 		return msm_context_set_sysprof(ctx, gpu, value);
 	case MSM_PARAM_EN_VM_BIND:
 		/* 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");
 
 		/*
-- 
2.55.0

Re: [PATCH v4 5/6] drm/msm: Fix per-process-pgtables check

[sashiko-bot]

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

Pre-existing issues:
- [High] Concurrent ioctl calls to MSM_PARAM_SYSPROF can cause unbalanced pm_runtime and sysprof_active refcounts.
- [Medium] TOCTOU race in MSM_PARAM_EN_VM_BIND handler leading to mismatched VM state.
--

--- Patch [5]: [PATCH v4 5/6] drm/msm: Fix per-process-pgtables check ---
commit 445f3f992eb83cba3cf22345c8afc2a87202a7ce
Author: Rob Clark <robin.clark@oss.qualcomm.com>
Subject: drm/msm: Fix per-process-pgtables check

This patch fixes the per-process-pgtable check in adreno_set_param() by
verifying create_private_vm instead of ctx->vm, as ctx->vm is not yet
initialized at this point.

> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index c62c45bb0ddbb..bff2a773c0d4e 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -505,7 +505,7 @@ int adreno_set_param(struct msm_gpu *gpu, struct msm_context *ctx,
>  		return msm_context_set_sysprof(ctx, gpu, value);

[Severity: High]
This isn't a bug introduced by this patch, but does calling
msm_context_set_sysprof() here without locks allow a race condition?

If two threads concurrently call MSM_PARAM_SYSPROF to transition from 2 to 0,
couldn't they both read ctx->sysprof == 2 in msm_context_set_sysprof() and both
call pm_runtime_put_autosuspend() and refcount_dec()?

This might underflow the pm_runtime refcount, causing premature hardware
suspension.

>  	case MSM_PARAM_EN_VM_BIND:
>  		/* 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");
>  
>  		/*
>  		 * We can only swtich to VM_BIND mode if the VM has not yet
>  		 * been created:
>  		 */
>  		if (ctx->vm)
>  			return UERR(EBUSY, drm, "VM already created");
>  
>  		ctx->userspace_managed_vm = value;

[Severity: Medium]
This is a pre-existing issue, but since the check if (ctx->vm) and the
assignment ctx->userspace_managed_vm = value are performed locklessly in
adreno_set_param(), is it possible for a concurrent lazy VM creation to race
with this?

If thread A issues MSM_PARAM_EN_VM_BIND, checks if (ctx->vm) and evaluates to
false, thread B could simultaneously issue an ioctl that lazily creates the VM
(e.g., DRM_IOCTL_MSM_GEM_SUBMIT).

Thread B would create the VM and set ctx->vm, and then thread A proceeds to set
ctx->userspace_managed_vm = value. This would leave the context state
desynchronized from the actual VM state.

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

[PATCH v4 6/6] drm/msm: Remove invalid overflow check

[Rob Clark]

On overflow struct_size() would return SIZE_MAX.  But kzalloc() (and
friends) check this already, so we can just remove the check.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/msm_gem_submit.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index 56929e821200..3113faedd04c 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -36,15 +36,11 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev,
 {
 	static atomic_t ident = ATOMIC_INIT(0);
 	struct msm_gem_submit *submit;
-	uint64_t sz;
+	size_t sz;
 	int ret;
 
 	sz = struct_size(submit, bos, nr_bos) +
 			((u64)nr_cmds * sizeof(submit->cmd[0]));
-
-	if (sz > SIZE_MAX)
-		return ERR_PTR(-ENOMEM);
-
 	submit = kzalloc(sz, GFP_KERNEL | __GFP_NOWARN);
 	if (!submit)
 		return ERR_PTR(-ENOMEM);
-- 
2.55.0

Re: [PATCH v4 6/6] drm/msm: Remove invalid overflow check

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Integer Truncation leading to Heap Buffer Overflow on 32-bit architectures.
--

--- Patch [6]: [PATCH v4 6/6] drm/msm: Remove invalid overflow check ---
commit b6fd344e5aae554bc6124eb5aa28a19da1bb53c5
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Remove invalid overflow check

This commit changes the size variable for the msm_gem_submit allocation
from uint64_t to size_t and removes the explicit check against SIZE_MAX,
relying on kzalloc to handle struct_size overflows natively.

> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> index 0dc983d155cd2..744af6389ac62 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -36,15 +36,11 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev,
>  {
>  	static atomic_t ident = ATOMIC_INIT(0);
>  	struct msm_gem_submit *submit;
> -	uint64_t sz;
> +	size_t sz;
>  	int ret;
>  
>  	sz = struct_size(submit, bos, nr_bos) +
>  			((u64)nr_cmds * sizeof(submit->cmd[0]));
> -
> -	if (sz > SIZE_MAX)
> -		return ERR_PTR(-ENOMEM);
> -

[Severity: Critical]
Does changing sz to size_t introduce an integer truncation vulnerability on
32-bit architectures?

The right side of this assignment evaluates to a 64-bit value because
nr_cmds is cast to u64. If struct_size() overflows and returns SIZE_MAX, or if
nr_cmds is extremely large, the addition results in a value exceeding 4GB.

When this 64-bit value is assigned to a 32-bit size_t variable on 32-bit
architectures, it will implicitly truncate and wrap around to a small value.
Since kzalloc() is now passed this truncated size, it will successfully
allocate a small buffer instead of failing.

Could this allow user-controlled nr_cmds and nr_bos values to trigger a heap
buffer overflow when subsequent loops write past the allocated bounds?

>  	submit = kzalloc(sz, GFP_KERNEL | __GFP_NOWARN);
>  	if (!submit)
>  		return ERR_PTR(-ENOMEM);

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