[PATCH v3 0/4] drm/msm: A couple lazy-vm fixes

[PATCH v3 0/4] 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 (4):
  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
  drm/msm: Fix per-process-pgtables 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           | 10 +++++++++-
 drivers/gpu/drm/msm/msm_gem_submit.c    |  9 +++++----
 4 files changed, 22 insertions(+), 12 deletions(-)

-- 
2.54.0

[PATCH v3 1/4] 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

[PATCH v3 2/4] 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 v3 2/4] 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:
- [High] Ineffective integer overflow protection during allocation size calculation in `submit_create` leads to potential heap buffer overflows.
--

commit 0bc0c946b740e56645786e074bd1e98b7bdd28a0
Author: Rob Clark <robin.clark@oss.qualcomm.com>
Subject: drm/msm: Validate lazy VM is created in GEM_SUBMIT

This commit fixes a potential NULL pointer dereference in the GEM_SUBMIT
ioctl. It ensures that the lazy VM is validated and created before any
buffer objects are mapped.

> 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: High]
This is a pre-existing issue, but does the overflow check in this function
work as intended?

In submit_create(), sz is calculated using struct_size() and assigned to a
uint64_t variable:

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

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

Since sz and SIZE_MAX share the same maximum width on 64-bit architectures,
can sz ever evaluate to strictly greater than SIZE_MAX?

If the struct_size() calculation saturates to SIZE_MAX upon overflow, the
subsequent addition of the command array size could wrap sz around to a small
positive value.

This would cause the check to evaluate to false and bypass the protection,
allocating a memory block that is too small for the requested elements and
allowing a potential heap buffer overflow if userspace provides extremely
large values for the number of buffer objects or commands.

[ ... ]

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

[PATCH v3 3/4] 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 | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index efd3d3c9a449..3ff0399c020e 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -1147,7 +1147,14 @@ 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) {
+			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 +1164,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.54.0

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

[sashiko-bot]

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

New issues:
- [Medium] The check `if (!vm)` fails to reject shared VMs because `msm_context_vm()` returns the shared VM on older targets.

Pre-existing issues:
- [Critical] Checking `if (sz > SIZE_MAX)` after assigning `struct_size()` to a u64 variable fails to detect overflow on 64-bit systems, leading to a heap buffer overflow.
--

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

drm/msm: Validate lazy VM in GEM_NEW
    
This commit fixes a potential NULL pointer dereference by validating that
a lazy VM exists before creating a _NO_SHARE buffer object.
    
Reported-by: Sashiko <sashiko-bot@kernel.org>
Fixes: b58e12a66e47 ("drm/msm: Add _NO_SHARE flag")

> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index 2cb3ab04f1250..b083790dba042 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -1148,7 +1148,14 @@ 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) {
                ^^^^

[Severity: Medium]
Does this check properly reject shared VMs?

When per-process page tables are unsupported, msm_context_vm() falls
back to returning the global shared VM (gpu->vm). Since the returned
vm is not NULL, this check is bypassed, allowing NO_SHARE buffers to
be created on the shared VM.

Should this check also ensure that the returned VM is not the global
shared VM?

> +			ret = UERR(EINVAL, dev, "not supported with shared VM");
> +			goto out_put;
> +		}
> +
> +		struct drm_gem_object *r_obj = drm_gpuvm_resv_obj(vm);

[ ... ]

While reviewing this, I noticed a pre-existing issue in the msm submit code:

drivers/gpu/drm/msm/msm_gem_submit.c:submit_create() {
	...
	sz = struct_size(submit, bos, nr_bos) +
			((u64)nr_cmds * sizeof(submit->cmd[0]));

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

[Severity: Critical]
This is a pre-existing issue, but does this sz check work on 64-bit systems?

Since sz is a u64, sz > SIZE_MAX is impossible on 64-bit platforms where
SIZE_MAX is U64_MAX. If nr_bos is excessively large, struct_size() returns
SIZE_MAX, and adding the cmd size wraps sz around to a small positive
value.

The check is bypassed, leading to a small buffer allocation. Could this
allow out-of-bounds writes in submit_lookup_objects() when it loops up
to nr_bos?

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

[PATCH v3 4/4] 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.54.0

Re: [PATCH v3 4/4] 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:
- [Critical] Insufficient capability check and silent fallback to the global GPU VM allows userspace to manage the global VM via VM_BIND.
- [High] A time-of-check to time-of-use (TOCTOU) data race when opting into VM_BIND mode allows a state mismatch between `ctx->userspace_managed_vm` and the underlying IOMMU page table configuration.
--

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

drm/msm: Fix per-process-pgtables check

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>

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

[Severity: Critical]
This is a pre-existing issue, but does relying on a static function
pointer here rather than verifying actual VM creation success allow an
invalid configuration to proceed?

If dynamic creation of the private VM fails in msm_gpu_create_private_vm()
(e.g., due to memory allocation failure or missing IOMMU support returning
-ENODEV), it silently falls back to returning the global, kernel-managed
gpu->vm:

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

Because this check allows userspace to set ctx->userspace_managed_vm to
true, could a malicious process then use DRM_IOCTL_MSM_GEM_VM_BIND to issue
unmap and map operations against the global VM?

This seems to allow unmapping kernel structures like the memptrs ringbuffer
and mapping arbitrary memory in its place, potentially hijacking the GPU
ringbuffer and compromising the kernel. Furthermore, passing offset != 0 or
handle == 0 could trigger a BUG_ON() causing a kernel panic.

>  			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: High]
This isn't a bug introduced by this patch, but is there a race condition
here because the check of ctx->vm and assignment to ctx->userspace_managed_vm
are done without synchronization?

Could a concurrent ioctl (like MSM_GEM_SUBMIT) invoke msm_context_vm(),
which reads ctx->userspace_managed_vm before it is set to true here, causing
it to lazily create a legacy kernel-managed private VM?

drivers/gpu/drm/msm/msm_drv.c:msm_context_vm() {
	...
	vm = msm_gpu_create_private_vm(
		priv->gpu, current, !ctx->userspace_managed_vm);
	...
}

If adreno_set_param() then overwrites ctx->userspace_managed_vm to true,
and userspace subsequently issues DRM_IOCTL_MSM_GEM_VM_BIND, would the
ioctl proceed on the kernel-managed VM?

During the bind operation, wouldn't msm_gem_vma_new() see
vm->managed == true and execute BUG_ON() if offset != 0 or handle == 0,
triggering a kernel panic?

>  
>  		return 0;
>  	default:

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