Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [Critical] Silent fallback to the global GPU VM when private VM allocation fails breaks memory isolation. - [High] Direct access to ctx->vm before initialization causes a NULL pointer dereference in IOCTL handlers. - [High] Missing smp_load_acquire() when reading ctx->vm in msm_gem_close() can lead to accessing partially initialized memory. -- --- Patch [1]: [PATCH v4 1/6] drm/msm: Fix barriers accessing ctx vm --- commit 5827c45dcfd83b1ca9d1ff0ea3f3c026fba67fb3 Author: Rob Clark <[email protected]> drm/msm: Fix barriers accessing ctx vm This commit fixes memory barrier issues when accessing the context VM by introducing smp_load_acquire and smp_store_release in msm_context_vm. It prevents callers from seeing a partially initialized virtual machine. > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > index 79f78e7e16e9d..3c8b5083a1fbe 100644 > --- a/drivers/gpu/drm/msm/msm_drv.c > +++ b/drivers/gpu/drm/msm/msm_drv.c > @@ -225,18 +225,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); [Severity: Critical] This isn't a bug introduced by this patch, but looking at the VM creation logic in msm_gpu_create_private_vm() called above, does it safely handle memory allocation failures? drivers/gpu/drm/msm/msm_gpu.c:msm_gpu_create_private_vm() { ... if (gpu->funcs->create_private_vm) { vm = gpu->funcs->create_private_vm(gpu, kernel_managed); ... } if (IS_ERR_OR_NULL(vm)) vm = drm_gpuvm_get(gpu->vm); return vm; } If the private VM allocation fails (returning ERR_PTR(-ENOMEM) due to system memory exhaustion), this fallback grants the userspace context access to the shared global GPU VM. Could an attacker intentionally exhaust memory to force this fallback, thereby bypassing private VM isolation and gaining unauthorized access to structures stored in the global VM? > - > + smp_store_release(&ctx->vm, vm); > } [Severity: High] This isn't a bug introduced by this patch, but since we are fixing the barriers for ctx->vm initialization here, does msm_gem_close() also need an smp_load_acquire() barrier? Looking at msm_gem_close() in drivers/gpu/drm/msm/msm_gem.c, it reads ctx->vm using a plain load: drivers/gpu/drm/msm/msm_gem.c:msm_gem_close() { ... if (!ctx->vm) return; ... } If msm_gem_close() executes concurrently with VM creation, could it see a non-NULL pointer without an acquire barrier and access partially initialized memory? [Severity: High] This is a pre-existing issue, but I noticed there are paths that directly access ctx->vm without calling msm_context_vm() first to ensure it is created. For example, in msm_ioctl_gem_submit(): drivers/gpu/drm/msm/msm_gem_submit.c:msm_ioctl_gem_submit() { ... if (to_msm_vm(ctx->vm)->unusable) return -EPIPE; ... } And in msm_gem_new_handle(): drivers/gpu/drm/msm/msm_gem.c:msm_gem_new_handle() { ... struct drm_gem_object *r_obj = drm_gpuvm_resv_obj(ctx->vm); ... } If userspace calls either of these IOCTLs on a fresh context before the VM is initialized, could ctx->vm be NULL here, leading to a NULL pointer dereference when computing the struct offset or dereferencing it? > - mutex_unlock(&init_lock); > > return ctx->vm; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
