Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Unwinding a failed framebuffer prepare while a concurrent prepare exists causes a double-unpin and refcount underflow. Pre-existing issues: - [High] Concurrent `msm_framebuffer_prepare` calls for the same framebuffer can race due to lockless `prepare_count`, leading to uninitialized `iova` access and IOMMU faults. -- commit 4b106914920d42508f14e872754ef791f46ae379 Author: Dmitry Baryshkov <[email protected]> drm/msm: fix framebuffer pin refcount leak on prepare failure This commit addresses a resource leak in msm_framebuffer_prepare() by unwinding successfully pinned planes and releasing the vma reference when a subsequent plane fails to pin. [Severity: High] This is a pre-existing issue, but can concurrent msm_framebuffer_prepare() calls for the same framebuffer race due to the lockless prepare_count check? Looking at msm_framebuffer_prepare() in drivers/gpu/drm/msm/msm_fb.c: if (atomic_inc_return(&msm_fb->prepare_count) > 1) return 0; If userspace issues concurrent atomic commits for different CRTCs sharing the same framebuffer, Thread A could increment prepare_count to 1 and start pinning planes. Thread B could concurrently increment prepare_count to 2, hit this lockless fast-path, and return success immediately before Thread A has finished populating the iova array. Thread B's commit might then proceed to scanout using uninitialized memory addresses (zeros) for DMA, which could cause an IOMMU unhandled context fault. Could msm_framebuffer_cleanup() also race in a similar way? if (atomic_dec_return(&msm_fb->prepare_count)) return; memset(msm_fb->iova, 0, sizeof(msm_fb->iova)); This lockless memset could race with a concurrent prepare_fb from Thread A, overwriting a newly pinned IOVA with zero. > diff --git a/drivers/gpu/drm/msm/msm_fb.c b/drivers/gpu/drm/msm/msm_fb.c > index 60c108d35d2a1..934337202afd4 100644 > --- a/drivers/gpu/drm/msm/msm_fb.c > +++ b/drivers/gpu/drm/msm/msm_fb.c > @@ -89,11 +89,27 @@ int msm_framebuffer_prepare(struct drm_framebuffer *fb, > bool needs_dirtyfb) [ ... ] > return 0; > + > +unwind: > + while (i--) { > + msm_gem_unpin_iova(fb->obj[i], vm); > + msm_gem_vma_put(fb->obj[i]); > + } > + > + memset(msm_fb->iova, 0, sizeof(msm_fb->iova)); > + > + atomic_dec(&msm_fb->prepare_count); [Severity: High] Does this unwind path create a double-unpin if there is a concurrent prepare that succeeded via the fast-path? If Thread A and B enter msm_framebuffer_prepare() concurrently, Thread A increments prepare_count to 1 and begins pinning. Thread B increments it to 2 and returns success. If Thread A encounters an error, it enters this newly added unwind block, unpins the planes it successfully pinned, and decrements prepare_count from 2 to 1. When Thread B's commit succeeds and later calls msm_framebuffer_cleanup(), it will decrement prepare_count from 1 to 0. Since it reaches 0, Thread B will unconditionally unpin all planes: drivers/gpu/drm/msm/msm_fb.c:msm_framebuffer_cleanup() { ... if (atomic_dec_return(&msm_fb->prepare_count)) return; memset(msm_fb->iova, 0, sizeof(msm_fb->iova)); if (crtc && msm_crtc_queue_fb_unpin(crtc, fb)) return; for (i = 0; i < n; i++) { msm_gem_unpin_iova(fb->obj[i], vm); msm_gem_vma_put(fb->obj[i]); } } Because Thread A already unpinned the planes it originally pinned, Thread B's cleanup would result in a double-unpin, causing vma_ref and pin_count underflows, and potentially a use-after-free of GEM memory. > + if (needs_dirtyfb) > + refcount_dec(&msm_fb->dirtyfb); > + > + return ret; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
