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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The size calculation for the `msm_gem_submit` structure wraps around on 64-bit architectures, bypassing the `sz > SIZE_MAX` check and leading to a heap buffer overflow.
- [High] Direct access to `ctx->vm` in `msm_ioctl_gem_submit` before the VM is guaranteed to be created causes a NULL pointer dereference.
--

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

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

This patch changes the submit path to access the VM directly from the context
rather than calling msm_context_vm() to lazily create it. It relies on the
assumption that the ioctl has already ensured the VM is created prior to this
point in the execution flow.

> 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.

[Severity: High]
This isn't a bug introduced by this patch, but does this assumption hold true?

Looking at the ioctl entry point in msm_ioctl_gem_submit(), if userspace
triggers this ioctl before any operation that creates the VM, the VM pointer
is initially null:

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

Can this direct access to ctx->vm cause a null pointer dereference, since a
preceding call to msm_context_vm() hasn't guaranteed its existence yet?

> 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;
>  	struct adreno_gpu *adreno_gpu = &a6xx_gpu->base;
>  	phys_addr_t ttbr;
>  	u32 asid;

While reviewing the submit path related to this change, I noticed another
pre-existing issue in the submission creation logic:

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 check protect against integer
overflows effectively?

Since sz is a 64-bit unsigned integer, the check sz > SIZE_MAX is
mathematically impossible and acts as dead code.

If a user provides a sufficiently large nr_bos, struct_size() saturates to
SIZE_MAX. Adding a non-zero value for nr_cmds then causes sz to wrap around
to a small value.

Could this lead to an undersized buffer allocation from kzalloc() and a
subsequent out-of-bounds heap write during initialization?

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