Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [Critical] `msm_context_vm()` returns `NULL` when VM creation fails, leading to unhandled NULL pointer dereference in callers. - [Critical] Flawed hardware capability check in `adreno_set_param()` allows VM_BIND on unsupported hardware. - [High] Missing error check for `drm_sched_entity_init()` leaves the scheduler entity uninitialized. -- --- Patch [3]: [PATCH v7 03/17] drm/msm: Synchronize VM creation on ctxlock --- commit fd449ed84ba9ef6ab17d7a557f7bc86f9a0a680e Author: Rob Clark <[email protected]> drm/msm: Synchronize VM creation on ctxlock This commit replaces the global init_lock mutex with a per-context rw_semaphore to synchronize VM creation and serialize setting EN_VM_BIND. > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c > b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > index c62c45bb0ddbb..0d54141ff089c 100644 > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c [ ... ] > @@ -503,7 +503,9 @@ int adreno_set_param(struct msm_gpu *gpu, struct > msm_context *ctx, > if (!perfmon_capable()) > return UERR(EPERM, drm, "invalid permissions"); > return msm_context_set_sysprof(ctx, gpu, value); > - case MSM_PARAM_EN_VM_BIND: > + 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"); [Severity: Critical] This isn't a bug introduced by this patch, but does this hardware capability check evaluate correctly before VM creation? Before this change, the check if (ctx->vm == gpu->vm) evaluates to false because ctx->vm is NULL before lazy VM creation. This allows an unprivileged user to successfully set ctx->userspace_managed_vm on hardware lacking create_private_vm support. When the VM is subsequently created, it returns NULL, and the caller crashes. [ ... ] > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > index 77681cb4fdc25..a5714e24fcc6c 100644 > --- a/drivers/gpu/drm/msm/msm_drv.c > +++ b/drivers/gpu/drm/msm/msm_drv.c [ ... ] > @@ -230,7 +229,7 @@ struct drm_gpuvm *msm_context_vm(struct drm_device *dev, > struct msm_context *ctx > if (vm) > return vm; > > - guard(mutex)(&init_lock); > + guard(rwsem_write)(&ctx->ctxlock); > > if (!ctx->vm) { > vm = msm_gpu_create_private_vm( [Severity: Critical] This isn't a bug introduced by this patch, but does msm_context_vm() properly propagate errors if VM creation fails? If msm_gpu_create_private_vm() fails (e.g., due to memory exhaustion) and returns an error pointer, msm_context_vm() does not populate ctx->vm and returns NULL. Callers like msm_submitqueue_create() unconditionally cast the return value using to_msm_vm() and dereference its ->sched member, which can cause a kernel panic. I also noticed a pre-existing issue in msm_submitqueue_create() in drivers/gpu/drm/msm/msm_submitqueue.c: [Severity: High] This isn't a bug introduced by this patch, but is it safe to ignore the return value of drm_sched_entity_init()? In msm_submitqueue_create(), the return value of drm_sched_entity_init() is ignored: drm_sched_entity_init(queue->entity, DRM_SCHED_PRIORITY_KERNEL, &sched, 1, NULL); If initialization fails, the function continues. The subsequent error check if (IS_ERR(queue->entity)) fails to detect the error because queue->entity points to an embedded array element, not an ERR_PTR. This could lead to using an uninitialized scheduler entity, potentially causing memory corruption or crashes. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
