Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] The patch fails to address a critical use-after-free of `crtc->fence_lock` because `synchronize_rcu()` does not protect external spinlocks. - [High] The synchronize_rcu() fix violates the RCU-001 'Remove Before Reclaim' rule, failing to prevent UAF of the CRTC when new readers query the fence. -- --- Patch [1]: [PATCH v3 1/2] drm/drm_crtc: ensure dma_fence_ops remain valid during device unbind --- commit 31b9417fd75463986e2e186faa6c672078a471fd Author: André Draszik <[email protected]> drm/drm_crtc: ensure dma_fence_ops remain valid during device unbind This commit adds a synchronize_rcu() call to drm_crtc_cleanup() before removing the CRTC from the DRM mode setting core. This is intended to wait for an RCU grace period to ensure that dma_fence_ops can safely dereference crtc->dev if userspace queries a sync_file concurrently with device hot-unplug. Link: https://sashiko.dev/#/patchset/[email protected]?part=1 > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 63ead8ba67564..e8e80c9368529 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -493,14 +493,23 @@ EXPORT_SYMBOL(__drmm_crtc_alloc_with_planes); [ ... ] > void drm_crtc_cleanup(struct drm_crtc *crtc) > { > struct drm_device *dev = crtc->dev; > > + /* Ensure our dma_fence_ops remain valid for an RCU grace period after > + * the fence is signaled. This is necessary because our dma_fence_ops > + * dereference crtc->dev. > + */ > + synchronize_rcu(); [Severity: High] This is a pre-existing issue, but does this actually protect the crtc->fence_lock? In drm_crtc_create_fence(), the dma_fence is initialized with a pointer to the CRTC's embedded spinlock: drivers/gpu/drm/drm_crtc.c:drm_crtc_create_fence() { ... dma_fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock, crtc->fence_context, ++crtc->fence_seqno); ... } Because the dma_fence does not hold a reference to the CRTC, the fence can outlive the CRTC. The synchronize_rcu() call added here provides no protection for the direct spinlock acquisition in the dma_fence API. If userspace calls ioctl(SYNC_IOC_FILE_INFO) after the CRTC is freed, the kernel calls dma_fence_get_status(), which executes spin_lock_irqsave() on the fence's external lock (pointing to the freed crtc->fence_lock). Can this still lead to a use-after-free of the crtc->fence_lock? [Severity: High] This isn't a bug introduced by this patch, but does calling synchronize_rcu() here violate the RCU "Remove Before Reclaim" rule? Because the fence is exposed to userspace via sync_file, it is not removed from RCU-accessible paths before synchronize_rcu() is called. synchronize_rcu() only waits for existing readers. New userspace calls to ioctl(SYNC_IOC_FILE_INFO) after the CRTC is destroyed will start a new rcu_read_lock(), call ops->get_driver_name(), cast the fence to the freed CRTC, and dereference crtc->dev. Does this leave the CRTC structure vulnerable to a use-after-free when new readers query the fence? > + > /* Note that the crtc_list is considered to be static; should we > * remove the drm_crtc at runtime we would have to decrement all > * the indices on the drm_crtc after us in the crtc_list. > */ -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
