[PATCH v7 00/17] drm/msm: A couple lazy-vm fixes

[PATCH v7 00/17] 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.  Or at least that is how this series started out.

Rob Clark (17):
  drm/msm: Fix barriers accessing ctx vm
  drm/msm: Rework queuelock
  drm/msm: Synchronize VM creation on ctxlock
  drm/msm: Synchronize set_sysprof on ctxlock
  drm/msm: Move nr_cmds initialization
  drm/msm: Remove redundant SIZE_MAX check
  drm/msm/a6xx: Access VM directly in submit path
  drm/msm: Add helper to check for per-process pgtables VM
  drm/msm/gem: Fix dma_buf import error paths
  drm/msm/gem: Remove useless locking in GEM import
  drm/msm/gem: Extract bookkeeping init helper
  drm/msm/gem: Set resv before exposing obj
  drm/msm/gem: Validate lazy VM in GEM_NEW
  drm/msm: Allow lazy VM creation to fail
  drm/msm: Don't fallback to shared VM for VM_BIND
  drm/msm: Fix per-process-pgtables check
  drm/msm: Fixup invalid overflow check

 drivers/gpu/drm/msm/adreno/a6xx_gmu.c    |  2 +-
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c    |  2 +-
 drivers/gpu/drm/msm/adreno/adreno_gpu.c  | 17 ++++-
 drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c |  2 +-
 drivers/gpu/drm/msm/msm_drv.c            | 26 ++++---
 drivers/gpu/drm/msm/msm_fb.c             |  4 +-
 drivers/gpu/drm/msm/msm_gem.c            | 94 ++++++++++++++----------
 drivers/gpu/drm/msm/msm_gem.h            |  5 +-
 drivers/gpu/drm/msm/msm_gem_prime.c      |  2 +-
 drivers/gpu/drm/msm/msm_gem_submit.c     | 31 ++++----
 drivers/gpu/drm/msm/msm_gem_vma.c        | 12 ++-
 drivers/gpu/drm/msm/msm_gpu.c            |  2 +-
 drivers/gpu/drm/msm/msm_gpu.h            |  4 +-
 drivers/gpu/drm/msm/msm_submitqueue.c    | 24 +++---
 14 files changed, 131 insertions(+), 96 deletions(-)

-- 
2.55.0

[PATCH v7 01/17] 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>
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 v7 01/17] drm/msm: Fix barriers accessing ctx vm

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Directly accessing `ctx->vm` in ioctl entry points before ensuring the VM is initialized via `msm_context_vm()` causes a NULL pointer dereference on the first operation.
- [Critical] `msm_context_vm()` unconditionally caches an `ERR_PTR` inside `ctx->vm` upon allocation failure, permanently breaking the context and causing crashes on subsequent accesses.
- [Critical] Callers of `msm_context_vm()` fail to check if the returned value is an error pointer, leading to a crash on VM creation failure.
- [High] The patch misses concurrent readers like `msm_gem_close()` that still perform plain reads of `ctx->vm`, leaving them vulnerable to data races and partially initialized VM states.
--

--- Patch [1]: [PATCH v7 01/17] drm/msm: Fix barriers accessing ctx vm ---
Commit SHA: 8c0550637c7f990d073da2dbc73f127555883df0
Author: Rob Clark <robin.clark@oss.qualcomm.com>
Subject: drm/msm: Fix barriers accessing ctx vm

This commit updates msm_context_vm() to use proper memory barriers
(smp_load_acquire and smp_store_release) to prevent callers from observing
a partially initialized VM.

> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index ac86b427c0e53..29075aafc5cc5 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);

[Severity: Critical]
This is a pre-existing issue, but I noticed that msm_gpu_create_private_vm()
can return an ERR_PTR on failure (e.g., if page table allocation fails). 

If we store that ERR_PTR into ctx->vm without an IS_ERR() check, will 
future fast-path readers see this truthy pointer and attempt to dereference 
the error value as a valid VM?

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

[Severity: Critical]
This is also a pre-existing issue, but does submit_create() need to check 
the return value of msm_context_vm() before assigning it to submit->vm?

drivers/gpu/drm/msm/msm_gem_submit.c:submit_create() {
	...
	submit->dev = dev;
	submit->vm = msm_context_vm(dev, queue->ctx);
	submit->gpu = gpu;
	...
}

If it returns an ERR_PTR, it will eventually cause a panic when submit->vm 
is dereferenced.

Similarly, in msm_submitqueue_create():

