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

[sashiko-bot@kernel.org]

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