Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Missing error check for msm_fence_context_alloc() return value leads to invalid pointer dereference. - [High] Direct access to ctx->vm without lazy initialization causes NULL pointer dereference. -- --- Patch [1]: [PATCH] drm/msm: Only fini scheduler after successful init --- commit b85d84ece3c22f19d85b416480e8fa8cc2d6160b Author: Ruoyu Wang <[email protected]> drm/msm: Only fini scheduler after successful init msm_ringbuffer_new() destroys a partially initialized ring through msm_ringbuffer_destroy() when an allocation or scheduler setup step fails. If drm_sched_init() fails before it finishes initializing the scheduler, the failure path still calls drm_sched_fini() which assumes the state was fully initialized. This patch tracks successful scheduler initialization and calls drm_sched_fini() only after drm_sched_init() returned 0. [ ... ] > diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.c > b/drivers/gpu/drm/msm/msm_ringbuffer.c > index 0d14c31bd4e47..af2b0edc4d586 100644 > --- a/drivers/gpu/drm/msm/msm_ringbuffer.c > +++ b/drivers/gpu/drm/msm/msm_ringbuffer.c [ ... ] > @@ -132,7 +132,8 @@ void msm_ringbuffer_destroy(struct msm_ringbuffer *ring) > if (IS_ERR_OR_NULL(ring)) > return; > > - drm_sched_fini(&ring->sched); > + if (ring->sched_initialized) > + drm_sched_fini(&ring->sched); > > msm_fence_context_free(ring->fctx); [Severity: High] This isn't a regression introduced by this patch, but there is a missing error check for the msm_fence_context_alloc() return value. drivers/gpu/drm/msm/msm_ringbuffer.c:msm_ringbuffer_new() { ... snprintf(name, sizeof(name), "gpu-ring-%d", ring->id); ring->fctx = msm_fence_context_alloc(gpu->dev, &ring->memptrs->fence, name); return ring; } msm_fence_context_alloc() can return ERR_PTR(-ENOMEM) on memory allocation failure. The error pointer is assigned to ring->fctx without an IS_ERR() validation, and the new ring object is returned as if successful. When the ring is later destroyed here, msm_ringbuffer_destroy() passes the error pointer to msm_fence_context_free(), which calls kfree() on it. Does this need an IS_ERR() check to prevent an invalid memory access? [ ... ] [Severity: High] This is also a pre-existing regression, but could this lead to a NULL pointer dereference? 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); ... } The MSM driver lazily initializes virtual memory address spaces. If userspace calls an ioctl that triggers msm_gem_new_handle() with the MSM_BO_NO_SHARE flag before the VM is initialized, ctx->vm will be NULL. Passing this NULL pointer to drm_gpuvm_resv_obj(ctx->vm) results in dereferencing NULL to access the r_obj field. Should this use the msm_context_vm() accessor instead to ensure the VM is initialized before access? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
