Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues: - [Critical] Directly accessing `ctx->vm` in ioctl entry points before ensuring the VM is initialized via `msm_context_vm()` causes a NULL pointer dereference on the first operation. - [Critical] `msm_context_vm()` unconditionally caches an `ERR_PTR` inside `ctx->vm` upon allocation failure, permanently breaking the context and causing crashes on subsequent accesses. - [Critical] Callers of `msm_context_vm()` fail to check if the returned value is an error pointer, leading to a crash on VM creation failure. - [High] The patch misses concurrent readers like `msm_gem_close()` that still perform plain reads of `ctx->vm`, leaving them vulnerable to data races and partially initialized VM states. -- --- Patch [1]: [PATCH v7 01/17] drm/msm: Fix barriers accessing ctx vm --- Commit SHA: 8c0550637c7f990d073da2dbc73f127555883df0 Author: Rob Clark <[email protected]> Subject: drm/msm: Fix barriers accessing ctx vm This commit updates msm_context_vm() to use proper memory barriers (smp_load_acquire and smp_store_release) to prevent callers from observing a partially initialized VM. > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > index ac86b427c0e53..29075aafc5cc5 100644 > --- a/drivers/gpu/drm/msm/msm_drv.c > +++ b/drivers/gpu/drm/msm/msm_drv.c > @@ -224,18 +224,19 @@ struct drm_gpuvm *msm_context_vm(struct drm_device > *dev, struct msm_context *ctx > { > static DEFINE_MUTEX(init_lock); > struct msm_drm_private *priv = dev->dev_private; > + struct drm_gpuvm *vm = smp_load_acquire(&ctx->vm); > > /* Once ctx->vm is created it is valid for the lifetime of the context: > */ > - if (ctx->vm) > - return ctx->vm; > + if (vm) > + return vm; > + > + guard(mutex)(&init_lock); > > - mutex_lock(&init_lock); > if (!ctx->vm) { > - ctx->vm = msm_gpu_create_private_vm( > + vm = msm_gpu_create_private_vm( > priv->gpu, current, !ctx->userspace_managed_vm); > - > + smp_store_release(&ctx->vm, vm); [Severity: Critical] This is a pre-existing issue, but I noticed that msm_gpu_create_private_vm() can return an ERR_PTR on failure (e.g., if page table allocation fails). If we store that ERR_PTR into ctx->vm without an IS_ERR() check, will future fast-path readers see this truthy pointer and attempt to dereference the error value as a valid VM? > } > - mutex_unlock(&init_lock); > > return ctx->vm; > } [Severity: Critical] This is also a pre-existing issue, but does submit_create() need to check the return value of msm_context_vm() before assigning it to submit->vm? drivers/gpu/drm/msm/msm_gem_submit.c:submit_create() { ... submit->dev = dev; submit->vm = msm_context_vm(dev, queue->ctx); submit->gpu = gpu; ... } If it returns an ERR_PTR, it will eventually cause a panic when submit->vm is dereferenced. Similarly, in msm_submitqueue_create(): drivers/gpu/drm/msm/msm_submitqueue.c:msm_submitqueue_create() { ... if (flags & MSM_SUBMITQUEUE_VM_BIND) { struct drm_gpu_scheduler *sched = &to_msm_vm(msm_context_vm(drm, ctx))->sched; queue->entity = &queue->_vm_bind_entity[0]; ... } Does this code risk a crash by passing an unchecked pointer to to_msm_vm(), which computes an offset from the error pointer and then accesses sched? [Severity: Critical] This isn't a bug introduced by this patch, but I noticed some paths directly access ctx->vm before it's guaranteed to be initialized. For example: drivers/gpu/drm/msm/msm_gem_submit.c:msm_ioctl_gem_submit() { ... if (args->pad) return -EINVAL; if (to_msm_vm(ctx->vm)->unusable) return UERR(EPIPE, dev, "context is unusable"); ... } If an unprivileged user calls this ioctl as the very first operation on a new context, won't ctx->vm still be NULL here, leading to an invalid pointer dereference via the to_msm_vm() offset macro? I noticed a similar pattern in msm_gem_new_handle(): drivers/gpu/drm/msm/msm_gem.c:msm_gem_new_handle() { ... if (flags & MSM_BO_NO_SHARE) { struct msm_context *ctx = file->driver_priv; struct drm_gem_object *r_obj = drm_gpuvm_resv_obj(ctx->vm); drm_gem_object_get(r_obj); ... } [Severity: High] This isn't a bug introduced by this patch, but since the patch addresses concurrent access to ctx->vm, does msm_gem_close() also need to use smp_load_acquire() instead of a plain read? drivers/gpu/drm/msm/msm_gem.c:msm_gem_close() { ... msm_gem_vma_put(obj); /* * If VM isn't created yet, nothing to cleanup. And in fact calling * put_iova_spaces() with vm=NULL would be bad, in that it will tear- * down the mappings of shared buffers in other contexts. */ if (!ctx->vm) return; ... } Without a read barrier, could a concurrent ioctl that initializes the VM cause this function to observe a non-NULL pointer but read uninitialized memory from the VM structure? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