drivers/gpu/drm/msm/msm_submitqueue.c:msm_submitqueue_create() {
	...
	if (flags & MSM_SUBMITQUEUE_VM_BIND) {
		struct drm_gpu_scheduler *sched =
			&to_msm_vm(msm_context_vm(drm, ctx))->sched;

		queue->entity = &queue->_vm_bind_entity[0];
	...
}

Does this code risk a crash by passing an unchecked pointer to to_msm_vm(), 
which computes an offset from the error pointer and then accesses sched?


[Severity: Critical]
This isn't a bug introduced by this patch, but I noticed some paths directly 
access ctx->vm before it's guaranteed to be initialized. For example:

drivers/gpu/drm/msm/msm_gem_submit.c:msm_ioctl_gem_submit() {
	...
	if (args->pad)
		return -EINVAL;

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

If an unprivileged user calls this ioctl as the very first operation on a
new context, won't ctx->vm still be NULL here, leading to an invalid
pointer dereference via the to_msm_vm() offset macro?

I noticed a similar pattern in msm_gem_new_handle():

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

		drm_gem_object_get(r_obj);
	...
}


[Severity: High]
This isn't a bug introduced by this patch, but since the patch addresses 
concurrent access to ctx->vm, does msm_gem_close() also need to use 
smp_load_acquire() instead of a plain read?

drivers/gpu/drm/msm/msm_gem.c:msm_gem_close() {
	...
	msm_gem_vma_put(obj);

	/*
	 * If VM isn't created yet, nothing to cleanup.  And in fact calling
	 * put_iova_spaces() with vm=NULL would be bad, in that it will tear-
	 * down the mappings of shared buffers in other contexts.
	 */
	if (!ctx->vm)
		return;
	...
}

Without a read barrier, could a concurrent ioctl that initializes the VM
cause this function to observe a non-NULL pointer but read uninitialized
memory from the VM structure?

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

[PATCH v7 02/17] drm/msm: Rework queuelock

[Rob Clark]

Rename to ctxlock, and use cleanup guards to manage releasing the lock.
This will let us re-use it for other per-context read/write serial-
ization, such as VM creation.

Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/msm_drv.c         |  2 +-
 drivers/gpu/drm/msm/msm_gpu.h         |  4 ++--
 drivers/gpu/drm/msm/msm_submitqueue.c | 14 +++-----------
 3 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index ec88155e0ed7..384f32f4d0c2 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -251,7 +251,7 @@ static int context_init(struct drm_device *dev, struct drm_file *file)
 		return -ENOMEM;
 
 	INIT_LIST_HEAD(&ctx->submitqueues);
-	rwlock_init(&ctx->queuelock);
+	init_rwsem(&ctx->ctxlock);
 
 	kref_init(&ctx->ref);
 	msm_submitqueue_init(dev, ctx);
diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
index 6c83b8cbbb90..d27d54bdb7a7 100644
--- a/drivers/gpu/drm/msm/msm_gpu.h
+++ b/drivers/gpu/drm/msm/msm_gpu.h
@@ -392,8 +392,8 @@ msm_gpu_sysprof_no_ifpc(struct msm_gpu *gpu)
  * struct msm_context - per-drm_file context
  */
 struct msm_context {
-	/** @queuelock: synchronizes access to submitqueues list */
-	rwlock_t queuelock;
+	/** @ctxlock: synchronizes access to submitqueues list, etc */
+	struct rw_semaphore ctxlock;
 	/** @submitqueues: list of &msm_gpu_submitqueue created by userspace */
 	struct list_head submitqueues;
 	/**
diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c
index 1a5a77b28016..8b5980d5a6a7 100644
--- a/drivers/gpu/drm/msm/msm_submitqueue.c
+++ b/drivers/gpu/drm/msm/msm_submitqueue.c
@@ -93,18 +93,15 @@ struct msm_gpu_submitqueue *msm_submitqueue_get(struct msm_context *ctx,
 	if (!ctx)
 		return NULL;
 
-	read_lock(&ctx->queuelock);
+	guard(rwsem_read)(&ctx->ctxlock);
 
 	list_for_each_entry(entry, &ctx->submitqueues, node) {
 		if (entry->id == id) {
 			kref_get(&entry->ref);
-			read_unlock(&ctx->queuelock);
-
 			return entry;
 		}
 	}
 
-	read_unlock(&ctx->queuelock);
 	return NULL;
 }
 
@@ -237,7 +234,7 @@ int msm_submitqueue_create(struct drm_device *drm, struct msm_context *ctx,
 		return ret;
 	}
 
-	write_lock(&ctx->queuelock);
+	guard(rwsem_write)(&ctx->ctxlock);
 
 	queue->ctx = msm_context_get(ctx);
 	queue->id = ctx->queueid++;
@@ -251,8 +248,6 @@ int msm_submitqueue_create(struct drm_device *drm, struct msm_context *ctx,
 
 	list_add_tail(&queue->node, &ctx->submitqueues);
 
-	write_unlock(&ctx->queuelock);
-
 	return 0;
 }
 
@@ -335,19 +330,16 @@ int msm_submitqueue_remove(struct msm_context *ctx, u32 id)
 	if (!id)
 		return -ENOENT;
 
-	write_lock(&ctx->queuelock);
+	guard(rwsem_write)(&ctx->ctxlock);
 
 	list_for_each_entry(entry, &ctx->submitqueues, node) {
 		if (entry->id == id) {
 			list_del(&entry->node);
-			write_unlock(&ctx->queuelock);
-
 			msm_submitqueue_put(entry);
 			return 0;
 		}
 	}
 
-	write_unlock(&ctx->queuelock);
 	return -ENOENT;
 }
 
-- 
2.55.0

Re: [PATCH v7 02/17] drm/msm: Rework queuelock

[sashiko-bot]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the return value of `drm_sched_entity_init()` is ignored when creating a VM_BIND submitqueue, which can lead to a kernel crash if the allocation fails.
--

--- Patch [2]: [PATCH v7 02/17] drm/msm: Rework queuelock ---
commit 1fecfbcd368f21f80f7078762ebd5da9fae4c122
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Rework queuelock

This commit renames queuelock to ctxlock and transitions it to use cleanup
guards. The change is intended to allow the lock to be reused for other
per-context read/write serialization tasks such as VM creation.

> diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c
> index 1a5a77b280163..8b5980d5a6a79 100644
> --- a/drivers/gpu/drm/msm/msm_submitqueue.c
> +++ b/drivers/gpu/drm/msm/msm_submitqueue.c

[ ... ]

> @@ -237,7 +234,7 @@ int msm_submitqueue_create(struct drm_device *drm, struct msm_context *ctx,
>  		return ret;
>  	}
>  
> -	write_lock(&ctx->queuelock);
> +	guard(rwsem_write)(&ctx->ctxlock);

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential bug in
the VM_BIND queue initialization just above this locking change in
msm_submitqueue_create().

Does this code fail to catch allocation errors from drm_sched_entity_init()?

	if (flags & MSM_SUBMITQUEUE_VM_BIND) {
		...
		drm_sched_entity_init(queue->entity, DRM_SCHED_PRIORITY_KERNEL,
				      &sched, 1, NULL);
	} else {
		...
	}

	if (IS_ERR(queue->entity)) {
		ret = PTR_ERR(queue->entity);
		...

If drm_sched_entity_init() fails, it returns an error but the return value is
silently ignored. The subsequent check for IS_ERR(queue->entity) will evaluate
to false because queue->entity points to a valid inline struct array element
(&queue->_vm_bind_entity[0]).

Could this result in a NULL pointer dereference later on if a job is submitted
to this partially initialized scheduler entity?

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

[PATCH v7 03/17] drm/msm: Synchronize VM creation on ctxlock

[Rob Clark]

And serialize setting EN_VM_BIND against VM creation.

Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 5 ++++-
 drivers/gpu/drm/msm/msm_drv.c           | 3 +--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index c62c45bb0ddb..0d54141ff089 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)
 			return UERR(EINVAL, drm, "requires per-process pgtables");
@@ -518,6 +520,7 @@ int adreno_set_param(struct msm_gpu *gpu, struct msm_context *ctx,
 		ctx->userspace_managed_vm = value;
 
 		return 0;
+	}
 	default:
 		return UERR(EINVAL, drm, "%s: invalid param: %u", gpu->name, param);
 	}
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 384f32f4d0c2..2fd1b717d8e6 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -222,7 +222,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);
 
@@ -230,7 +229,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(
-- 
2.55.0

Re: [PATCH v7 03/17] drm/msm: Synchronize VM creation on ctxlock

[sashiko-bot]

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

Pre-existing issues:
- [Critical] `msm_context_vm()` returns `NULL` when VM creation fails, leading to unhandled NULL pointer dereference in callers.
- [Critical] Flawed hardware capability check in `adreno_set_param()` allows VM_BIND on unsupported hardware.
- [High] Missing error check for `drm_sched_entity_init()` leaves the scheduler entity uninitialized.
--

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

drm/msm: Synchronize VM creation on ctxlock

This commit replaces the global init_lock mutex with a per-context
rw_semaphore to synchronize VM creation and serialize setting EN_VM_BIND.

> 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)
> -			return UERR(EINVAL, drm, "requires per-process pgtables");

[Severity: Critical]
This isn't a bug introduced by this patch, but does this hardware capability
check evaluate correctly before VM creation?

Before this change, the check if (ctx->vm == gpu->vm) evaluates to false
because ctx->vm is NULL before lazy VM creation. This allows an unprivileged
user to successfully set ctx->userspace_managed_vm on hardware lacking
create_private_vm support. 

When the VM is subsequently created, it returns NULL, and the caller crashes.

[ ... ]
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 77681cb4fdc25..a5714e24fcc6c 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
[ ... ]
> @@ -230,7 +229,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 isn't a bug introduced by this patch, but does msm_context_vm()
properly propagate errors if VM creation fails?

If msm_gpu_create_private_vm() fails (e.g., due to memory exhaustion) and
returns an error pointer, msm_context_vm() does not populate ctx->vm and
returns NULL.

Callers like msm_submitqueue_create() unconditionally cast the return value
using to_msm_vm() and dereference its ->sched member, which can cause a
kernel panic.

I also noticed a pre-existing issue in msm_submitqueue_create() in
drivers/gpu/drm/msm/msm_submitqueue.c:

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to ignore the
return value of drm_sched_entity_init()?

In msm_submitqueue_create(), the return value of drm_sched_entity_init() is
ignored:

drm_sched_entity_init(queue->entity, DRM_SCHED_PRIORITY_KERNEL,
                      &sched, 1, NULL);

If initialization fails, the function continues. The subsequent error check
if (IS_ERR(queue->entity)) fails to detect the error because queue->entity
points to an embedded array element, not an ERR_PTR. 

This could lead to using an uninitialized scheduler entity, potentially
causing memory corruption or crashes.

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

[PATCH v7 04/17] drm/msm: Synchronize set_sysprof on ctxlock

[Rob Clark]

A user that was perfmon_capable() could try to race setting SYSPROF
param on multiple threads to trigger a reference leak.

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

diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c
index 8b5980d5a6a7..233c4f35ab12 100644
--- a/drivers/gpu/drm/msm/msm_submitqueue.c
+++ b/drivers/gpu/drm/msm/msm_submitqueue.c
@@ -9,6 +9,8 @@
 
 int msm_context_set_sysprof(struct msm_context *ctx, struct msm_gpu *gpu, int sysprof)
 {
+	guard(rwsem_write)(&ctx->ctxlock);
+
 	/*
 	 * Since pm_runtime and sysprof_active are both refcounts, we
 	 * call apply the new value first, and then unwind the previous
-- 
2.55.0

Re: [PATCH v7 04/17] drm/msm: Synchronize set_sysprof on ctxlock

[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` allows userspace to enable VM_BIND on hardware lacking per-process page tables.
--

--- Patch [4]: [PATCH v7 04/17] drm/msm: Synchronize set_sysprof on ctxlock ---
commit 80ad21c966f9ce52663d9bf52f28781c4e97e7e0
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Synchronize set_sysprof on ctxlock

A user that was perfmon_capable() could try to race setting SYSPROF
param on multiple threads to trigger a reference leak.

> diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c
> index 8b5980d5a6a79..233c4f35ab121 100644
> --- a/drivers/gpu/drm/msm/msm_submitqueue.c
> +++ b/drivers/gpu/drm/msm/msm_submitqueue.c
> @@ -9,6 +9,8 @@
>  
>  int msm_context_set_sysprof(struct msm_context *ctx, struct msm_gpu *gpu, int sysprof)
>  {
> +	guard(rwsem_write)(&ctx->ctxlock);
> +
>  	/*
>  	 * Since pm_runtime and sysprof_active are both refcounts, we
>  	 * call apply the new value first, and then unwind the previous

[Severity: High]
This isn't a bug introduced by this patch, but there is a pre-existing issue in
adreno_set_param() when handling MSM_PARAM_EN_VM_BIND:

drivers/gpu/drm/msm/adreno/adreno_gpu.c:adreno_set_param() {
    ...
	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)
			return UERR(EINVAL, drm, "requires per-process pgtables");
    ...
}

Does this direct comparison against an uninitialized (NULL) ctx->vm fail to
reject the VM_BIND enablement on GPUs without per-process page tables?

If a userspace program calls the MSM_PARAM_EN_VM_BIND ioctl early, ctx->vm is
NULL due to lazy initialization. The check if (ctx->vm == gpu->vm) in
adreno_set_param() evaluates to false, bypassing the hardware capability
check and incorrectly setting ctx->userspace_managed_vm = true.

Later, when msm_context_vm() is called, msm_gpu_create_private_vm() returns
the global gpu->vm because the hardware lacks support for private VMs.
Userspace can then use the MSM_VM_BIND ioctl to map memory directly in the
global GPU VM.

Could a malicious userspace program exploit this to map and unmap arbitrary
memory in the global GPU page tables, potentially overwriting mappings of
other contexts and the kernel?

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

[PATCH v7 05/17] drm/msm: Move nr_cmds initialization

[Rob Clark]

Previously if we entered an error path between these two points, we
could leak the relocs tables due to submit->nr_cmds still being zero.

In practice, relocs are disallowed on a6xx+, and non-ancient userspace
will not use relocs on earlier gens unless running on an ancient kernel.
But userspace could use this to trigger a memory leak.

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 | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index 3c6bc90c3d48..aa48ea4e7f58 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -260,6 +260,9 @@ static int submit_lookup_cmds(struct msm_gem_submit *submit,
 			ret = -ENOMEM;
 			goto out;
 		}
+
+		submit->nr_cmds = i + 1;
+
 		ret = copy_from_user(submit->cmd[i].relocs, userptr, sz);
 		if (ret) {
 			ret = -EFAULT;
@@ -719,8 +722,6 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
 			goto out;
 	}
 
-	submit->nr_cmds = args->nr_cmds;
-
 	idr_preload(GFP_KERNEL);
 
 	spin_lock(&queue->idr_lock);
-- 
2.55.0

Re: [PATCH v7 05/17] drm/msm: Move nr_cmds initialization

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Direct access to lazy-initialized `ctx->vm` without ensuring it exists causes a NULL pointer dereference.
- [Critical] The overflow check `sz > SIZE_MAX` after using `struct_size()` is dead code on 64-bit architectures, providing no protection against integer overflows.
--

--- Patch [5]: [PATCH v7 05/17] drm/msm: Move nr_cmds initialization ---
commit cc137d18d60eae340c7d4959cae7bc281443d450
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Move nr_cmds initialization

Previously if we entered an error path between these two points, we
could leak the relocs tables due to submit->nr_cmds still being zero.

In practice, relocs are disallowed on a6xx+, and non-ancient userspace
will not use relocs on earlier gens unless running on an ancient kernel.
But userspace could use this to trigger a memory leak.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>

> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> index 3c6bc90c3d486..aa48ea4e7f584 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c

[Severity: Critical]
This is a pre-existing issue, but does submit_create() have a dead code
overflow check?

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

On 64-bit architectures, sz (a uint64_t) cannot exceed SIZE_MAX. If
struct_size() overflows, it returns SIZE_MAX, and adding to it wraps
around to a small value.

This bypasses the size check and causes an undersized allocation, which
could lead to a heap buffer overflow since nr_bos and nr_cmds are
user-controlled via the MSM_GEM_SUBMIT ioctl.

[Severity: Critical]
This is a pre-existing issue, but does msm_ioctl_gem_submit() dereference
an uninitialized pointer?

In 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 uses lazy initialization, it can be NULL if the DRM context
was just created.

Accessing ctx->vm without calling msm_context_vm() first can lead to a
NULL pointer dereference if a userspace application calls the
MSM_GEM_SUBMIT ioctl immediately after context creation.

[ ... ]

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

[PATCH v7 06/17] drm/msm: Remove redundant SIZE_MAX check

[Rob Clark]

kmalloc() will already fail and return NULL if passed SIZE_MAX.

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

diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index aa48ea4e7f58..040354ee16d1 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -250,11 +250,6 @@ static int submit_lookup_cmds(struct msm_gem_submit *submit,
 
 		sz = array_size(submit_cmd.nr_relocs,
 				sizeof(struct drm_msm_gem_submit_reloc));
-		/* check for overflow: */
-		if (sz == SIZE_MAX) {
-			ret = -ENOMEM;
-			goto out;
-		}
 		submit->cmd[i].relocs = kmalloc(sz, GFP_KERNEL | __GFP_NOWARN);
 		if (!submit->cmd[i].relocs) {
 			ret = -ENOMEM;
-- 
2.55.0

Re: [PATCH v7 06/17] drm/msm: Remove redundant SIZE_MAX check

[sashiko-bot]

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

Pre-existing issues:
- [High] NULL pointer dereference from direct access to `ctx->vm` before VM initialization.
--

--- Patch [6]: [PATCH v7 06/17] drm/msm: Remove redundant SIZE_MAX check ---
commit 704799112fb8f5dd4e2424f4cac69e1d4b373f9d
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Remove redundant SIZE_MAX check

This commit removes a redundant SIZE_MAX overflow check in submit_lookup_cmds().
Since kmalloc() will inherently fail and return NULL when passed SIZE_MAX,
the explicit check is unnecessary.

> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> index aa48ea4e7f584..040354ee16d11 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -250,11 +250,6 @@ static int submit_lookup_cmds(struct msm_gem_submit *submit,
>  
>  		sz = array_size(submit_cmd.nr_relocs,
>  				sizeof(struct drm_msm_gem_submit_reloc));
> -		/* check for overflow: */
> -		if (sz == SIZE_MAX) {
> -			ret = -ENOMEM;
> -			goto out;
> -		}
>  		submit->cmd[i].relocs = kmalloc(sz, GFP_KERNEL | __GFP_NOWARN);
>  		if (!submit->cmd[i].relocs) {
>  			ret = -ENOMEM;

[Severity: High]
This is a pre-existing issue, but I noticed it while reviewing this file.
Can this code cause a NULL pointer dereference if a local user opens a DRM
context and immediately calls the GEM submit ioctl before any VM has been
created for that context?

Looking at msm_ioctl_gem_submit() in the same file:

drivers/gpu/drm/msm/msm_gem_submit.c:msm_ioctl_gem_submit() {
    ...
	if (args->pad)
		return -EINVAL;

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

The to_msm_vm() macro performs a container_of() calculation, which on a NULL
ctx->vm yields an invalid pointer offset. Dereferencing ->unusable on this
invalid pointer will cause a page fault.

Should this use msm_context_vm(dev, ctx) instead of accessing ctx->vm
directly to ensure the VM is properly initialized before checking its
unusable state?

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

[PATCH v7 07/17] drm/msm/a6xx: Access VM directly in submit path

[Rob Clark]

The GEM_SUBMIT ioctl has already ensured that the VM is created, so we
aren't expecting to lazily create the VM this deep into the ioctl.

Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 8b3bb2fd433b..7f1673163f67 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -229,7 +229,7 @@ static void a6xx_set_pagetable(struct a6xx_gpu *a6xx_gpu,
 {
 	bool sysprof = msm_gpu_sysprof_no_perfcntr_zap(&a6xx_gpu->base.base);
 	struct msm_context *ctx = submit->queue->ctx;
-	struct drm_gpuvm *vm = msm_context_vm(submit->dev, ctx);
+	struct drm_gpuvm *vm = ctx->vm;
 	struct adreno_gpu *adreno_gpu = &a6xx_gpu->base;
 	phys_addr_t ttbr;
 	u32 asid;
-- 
2.55.0

Re: [PATCH v7 07/17] drm/msm/a6xx: Access VM directly in submit path

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Direct access to ctx->vm in ioctl entry point without preceding call to msm_context_vm() causes NULL pointer dereference.
- [High] Unchecked ERR_PTR assignment from msm_context_vm() leads to kernel crash in submit_lock_objects().
--

--- Patch [7]: [PATCH v7 07/17] drm/msm/a6xx: Access VM directly in submit path ---
commit fb968654323d1792a0d26ac37c9d0a8e1d71c1dd
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm/a6xx: Access VM directly in submit path

The GEM_SUBMIT ioctl has already ensured that the VM is created, so we
aren't expecting to lazily create the VM this deep into the ioctl.

> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index e293b4ca808a4..9240c4af687c9 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -229,7 +229,7 @@ static void a6xx_set_pagetable(struct a6xx_gpu *a6xx_gpu,
>  {
>  	bool sysprof = msm_gpu_sysprof_no_perfcntr_zap(&a6xx_gpu->base.base);
>  	struct msm_context *ctx = submit->queue->ctx;
> -	struct drm_gpuvm *vm = msm_context_vm(submit->dev, ctx);
> +	struct drm_gpuvm *vm = ctx->vm;

[Severity: Critical]
This is a pre-existing issue, but since the commit message mentions that
the GEM_SUBMIT ioctl has already ensured that the VM is created, I wanted to
ask about msm_ioctl_gem_submit().

Looking at msm_ioctl_gem_submit() in drivers/gpu/drm/msm/msm_gem_submit.c, it
accesses ctx->vm directly:

drivers/gpu/drm/msm/msm_gem_submit.c:msm_ioctl_gem_submit() {
    ...
	if (args->pad)
		return -EINVAL;

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

Can this cause a NULL pointer dereference if an unprivileged user creates a
legacy submit queue without VM_BIND and immediately calls the submit ioctl?
The VM is initialized lazily, so it seems ctx->vm could be NULL here if we
haven't called msm_context_vm() first.

[Severity: High]
This is another pre-existing issue, but I wanted to check how we handle VM
allocation failures in the submit path.

In submit_create() in drivers/gpu/drm/msm/msm_gem_submit.c:

drivers/gpu/drm/msm/msm_gem_submit.c:submit_create() {
    ...
	submit->vm = msm_context_vm(dev, queue->ctx);
    ...
}

msm_context_vm() can return an ERR_PTR(-ENOMEM). Later in the same ioctl,
submit_lock_objects() unconditionally dereferences it:

drivers/gpu/drm/msm/msm_gem_submit.c:submit_lock_objects() {
    ...
	drm_exec_until_all_locked (&submit->exec) {
		ret = drm_exec_lock_obj(&submit->exec,
					drm_gpuvm_resv_obj(submit->vm));
    ...
}

Does submit->vm need an IS_ERR() check after calling msm_context_vm() to
prevent a kernel crash if the VM allocation fails?

>  	struct adreno_gpu *adreno_gpu = &a6xx_gpu->base;
>  	phys_addr_t ttbr;
>  	u32 asid;

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

[PATCH v7 08/17] drm/msm: Add helper to check for per-process pgtables VM

[Rob Clark]

This will simplify a following commit to allow lazy VM creation to fail.

Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 0d54141ff089..5738eac9aebd 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -356,6 +356,12 @@ int adreno_fault_handler(struct msm_gpu *gpu, unsigned long iova, int flags,
 	return 0;
 }
 
+static bool
+valid_per_process_vm(struct msm_gpu *gpu, struct drm_gpuvm *vm)
+{
+	return (vm != gpu->vm);
+}
+
 int adreno_get_param(struct msm_gpu *gpu, struct msm_context *ctx,
 		     uint32_t param, uint64_t *value, uint32_t *len)
 {
@@ -414,12 +420,12 @@ int adreno_get_param(struct msm_gpu *gpu, struct msm_context *ctx,
 		*value = gpu->suspend_count;
 		return 0;
 	case MSM_PARAM_VA_START:
-		if (vm == gpu->vm)
+		if (!valid_per_process_vm(gpu, vm))
 			return UERR(EINVAL, drm, "requires per-process pgtables");
 		*value = vm->mm_start;
 		return 0;
 	case MSM_PARAM_VA_SIZE:
-		if (vm == gpu->vm)
+		if (!valid_per_process_vm(gpu, vm))
 			return UERR(EINVAL, drm, "requires per-process pgtables");
 		*value = vm->mm_range;
 		return 0;
-- 
2.55.0

Re: [PATCH v7 08/17] drm/msm: Add helper to check for per-process pgtables 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 `ctx->vm` in `adreno_set_param()` bypasses capability checks for per-process pgtables, allowing userspace to enable VM_BIND on unsupported hardware and crash the kernel.
--

--- Patch [8]: [PATCH v7 08/17] drm/msm: Add helper to check for per-process pgtables VM ---
commit c61563d7d541358abb7f78356a4f5b717a237572
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Add helper to check for per-process pgtables VM

This patch adds a helper function to validate whether a GPU VM supports
per-process pagetables. It then updates adreno_get_param() to use this
new helper.

> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index 0d54141ff089c..5738eac9aebd6 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -356,6 +356,12 @@ int adreno_fault_handler(struct msm_gpu *gpu, unsigned long iova, int flags,
>  	return 0;
>  }
>  
> +static bool
> +valid_per_process_vm(struct msm_gpu *gpu, struct drm_gpuvm *vm)
> +{
> +	return (vm != gpu->vm);
> +}
> +
>  int adreno_get_param(struct msm_gpu *gpu, struct msm_context *ctx,
>  		     uint32_t param, uint64_t *value, uint32_t *len)
>  {
> @@ -414,12 +420,12 @@ int adreno_get_param(struct msm_gpu *gpu, struct msm_context *ctx,
>  		*value = gpu->suspend_count;
>  		return 0;
>  	case MSM_PARAM_VA_START:
> -		if (vm == gpu->vm)
> +		if (!valid_per_process_vm(gpu, vm))
>  			return UERR(EINVAL, drm, "requires per-process pgtables");
>  		*value = vm->mm_start;
>  		return 0;
>  	case MSM_PARAM_VA_SIZE:
> -		if (vm == gpu->vm)
> +		if (!valid_per_process_vm(gpu, vm))
>  			return UERR(EINVAL, drm, "requires per-process pgtables");
>  		*value = vm->mm_range;
>  		return 0;

[Severity: High]
This isn't a bug introduced by this patch, but while you are updating the
VM checks here, should the same logic be applied to adreno_set_param()?

Looking at adreno_set_param() for MSM_PARAM_EN_VM_BIND, it accesses ctx->vm
directly:

drivers/gpu/drm/msm/adreno/adreno_gpu.c:adreno_set_param() {
    ...
	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)
			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");
    ...
}

Because msm uses lazy initialization for virtual memory, isn't ctx->vm
guaranteed to be NULL here prior to creation? 

If ctx->vm is NULL, then ctx->vm == gpu->vm evaluates to false, which bypasses
the capability check intended to prevent VM_BIND on platforms lacking
per-process pgtables.

Could this allow userspace to enable VM_BIND on older hardware and subsequently
crash the kernel when it binds to an uninitialized scheduler on the global VM?

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

[PATCH v7 09/17] drm/msm/gem: Fix dma_buf import error paths

[Rob Clark]

Set import_attach early, so that if we hit an error path
msm_gem_free_object() goes down the drm_gem_is_imported()
path.

Set sgt late so _free_object() skips drm_prime_gem_destroy()
as this is done by drm_gem_prime_import_dev().

Reported-by: Sashiko <sashiko-bot@kernel.org>
Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/msm_gem.c       | 17 ++++++++++++++---
 drivers/gpu/drm/msm/msm_gem.h       |  3 ++-
 drivers/gpu/drm/msm/msm_gem_prime.c |  2 +-
 3 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index efd3d3c9a449..74c3728f45bd 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -1093,7 +1093,9 @@ static void msm_gem_free_object(struct drm_gem_object *obj)
 		 */
 		kvfree(msm_obj->pages);
 
-		drm_prime_gem_destroy(obj, msm_obj->sgt);
+		/* In msm_gem_import() error path, sgt won't be set yet: */
+		if (msm_obj->sgt)
+			drm_prime_gem_destroy(obj, msm_obj->sgt);
 	} else {
 		msm_gem_vunmap(obj);
 		put_pages(obj);
@@ -1282,11 +1284,13 @@ struct drm_gem_object *msm_gem_new(struct drm_device *dev, size_t size, uint32_t
 }
 
 struct drm_gem_object *msm_gem_import(struct drm_device *dev,
-		struct dma_buf *dmabuf, struct sg_table *sgt)
+				      struct dma_buf_attachment *attach,
+				      struct sg_table *sgt)
 {
 	struct msm_drm_private *priv = dev->dev_private;
 	struct msm_gem_object *msm_obj;
 	struct drm_gem_object *obj;
+	struct dma_buf *dmabuf = attach->dmabuf;
 	size_t size, npages;
 	int ret;
 
@@ -1296,13 +1300,17 @@ struct drm_gem_object *msm_gem_import(struct drm_device *dev,
 	if (ret)
 		return ERR_PTR(ret);
 
+	/*
+	 * Set import_attach here in case we hit an error path that ends
+	 * up in drm_gem_object_put() -> msm_gem_free_object()
+	 */
+	obj->import_attach = attach;
 	drm_gem_private_object_init(dev, obj, size);
 
 	npages = size / PAGE_SIZE;
 
 	msm_obj = to_msm_bo(obj);
 	msm_gem_lock(obj);
-	msm_obj->sgt = sgt;
 	msm_obj->pages = kvmalloc_objs(struct page *, npages);
 	if (!msm_obj->pages) {
 		msm_gem_unlock(obj);
@@ -1328,6 +1336,9 @@ struct drm_gem_object *msm_gem_import(struct drm_device *dev,
 	if (ret)
 		goto fail;
 
+	/* Now that we are past potential failure points, set sgt: */
+	msm_obj->sgt = sgt;
+
 	return obj;
 
 fail:
diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
index 762e546d25ef..86dc93cc1804 100644
--- a/drivers/gpu/drm/msm/msm_gem.h
+++ b/drivers/gpu/drm/msm/msm_gem.h
@@ -302,7 +302,8 @@ void *msm_gem_kernel_new(struct drm_device *dev, size_t size, uint32_t flags,
 			 uint64_t *iova);
 void msm_gem_kernel_put(struct drm_gem_object *bo, struct drm_gpuvm *vm);
 struct drm_gem_object *msm_gem_import(struct drm_device *dev,
-		struct dma_buf *dmabuf, struct sg_table *sgt);
+				      struct dma_buf_attachment *attach,
+				      struct sg_table *sgt);
 __printf(2, 3)
 void msm_gem_object_set_name(struct drm_gem_object *bo, const char *fmt, ...);
 
diff --git a/drivers/gpu/drm/msm/msm_gem_prime.c b/drivers/gpu/drm/msm/msm_gem_prime.c
index 036d34c674d9..beb7f22fd694 100644
--- a/drivers/gpu/drm/msm/msm_gem_prime.c
+++ b/drivers/gpu/drm/msm/msm_gem_prime.c
@@ -83,7 +83,7 @@ struct drm_gem_object *msm_gem_prime_import(struct drm_device *dev,
 struct drm_gem_object *msm_gem_prime_import_sg_table(struct drm_device *dev,
 		struct dma_buf_attachment *attach, struct sg_table *sg)
 {
-	return msm_gem_import(dev, attach->dmabuf, sg);
+	return msm_gem_import(dev, attach, sg);
 }
 
 struct dma_buf *msm_gem_prime_export(struct drm_gem_object *obj, int flags)
-- 
2.55.0

[PATCH v7 10/17] drm/msm/gem: Remove useless locking in GEM import

[Rob Clark]

The locking has changed a few times over the years, and this extra
locking was the mistake of evolution.  Harmless but useless.

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

diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 74c3728f45bd..cc45392691e5 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -1310,22 +1310,17 @@ struct drm_gem_object *msm_gem_import(struct drm_device *dev,
 	npages = size / PAGE_SIZE;
 
 	msm_obj = to_msm_bo(obj);
-	msm_gem_lock(obj);
 	msm_obj->pages = kvmalloc_objs(struct page *, npages);
 	if (!msm_obj->pages) {
-		msm_gem_unlock(obj);
 		ret = -ENOMEM;
 		goto fail;
 	}
 
 	ret = drm_prime_sg_to_page_array(sgt, msm_obj->pages, npages);
 	if (ret) {
-		msm_gem_unlock(obj);
 		goto fail;
 	}
 
-	msm_gem_unlock(obj);
-
 	drm_gem_lru_move_tail(&priv->lru.pinned, obj);
 
 	mutex_lock(&priv->obj_lock);
-- 
2.55.0

Re: [PATCH v7 10/17] drm/msm/gem: Remove useless locking in GEM import

[sashiko-bot]

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

Pre-existing issues:
- [High] Concurrent access to imported GEM objects via debugfs causes a lock imbalance, leading to system crash or permanent resource lock.
--

--- Patch [10]: [PATCH v7 10/17] drm/msm/gem: Remove useless locking in GEM import ---
commit 1dfbce02f7920b37d011abac3bc5352c1cadab22
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm/gem: Remove useless locking in GEM import
    
The locking has changed a few times over the years, and this extra
locking was the mistake of evolution.  Harmless but useless.

> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index 74c3728f45bd0..cc45392691e50 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
[ ... ]
> @@ -1310,22 +1310,17 @@ struct drm_gem_object *msm_gem_import(struct drm_device *dev,
[ ... ]
>  	drm_gem_lru_move_tail(&priv->lru.pinned, obj);
>  
>  	mutex_lock(&priv->obj_lock);

[Severity: High]
This is a pre-existing issue, but can publishing the object to the global
list here lead to a lock imbalance if debugfs is accessed concurrently?

Once the object is added to the priv->objects list, it becomes visible to
concurrent debugfs iteration before the import process has finished.

According to drm_prime.c, drm_gem_prime_import_dev() overwrites obj->resv
after msm_gem_import() has already published it:

	obj->import_attach = attach;
	obj->resv = dma_buf->resv;

If msm_gem_describe() in debugfs iterates the list concurrently during this
window, it locks the original obj->resv via msm_gem_trylock(obj). If the
resv pointer is then overwritten mid-flight, the subsequent
msm_gem_unlock(obj) call will unlock a completely different lock:

	if (!msm_gem_trylock(obj))
		return;
	...
	dma_resv_describe(robj, m);
	msm_gem_unlock(obj);

Could this lead to a kernel oops due to a bad unlock balance, or leave the
initial reservation object permanently locked?

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

[PATCH v7 11/17] drm/msm/gem: Extract bookkeeping init helper

[Rob Clark]

Clean up duplicated logic between import and new paths.

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

diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index cc45392691e5..3c36b1c4a4d5 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -1234,10 +1234,25 @@ static int msm_gem_new_impl(struct drm_device *dev, uint32_t flags,
 	return 0;
 }
 
+static int msm_gem_init_bookkeeping(struct drm_gem_object *obj)
+{
+	struct msm_drm_private *priv = obj->dev->dev_private;
+
+	if (drm_gem_is_imported(obj)) {
+		drm_gem_lru_move_tail(&priv->lru.pinned, obj);
+	} else {
+		drm_gem_lru_move_tail(&priv->lru.unbacked, obj);
+	}
+
+	mutex_lock(&priv->obj_lock);
+	list_add_tail(&to_msm_bo(obj)->node, &priv->objects);
+	mutex_unlock(&priv->obj_lock);
+
+	return drm_gem_create_mmap_offset(obj);
+}
+
 struct drm_gem_object *msm_gem_new(struct drm_device *dev, size_t size, uint32_t flags)
 {
-	struct msm_drm_private *priv = dev->dev_private;
-	struct msm_gem_object *msm_obj;
 	struct drm_gem_object *obj = NULL;
 	int ret;
 
@@ -1253,8 +1268,6 @@ struct drm_gem_object *msm_gem_new(struct drm_device *dev, size_t size, uint32_t
 	if (ret)
 		return ERR_PTR(ret);
 
-	msm_obj = to_msm_bo(obj);
-
 	ret = drm_gem_object_init(dev, obj, size);
 	if (ret)
 		goto fail;
@@ -1266,13 +1279,7 @@ struct drm_gem_object *msm_gem_new(struct drm_device *dev, size_t size, uint32_t
 	 */
 	mapping_set_gfp_mask(obj->filp->f_mapping, GFP_HIGHUSER);
 
-	drm_gem_lru_move_tail(&priv->lru.unbacked, obj);
-
-	mutex_lock(&priv->obj_lock);
-	list_add_tail(&msm_obj->node, &priv->objects);
-	mutex_unlock(&priv->obj_lock);
-
-	ret = drm_gem_create_mmap_offset(obj);
+	ret = msm_gem_init_bookkeeping(obj);
 	if (ret)
 		goto fail;
 
@@ -1287,7 +1294,6 @@ struct drm_gem_object *msm_gem_import(struct drm_device *dev,
 				      struct dma_buf_attachment *attach,
 				      struct sg_table *sgt)
 {
-	struct msm_drm_private *priv = dev->dev_private;
 	struct msm_gem_object *msm_obj;
 	struct drm_gem_object *obj;
 	struct dma_buf *dmabuf = attach->dmabuf;
@@ -1321,13 +1327,7 @@ struct drm_gem_object *msm_gem_import(struct drm_device *dev,
 		goto fail;
 	}
 
-	drm_gem_lru_move_tail(&priv->lru.pinned, obj);
-
-	mutex_lock(&priv->obj_lock);
-	list_add_tail(&msm_obj->node, &priv->objects);
-	mutex_unlock(&priv->obj_lock);
-
-	ret = drm_gem_create_mmap_offset(obj);
+	ret = msm_gem_init_bookkeeping(obj);
 	if (ret)
 		goto fail;
 
-- 
2.55.0

[PATCH v7 12/17] drm/msm/gem: Set resv before exposing obj

[Rob Clark]

Don't swap the resv object _after_ exposing the newly created obj in LRU
or global objects list, as that creates a race condition where another
thread could lock the object using the original (per-obj) resv, but then
unlock after the resv is replaced.

Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/adreno/a6xx_gmu.c    |  2 +-
 drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c |  2 +-
 drivers/gpu/drm/msm/msm_fb.c             |  4 +--
 drivers/gpu/drm/msm/msm_gem.c            | 32 ++++++++++++++----------
 drivers/gpu/drm/msm/msm_gem.h            |  2 +-
 5 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index 2e5d7b53a0c3..9aed142faea1 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -1546,7 +1546,7 @@ static int a6xx_gmu_memory_alloc(struct a6xx_gmu *gmu, struct a6xx_gmu_bo *bo,
 		flags |= MSM_BO_MAP_PRIV;
 	}
 
-	bo->obj = msm_gem_new(dev, size, flags);
+	bo->obj = msm_gem_new(dev, size, flags, NULL);
 	if (IS_ERR(bo->obj))
 		return PTR_ERR(bo->obj);
 
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
index 7726edb0d4ed..c289dff78cd5 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
@@ -463,7 +463,7 @@ static int mdp4_kms_init(struct drm_device *dev)
 		goto fail;
 	}
 
-	mdp4_kms->blank_cursor_bo = msm_gem_new(dev, SZ_16K, MSM_BO_WC | MSM_BO_SCANOUT);
+	mdp4_kms->blank_cursor_bo = msm_gem_new(dev, SZ_16K, MSM_BO_WC | MSM_BO_SCANOUT, NULL);
 	if (IS_ERR(mdp4_kms->blank_cursor_bo)) {
 		ret = PTR_ERR(mdp4_kms->blank_cursor_bo);
 		DRM_DEV_ERROR(dev->dev, "could not allocate blank-cursor bo: %d\n", ret);
diff --git a/drivers/gpu/drm/msm/msm_fb.c b/drivers/gpu/drm/msm/msm_fb.c
index 9b681e144c07..ee17a47f31c4 100644
--- a/drivers/gpu/drm/msm/msm_fb.c
+++ b/drivers/gpu/drm/msm/msm_fb.c
@@ -267,11 +267,11 @@ msm_alloc_stolen_fb(struct drm_device *dev, int w, int h, int p, uint32_t format
 	/* allocate backing bo */
 	size = mode_cmd.pitches[0] * mode_cmd.height;
 	DBG("allocating %d bytes for fb %d", size, dev->primary->index);
-	bo = msm_gem_new(dev, size, MSM_BO_SCANOUT | MSM_BO_WC | MSM_BO_STOLEN);
+	bo = msm_gem_new(dev, size, MSM_BO_SCANOUT | MSM_BO_WC | MSM_BO_STOLEN, NULL);
 	if (IS_ERR(bo)) {
 		dev_warn(dev->dev, "could not allocate stolen bo\n");
 		/* try regular bo: */
-		bo = msm_gem_new(dev, size, MSM_BO_SCANOUT | MSM_BO_WC);
+		bo = msm_gem_new(dev, size, MSM_BO_SCANOUT | MSM_BO_WC, NULL);
 	}
 	if (IS_ERR(bo)) {
 		DRM_DEV_ERROR(dev->dev, "failed to allocate buffer object\n");
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 3c36b1c4a4d5..535c6e1acb3c 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -1136,10 +1136,17 @@ int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
 		size_t size, uint32_t flags, uint32_t *handle,
 		char *name)
 {
-	struct drm_gem_object *obj;
+	struct drm_gem_object *obj, *r_obj = NULL;
 	int ret;
 
-	obj = msm_gem_new(dev, size, flags);
+	if (flags & MSM_BO_NO_SHARE) {
+		struct msm_context *ctx = file->driver_priv;
+		struct drm_gpuvm *vm = msm_context_vm(dev, ctx);
+
+		r_obj = drm_gpuvm_resv_obj(vm);
+	}
+
+	obj = msm_gem_new(dev, size, flags, r_obj);
 
 	if (IS_ERR(obj))
 		return PTR_ERR(obj);
@@ -1147,15 +1154,6 @@ int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
 	if (name)
 		msm_gem_object_set_name(obj, "%s", name);
 
-	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);
-
-		drm_gem_object_get(r_obj);
-
-		obj->resv = r_obj->resv;
-	}
-
 	ret = drm_gem_handle_create(file, obj, handle);
 
 	/* drop reference from allocate - handle holds it now */
@@ -1251,7 +1249,9 @@ static int msm_gem_init_bookkeeping(struct drm_gem_object *obj)
 	return drm_gem_create_mmap_offset(obj);
 }
 
-struct drm_gem_object *msm_gem_new(struct drm_device *dev, size_t size, uint32_t flags)
+struct drm_gem_object *
+msm_gem_new(struct drm_device *dev, size_t size, uint32_t flags,
+	    struct drm_gem_object *r_obj)
 {
 	struct drm_gem_object *obj = NULL;
 	int ret;
@@ -1268,6 +1268,11 @@ struct drm_gem_object *msm_gem_new(struct drm_device *dev, size_t size, uint32_t
 	if (ret)
 		return ERR_PTR(ret);
 
+	if (flags & MSM_BO_NO_SHARE) {
+		drm_gem_object_get(r_obj);
+		obj->resv = r_obj->resv;
+	}
+
 	ret = drm_gem_object_init(dev, obj, size);
 	if (ret)
 		goto fail;
@@ -1311,6 +1316,7 @@ struct drm_gem_object *msm_gem_import(struct drm_device *dev,
 	 * up in drm_gem_object_put() -> msm_gem_free_object()
 	 */
 	obj->import_attach = attach;
+	obj->resv = dmabuf->resv;
 	drm_gem_private_object_init(dev, obj, size);
 
 	npages = size / PAGE_SIZE;
@@ -1346,7 +1352,7 @@ void *msm_gem_kernel_new(struct drm_device *dev, size_t size, uint32_t flags,
 			 uint64_t *iova)
 {
 	void *vaddr;
-	struct drm_gem_object *obj = msm_gem_new(dev, size, flags);
+	struct drm_gem_object *obj = msm_gem_new(dev, size, flags, NULL);
 	int ret;
 
 	if (IS_ERR(obj))
diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
index 86dc93cc1804..dff60cbc9d95 100644
--- a/drivers/gpu/drm/msm/msm_gem.h
+++ b/drivers/gpu/drm/msm/msm_gem.h
@@ -296,7 +296,7 @@ int msm_gem_cpu_fini(struct drm_gem_object *obj);
 int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
 		size_t size, uint32_t flags, uint32_t *handle, char *name);
 struct drm_gem_object *msm_gem_new(struct drm_device *dev,
-		size_t size, uint32_t flags);
+		size_t size, uint32_t flags, struct drm_gem_object *r_obj);
 void *msm_gem_kernel_new(struct drm_device *dev, size_t size, uint32_t flags,
 			 struct drm_gpuvm *vm, struct drm_gem_object **bo,
 			 uint64_t *iova);
-- 
2.55.0

Re: [PATCH v7 12/17] drm/msm/gem: Set resv before exposing obj

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing NULL check for the return value of `msm_context_vm()` causes a kernel panic when dereferencing the shared resv object.
--

--- Patch [12]: [PATCH v7 12/17] drm/msm/gem: Set resv before exposing obj ---
commit 1309e8291ed5a0d77adef43c83bb315bbd37c9af
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm/gem: Set resv before exposing obj

Don't swap the resv object _after_ exposing the newly created obj in LRU
or global objects list, as that creates a race condition where another
thread could lock the object using the original (per-obj) resv, but then
unlock after the resv is replaced.

> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index 3c36b1c4a4d59..535c6e1acb3c1 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
[ ... ]
> @@ -1136,10 +1136,17 @@ int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
>  		size_t size, uint32_t flags, uint32_t *handle,
>  		char *name)
>  {
> -	struct drm_gem_object *obj;
> +	struct drm_gem_object *obj, *r_obj = NULL;
>  	int ret;
>  
> -	obj = msm_gem_new(dev, size, flags);
> +	if (flags & MSM_BO_NO_SHARE) {
> +		struct msm_context *ctx = file->driver_priv;
> +		struct drm_gpuvm *vm = msm_context_vm(dev, ctx);
> +
> +		r_obj = drm_gpuvm_resv_obj(vm);

[Severity: High]
This is a pre-existing issue, but does this code need a NULL check for vm
before dereferencing it to access the shared resv object?

If userspace passes the MSM_BO_NO_SHARE flag in a display-only configuration
where no GPU is attached, msm_context_vm() will return NULL. Passing that
NULL pointer into the drm_gpuvm_resv_obj() macro leads to an immediate kernel
panic. 

Before this patch, the code directly dereferenced ctx->vm which would also be
NULL on display-only setups, so this problem wasn't introduced here but
remains present.

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

[PATCH v7 13/17] drm/msm/gem: 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>
Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/msm_gem.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 535c6e1acb3c..de4e60e28a71 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -1140,9 +1140,13 @@ int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
 	int ret;
 
 	if (flags & MSM_BO_NO_SHARE) {
+		struct msm_drm_private *priv = dev->dev_private;
 		struct msm_context *ctx = file->driver_priv;
 		struct drm_gpuvm *vm = msm_context_vm(dev, ctx);
 
+		if (!priv->gpu || !vm)
+			return UERR(EINVAL, dev, "not supported with shared VM");
+
 		r_obj = drm_gpuvm_resv_obj(vm);
 	}
 
-- 
2.55.0

Re: [PATCH v7 13/17] drm/msm/gem: Validate lazy VM in GEM_NEW

[sashiko-bot]

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

Pre-existing issues:
- [High] Unsafe Lazy VM Access / Null Pointer Dereference in msm_ioctl_gem_submit.
--

--- Patch [13]: [PATCH v7 13/17] drm/msm/gem: Validate lazy VM in GEM_NEW ---
commit c70d69b1e94af5ce03e9eafbf80189affee59538
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm/gem: Validate lazy VM in GEM_NEW

This commit fixes a potential NULL pointer dereference when creating a
NO_SHARE buffer object before any buffer objects are mapped. It ensures
that the lazy VM is properly initialized and supported before proceeding.

> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -1140,9 +1140,13 @@ int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
[ ... ]
>  	if (flags & MSM_BO_NO_SHARE) {
> +		struct msm_drm_private *priv = dev->dev_private;
>  		struct msm_context *ctx = file->driver_priv;
>  		struct drm_gpuvm *vm = msm_context_vm(dev, ctx);
>  
> +		if (!priv->gpu || !vm)
> +			return UERR(EINVAL, dev, "not supported with shared VM");
> +
>  		r_obj = drm_gpuvm_resv_obj(vm);
>  	}

[Severity: High]
This is a pre-existing issue, but seeing how msm_context_vm() is used here
to safely initialize the lazy VM, does a similar risk exist in
msm_ioctl_gem_submit() where ctx->vm is accessed directly?

If a user opens the DRM device node and immediately issues the MSM_GEM_SUBMIT
ioctl before any other ioctls trigger VM creation, could this sequence cause
a NULL pointer dereference?

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

Since base is at offset 0 in struct msm_gem_vm, to_msm_vm(NULL) evaluates to
NULL. Would accessing the unusable flag immediately cause a dereference
before msm_context_vm() is called to safely initialize the VM?

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

[PATCH v7 14/17] drm/msm: Allow lazy VM creation to fail

[Rob Clark]

In the next commit, we'll stop falling back to shared VM if private VM
creation fails.

This isn't expected to happen in practice, it would either require small
memory allocations to fail, or missing support in arm-smmu-qcom for
setting up per-process pgtable support (ie. missing patch during
bringup).

Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/adreno/adreno_gpu.c |  2 +-
 drivers/gpu/drm/msm/msm_drv.c           |  9 ++++++++-
 drivers/gpu/drm/msm/msm_gem_submit.c    | 12 ++++++++----
 drivers/gpu/drm/msm/msm_gem_vma.c       | 12 ++++++++----
 drivers/gpu/drm/msm/msm_submitqueue.c   |  8 +++++++-
 5 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 5738eac9aebd..84101e534ebe 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -359,7 +359,7 @@ int adreno_fault_handler(struct msm_gpu *gpu, unsigned long iova, int flags,
 static bool
 valid_per_process_vm(struct msm_gpu *gpu, struct drm_gpuvm *vm)
 {
-	return (vm != gpu->vm);
+	return vm && (vm != gpu->vm);
 }
 
 int adreno_get_param(struct msm_gpu *gpu, struct msm_context *ctx,
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 2fd1b717d8e6..52ff72d8e17a 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -422,10 +422,14 @@ static int msm_ioctl_gem_info_iova(struct drm_device *dev,
 {
 	struct msm_drm_private *priv = dev->dev_private;
 	struct msm_context *ctx = file->driver_priv;
+	struct drm_gpuvm *vm = msm_context_vm(dev, ctx);
 
 	if (!priv->gpu)
 		return -EINVAL;
 
+	if (!vm)
+		return UERR(ENOMEM, dev, "no VM");
+
 	if (msm_context_is_vmbind(ctx))
 		return UERR(EINVAL, dev, "VM_BIND is enabled");
 
@@ -436,7 +440,7 @@ static int msm_ioctl_gem_info_iova(struct drm_device *dev,
 	 * Don't pin the memory here - just get an address so that userspace can
 	 * be productive
 	 */
-	return msm_gem_get_iova(obj, msm_context_vm(dev, ctx), iova);
+	return msm_gem_get_iova(obj, vm, iova);
 }
 
 static int msm_ioctl_gem_info_set_iova(struct drm_device *dev,
@@ -450,6 +454,9 @@ static int msm_ioctl_gem_info_set_iova(struct drm_device *dev,
 	if (!priv->gpu)
 		return -EINVAL;
 
+	if (!vm)
+		return UERR(ENOMEM, dev, "no VM");
+
 	if (msm_context_is_vmbind(ctx))
 		return UERR(EINVAL, dev, "VM_BIND is enabled");
 
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index 040354ee16d1..6b0bee6c39bc 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;
@@ -552,6 +552,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;
@@ -567,10 +568,13 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
 	if (!gpu)
 		return -ENXIO;
 
+	if (!vm)
+		return UERR(ENOMEM, dev, "no VM");
+
 	if (args->pad)
 		return -EINVAL;
 
-	if (to_msm_vm(ctx->vm)->unusable)
+	if (to_msm_vm(vm)->unusable)
 		return UERR(EPIPE, dev, "context is unusable");
 
 	/* for now, we just have 3d pipe.. eventually this would need to
@@ -607,7 +611,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);
diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
index 3ed05ab0eeef..1fd4789f2be3 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;
@@ -1444,11 +1445,14 @@ msm_ioctl_vm_bind(struct drm_device *dev, void *data, struct drm_file *file)
 	if (!gpu)
 		return -ENXIO;
 
+	if (!vm)
+		return UERR(ENOMEM, dev, "no VM");
+
 	/*
 	 * 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");
 
 	/*
@@ -1479,7 +1483,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;
diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c
index 233c4f35ab12..d307cdd17d90 100644
--- a/drivers/gpu/drm/msm/msm_submitqueue.c
+++ b/drivers/gpu/drm/msm/msm_submitqueue.c
@@ -174,6 +174,7 @@ int msm_submitqueue_create(struct drm_device *drm, struct msm_context *ctx,
 	struct msm_drm_private *priv = drm->dev_private;
 	struct msm_gpu_submitqueue *queue;
 	enum drm_sched_priority sched_prio;
+	struct drm_gpuvm *vm = NULL;
 	unsigned ring_nr;
 	int ret;
 
@@ -186,6 +187,11 @@ int msm_submitqueue_create(struct drm_device *drm, struct msm_context *ctx,
 	if (flags & MSM_SUBMITQUEUE_VM_BIND) {
 		unsigned sz;
 
+		vm = msm_context_vm(drm, ctx);
+
+		if (!vm)
+			return UERR(ENOMEM, drm, "no VM");
+
 		/* Not allowed for kernel managed VMs (ie. kernel allocs VA) */
 		if (!msm_context_is_vmbind(ctx))
 			return -EINVAL;
@@ -217,7 +223,7 @@ int msm_submitqueue_create(struct drm_device *drm, struct msm_context *ctx,
 	queue->flags = flags;
 
 	if (flags & MSM_SUBMITQUEUE_VM_BIND) {
-		struct drm_gpu_scheduler *sched = &to_msm_vm(msm_context_vm(drm, ctx))->sched;
+		struct drm_gpu_scheduler *sched = &to_msm_vm(vm)->sched;
 
 		queue->entity = &queue->_vm_bind_entity[0];
 
-- 
2.55.0

Re: [PATCH v7 14/17] drm/msm: Allow lazy VM creation to fail

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Bypass of per-process pgtable support check in adreno_set_param for MSM_PARAM_EN_VM_BIND
--

--- Patch [14]: [PATCH v7 14/17] drm/msm: Allow lazy VM creation to fail ---
commit 4152ad8d83f2b59969c7edf51ff58a105d0b46bf
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Allow lazy VM creation to fail

This commit updates the driver to handle failures during lazy VM creation,
returning ENOMEM or EPIPE instead of silently proceeding without a VM.
It modifies various entry points like gem_submit, vm_bind, and submitqueue
creation to validate the VM pointer.

> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index 5738eac9aebd6..84101e534ebef 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -359,7 +359,7 @@ int adreno_fault_handler(struct msm_gpu *gpu, unsigned long iova, int flags,
>  static bool
>  valid_per_process_vm(struct msm_gpu *gpu, struct drm_gpuvm *vm)
>  {
> -	return (vm != gpu->vm);
> +	return vm && (vm != gpu->vm);
>  }
>  
>  int adreno_get_param(struct msm_gpu *gpu, struct msm_context *ctx,

[Severity: Critical]
This is a pre-existing issue, but while reviewing the VM validation changes
in this file, I noticed a potential check bypass in adreno_set_param().

If a platform lacks per-process pgtable support, lazy initialization means
ctx->vm will be NULL when adreno_set_param() is called.

Because ctx->vm is NULL, the following check:

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

will evaluate to false. It will then pass the second check:

    if (ctx->vm)
            return UERR(EBUSY, drm, "VM already created");

and successfully set ctx->userspace_managed_vm to the requested value.

When the VM is later lazily created via msm_context_vm(), it falls back to
the global gpu->vm, improperly setting up the global VM as a VM_BIND queue.

Does this allow userspace to opt into VM_BIND functionality on a global,
kernel-managed GPU VM?

[ ... ]

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

[PATCH v7 15/17] drm/msm: Don't fallback to shared VM for VM_BIND

[Rob Clark]

If the user wants a userspace managed VM (EN_VM_BIND) don't silently
fall back to shared VM.

Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/msm_drv.c | 3 ++-
 drivers/gpu/drm/msm/msm_gpu.c | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 52ff72d8e17a..73d99bde26f1 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -234,7 +234,8 @@ struct drm_gpuvm *msm_context_vm(struct drm_device *dev, struct msm_context *ctx
 	if (!ctx->vm) {
 		vm = msm_gpu_create_private_vm(
 			priv->gpu, current, !ctx->userspace_managed_vm);
-		smp_store_release(&ctx->vm, vm);
+		if (!IS_ERR_OR_NULL(vm))
+			smp_store_release(&ctx->vm, vm);
 	}
 
 	return ctx->vm;
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index 18ed00e5f143..a1c4adcac128 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -875,7 +875,7 @@ msm_gpu_create_private_vm(struct msm_gpu *gpu, struct task_struct *task,
 			to_msm_vm(vm)->pid = get_pid(task_pid(task));
 	}
 
-	if (IS_ERR_OR_NULL(vm))
+	if (IS_ERR_OR_NULL(vm) && kernel_managed)
 		vm = drm_gpuvm_get(gpu->vm);
 
 	return vm;
-- 
2.55.0

Re: [PATCH v7 15/17] drm/msm: Don't fallback to shared VM for VM_BIND

[sashiko-bot]

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

Pre-existing issues:
- [High] Unbounded memory leak of mmu objects in a6xx_create_private_vm() on userspace-managed VM creation failure
- [Medium] Flawed struct_size() overflow check in submit_create()
--

--- Patch [15]: [PATCH v7 15/17] drm/msm: Don't fallback to shared VM for VM_BIND ---
commit fad81fcf16686d778f256dee3ff6070327d5931e
Author: Rob Clark <robin.clark@oss.qualcomm.com>
Subject: drm/msm: Don't fallback to shared VM for VM_BIND

This patch modifies the virtual memory creation path in the MSM DRM driver.
If a user process requests a userspace managed VM, the driver will no longer
silently fall back to the shared, kernel-managed VM upon failure or absence
of private VM support.

> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 84b126c6347f7..db1b655dd055b 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -234,7 +234,8 @@ struct drm_gpuvm *msm_context_vm(struct drm_device *dev, struct msm_context *ctx
>  	if (!ctx->vm) {
>  		vm = msm_gpu_create_private_vm(
>  			priv->gpu, current, !ctx->userspace_managed_vm);
> -		smp_store_release(&ctx->vm, vm);
> +		if (!IS_ERR_OR_NULL(vm))
> +			smp_store_release(&ctx->vm, vm);
>  	}
>  
>  	return ctx->vm;
> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index 03c0578560658..0c2c356362518 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> @@ -879,7 +879,7 @@ msm_gpu_create_private_vm(struct msm_gpu *gpu, struct task_struct *task,
>  			to_msm_vm(vm)->pid = get_pid(task_pid(task));
>  	}
>  
> -	if (IS_ERR_OR_NULL(vm))
> +	if (IS_ERR_OR_NULL(vm) && kernel_managed)
>  		vm = drm_gpuvm_get(gpu->vm);
>  
>  	return vm;

[Severity: High]
This isn't a bug introduced by this patch, but does this change allow
userspace to trigger an unbounded memory leak?

When kernel_managed is false and gpu->funcs->create_private_vm()
(e.g., a6xx_create_private_vm()) fails, it returns an error pointer.
Before this patch, the code would fall back to the global gpu->vm.
Now, it propagates the error pointer back to the caller.

If we look at a6xx_create_private_vm():

drivers/gpu/drm/msm/adreno/a6xx_gpu.c:a6xx_create_private_vm() {
	...
	mmu = msm_iommu_pagetable_create(gpu->aspace->mmu);
	if (IS_ERR(mmu))
		return ERR_CAST(mmu);

	return msm_gem_vm_create(gpu->dev, mmu, "gpu", ADRENO_VM_START,
				 adreno_private_vm_size(gpu), kernel_managed);
}

If msm_gem_vm_create() fails, the previously allocated mmu object is not
freed.

Because this patch propagates the error and leaves ctx->vm as NULL in
msm_context_vm(), can an unprivileged user repeatedly issue an ioctl with
MSM_PARAM_EN_VM_BIND set, hit this error path, and continuously leak
mmu objects?

A similar pre-existing leak exists in the GMU probe path if
msm_gem_vm_create() fails:

drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a6xx_gmu_memory_probe() {
	...
	gmu->vm = msm_gem_vm_create(drm, mmu, "gmu", 0x0, 0x80000000, true);
	if (IS_ERR(gmu->vm))
		return PTR_ERR(gmu->vm);
	...
}


[Severity: Medium]
This is a pre-existing issue, but does this code properly handle overflow?

Looking at submit_create() in msm_gem_submit.c:

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

According to the DRM subsystem guidelines, struct_size() saturates at
SIZE_MAX upon overflow. On a 64-bit system where SIZE_MAX is UINT64_MAX,
adding a non-zero value wraps the uint64_t variable to a small value.
Does this make the sz > SIZE_MAX check dead code that fails to catch the
overflow?

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

[PATCH v7 16/17] 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>
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 84101e534ebe..3370cd44382f 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -513,7 +513,7 @@ int adreno_set_param(struct msm_gpu *gpu, struct msm_context *ctx,
 		guard(rwsem_read)(&ctx->ctxlock);
 
 		/* 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

[PATCH v7 17/17] drm/msm: Fixup 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.

On the other hand, we should be using the overflow helpers to calculate
the cmd array size.

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 | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index 6b0bee6c39bc..5862db05297a 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -36,14 +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);
+	sz = size_add(struct_size(submit, bos, nr_bos),
+		      array_size(sizeof(submit->cmd[0]), nr_cmds));
 
 	submit = kzalloc(sz, GFP_KERNEL | __GFP_NOWARN);
 	if (!submit)
-- 
2.55.0