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

[sashiko-bot@kernel.org]

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

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

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

drm/msm: Validate lazy VM is created in GEM_SUBMIT

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

> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> index 26ea8a28be474..0dc983d155cd2 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -30,7 +30,7 @@
>   */
>  
>  static struct msm_gem_submit *submit_create(struct drm_device *dev,
> -		struct msm_gpu *gpu,
> +		struct msm_gpu *gpu, struct drm_gpuvm *vm,
>  		struct msm_gpu_submitqueue *queue, uint32_t nr_bos,
>  		uint32_t nr_cmds, u64 drm_client_id)
>  {

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

submit_create() calculates the size as:

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

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

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

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

[ ... ]

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

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

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

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

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

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

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

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

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

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