Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [Critical] Incomplete fix: Falling back to the shared VM on private VM allocation failure allows EXECBUF isolation bypass, arbitrary kernel memory access, and information leaks. - [High] Direct access to `ctx->vm` in `msm_ioctl_gem_submit()` before the VM is guaranteed to be initialized causes a NULL pointer dereference. -- --- Patch [2]: [PATCH v4 2/6] drm/msm: Block VM_BIND for shared VMs --- commit 14b0c06028a80fcfb812d65ea727ff988063c94e Author: Rob Clark <[email protected]> drm/msm: Block VM_BIND for shared VMs We already block MSM_PARAM_EN_VM_BIND where per-process VMs are not supported. But in error paths when we actually try to create the VM (allocation failures, etc) we could still end up with a shared-VM. Since we need to create the VM lazily, for backwards compat, there isn't really a better place to return an error. But this isn't to happen in practice. Just block VM_BIND ioctls in this case so we aren't giving userspace a way to manage the shared VM. > diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c > b/drivers/gpu/drm/msm/msm_gem_vma.c > index 1a952b171ed7f..c88b4352ad751 100644 > --- a/drivers/gpu/drm/msm/msm_gem_vma.c > +++ b/drivers/gpu/drm/msm/msm_gem_vma.c [ ... ] > @@ -1449,7 +1450,7 @@ msm_ioctl_vm_bind(struct drm_device *dev, void *data, > struct drm_file *file) > * Maybe we could allow just UNMAP ops? OTOH userspace should just > * immediately close the device file and all will be torn down. > */ > - if (to_msm_vm(msm_context_vm(dev, ctx))->unusable) > + if (to_msm_vm(vm)->unusable) > return UERR(EPIPE, dev, "context is unusable"); > > /* > @@ -1457,7 +1458,7 @@ msm_ioctl_vm_bind(struct drm_device *dev, void *data, > struct drm_file *file) > * place, if you haven't opted in to VM_BIND context. But it is > * cleaner / less confusing, to check this case directly. > */ > - if (!msm_context_is_vmbind(ctx)) > + if (to_msm_vm(vm)->managed) [Severity: Critical] This is a pre-existing issue, but does falling back to the shared VM on private VM allocation failure allow an execbuf isolation bypass? While this patch blocks vm_bind ioctls for shared VMs, an attacker could still opt into vm_bind (setting ctx->userspace_managed_vm = true) and intentionally cause private VM allocation to fail. In this case, msm_gpu_create_private_vm() falls back to the global kernel VM: drivers/gpu/drm/msm/msm_gpu.c:msm_gpu_create_private_vm() { ... vm = drm_gpuvm_get(gpu->vm); ... } If the attacker then creates a normal submitqueue and calls msm_ioctl_gem_submit(), command validation is skipped because msm_context_is_vmbind(ctx) still returns true based on user intent rather than actual VM state: drivers/gpu/drm/msm/msm_gem_submit.c:msm_ioctl_gem_submit() { ... cmds_to_parse = msm_context_is_vmbind(ctx) ? 0 : args->nr_cmds; ... } This allows the unvalidated user-provided IOVA to be mapped and executed within the global kernel VM: drivers/gpu/drm/msm/msm_gem_submit.c:submit_lookup_cmds() { ... submit->cmd[i].iova = submit_cmd.iova; ... } Could this lead to arbitrary kernel memory access, or leak other processes' data into the devcoredump if a job hangs? [Severity: High] This isn't a bug introduced by this patch, but does direct access to ctx->vm in msm_ioctl_gem_submit() cause a null pointer dereference? If a user directly calls msm_ioctl_gem_submit() on a fresh context without performing any prior operation (like msm_ioctl_vm_bind) that initializes the VM, ctx->vm remains null due to lazy initialization. Does this check in msm_ioctl_gem_submit() cause an immediate crash when dereferencing the offset to unusable? drivers/gpu/drm/msm/msm_gem_submit.c:msm_ioctl_gem_submit() { ... if (to_msm_vm(ctx->vm)->unusable) ... } > return UERR(EINVAL, dev, "context does not support vmbind"); > > if (args->flags & ~MSM_VM_BIND_FLAGS) -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
