Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [Critical] Insufficient capability check and silent fallback to the global GPU VM allows userspace to manage the global VM via VM_BIND. - [High] A time-of-check to time-of-use (TOCTOU) data race when opting into VM_BIND mode allows a state mismatch between `ctx->userspace_managed_vm` and the underlying IOMMU page table configuration. -- commit 29eb04f2a503cc0bd049bafc825248eb1ad0a013 Author: Rob Clark <[email protected]> drm/msm: Fix per-process-pgtables check 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 <[email protected]> Fixes: feb8ef4636a4 ("drm/msm: Add opt-in for VM_BIND") Signed-off-by: Rob Clark <[email protected]> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c > b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > index c62c45bb0ddbb..bff2a773c0d4e 100644 > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > @@ -505,7 +505,7 @@ int adreno_set_param(struct msm_gpu *gpu, struct > msm_context *ctx, > return msm_context_set_sysprof(ctx, gpu, value); > case MSM_PARAM_EN_VM_BIND: > /* We can only support VM_BIND with per-process pgtables: */ > - if (ctx->vm == gpu->vm) > + if (!gpu->funcs->create_private_vm) [Severity: Critical] This is a pre-existing issue, but does relying on a static function pointer here rather than verifying actual VM creation success allow an invalid configuration to proceed? If dynamic creation of the private VM fails in msm_gpu_create_private_vm() (e.g., due to memory allocation failure or missing IOMMU support returning -ENODEV), it silently falls back to returning the global, kernel-managed gpu->vm: drivers/gpu/drm/msm/msm_gpu.c:msm_gpu_create_private_vm() { ... if (IS_ERR_OR_NULL(vm)) vm = drm_gpuvm_get(gpu->vm); ... } Because this check allows userspace to set ctx->userspace_managed_vm to true, could a malicious process then use DRM_IOCTL_MSM_GEM_VM_BIND to issue unmap and map operations against the global VM? This seems to allow unmapping kernel structures like the memptrs ringbuffer and mapping arbitrary memory in its place, potentially hijacking the GPU ringbuffer and compromising the kernel. Furthermore, passing offset != 0 or handle == 0 could trigger a BUG_ON() causing a kernel panic. > 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"); > > ctx->userspace_managed_vm = value; [Severity: High] This isn't a bug introduced by this patch, but is there a race condition here because the check of ctx->vm and assignment to ctx->userspace_managed_vm are done without synchronization? Could a concurrent ioctl (like MSM_GEM_SUBMIT) invoke msm_context_vm(), which reads ctx->userspace_managed_vm before it is set to true here, causing it to lazily create a legacy kernel-managed private VM? drivers/gpu/drm/msm/msm_drv.c:msm_context_vm() { ... vm = msm_gpu_create_private_vm( priv->gpu, current, !ctx->userspace_managed_vm); ... } If adreno_set_param() then overwrites ctx->userspace_managed_vm to true, and userspace subsequently issues DRM_IOCTL_MSM_GEM_VM_BIND, would the ioctl proceed on the kernel-managed VM? During the bind operation, wouldn't msm_gem_vma_new() see vm->managed == true and execute BUG_ON() if offset != 0 or handle == 0, triggering a kernel panic? > > return 0; > default: -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
