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 <[email protected]> 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/[email protected]?part=15
