Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] Direct access to `ctx->vm` in `adreno_set_param()` bypasses capability checks for per-process pgtables, allowing userspace to enable VM_BIND on unsupported hardware and crash the kernel. -- --- Patch [8]: [PATCH v7 08/17] drm/msm: Add helper to check for per-process pgtables VM --- commit c61563d7d541358abb7f78356a4f5b717a237572 Author: Rob Clark <[email protected]> drm/msm: Add helper to check for per-process pgtables VM This patch adds a helper function to validate whether a GPU VM supports per-process pagetables. It then updates adreno_get_param() to use this new helper. > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c > b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > index 0d54141ff089c..5738eac9aebd6 100644 > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > @@ -356,6 +356,12 @@ int adreno_fault_handler(struct msm_gpu *gpu, unsigned > long iova, int flags, > return 0; > } > > +static bool > +valid_per_process_vm(struct msm_gpu *gpu, struct drm_gpuvm *vm) > +{ > + return (vm != gpu->vm); > +} > + > int adreno_get_param(struct msm_gpu *gpu, struct msm_context *ctx, > uint32_t param, uint64_t *value, uint32_t *len) > { > @@ -414,12 +420,12 @@ int adreno_get_param(struct msm_gpu *gpu, struct > msm_context *ctx, > *value = gpu->suspend_count; > return 0; > case MSM_PARAM_VA_START: > - if (vm == gpu->vm) > + if (!valid_per_process_vm(gpu, vm)) > return UERR(EINVAL, drm, "requires per-process > pgtables"); > *value = vm->mm_start; > return 0; > case MSM_PARAM_VA_SIZE: > - if (vm == gpu->vm) > + if (!valid_per_process_vm(gpu, vm)) > return UERR(EINVAL, drm, "requires per-process > pgtables"); > *value = vm->mm_range; > return 0; [Severity: High] This isn't a bug introduced by this patch, but while you are updating the VM checks here, should the same logic be applied to adreno_set_param()? Looking at adreno_set_param() for MSM_PARAM_EN_VM_BIND, it accesses ctx->vm directly: drivers/gpu/drm/msm/adreno/adreno_gpu.c:adreno_set_param() { ... case MSM_PARAM_EN_VM_BIND: { guard(rwsem_read)(&ctx->ctxlock); /* We can only support VM_BIND with per-process pgtables: */ if (ctx->vm == gpu->vm) 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"); ... } Because msm uses lazy initialization for virtual memory, isn't ctx->vm guaranteed to be NULL here prior to creation? If ctx->vm is NULL, then ctx->vm == gpu->vm evaluates to false, which bypasses the capability check intended to prevent VM_BIND on platforms lacking per-process pgtables. Could this allow userspace to enable VM_BIND on older hardware and subsequently crash the kernel when it binds to an uninitialized scheduler on the global VM? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=8
