Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] The identical use-after-free vulnerability fixed in the fbdev OMAP DSI driver remains unpatched in the equivalent DRM driver. - [High] The Use-After-Free vulnerability fixed in the DSI driver is identically present in the DISPC hardware interrupt demultiplexer. - [High] The driver fails to cancel the delayed work `framedone_timeout_work` and `te_timer` in its unbind/teardown path, causing a Use-After-Free. -- commit c84d1fd5cdf35436c608e4883f1ea5ab4237c8b4 Author: Andreas Kemnade <[email protected]> fb: omap2: dsi: do not copy isr table This commit prevents a use-after-free in the omap2 dsi driver by no longer copying the ISR table and dropping the lock before executing handlers. This avoids a race condition where a stack-allocated object could be freed while the lockless ISR copy is still being processed. > diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dsi.c > b/drivers/video/fbdev/omap2/omapfb/dss/dsi.c > index 330d9fb7d2b00..d98db01fdd39c 100644 > --- a/drivers/video/fbdev/omap2/omapfb/dss/dsi.c > +++ b/drivers/video/fbdev/omap2/omapfb/dss/dsi.c [ ... ] > @@ -838,15 +836,10 @@ static irqreturn_t omap_dsi_irq_handler(int irq, void > *arg) > timer_delete(&dsi->te_timer); [Severity: High] This is a pre-existing issue, but does the driver fail to cancel this timer and related work queues during teardown? Looking at dsi_unbind(), it returns without cancelling pending work or timers: drivers/video/fbdev/omap2/omapfb/dss/dsi.c:dsi_unbind() { ... of_platform_depopulate(&dsidev->dev); WARN_ON(dsi->scp_clk_refcount > 0); dss_pll_unregister(&dsi->pll); dsi_uninit_output(dsidev); pm_runtime_disable(&dsidev->dev); ... } If framedone_timeout_work or te_timer expires after the driver is unbound and the devres subsystem frees the dsi structure, will this result in a use-after-free? > #endif > > - /* make a copy and unlock, so that isrs can unregister > - * themselves */ > - memcpy(&dsi->isr_tables_copy, &dsi->isr_tables, > - sizeof(dsi->isr_tables)); > > + dsi_handle_isrs(&dsi->isr_tables, irqstatus, vcstatus, ciostatus); > spin_unlock(&dsi->irq_lock); [Severity: High] This isn't a bug introduced by this patch, but it appears the identical use-after-free vulnerability you are fixing here remains unpatched in the equivalent DRM driver. Does drivers/gpu/drm/omapdrm/dss/dsi.c:omap_dsi_irq_handler() need the same fix? It still copies the table and drops the lock: drivers/gpu/drm/omapdrm/dss/dsi.c:omap_dsi_irq_handler() { ... memcpy(&dsi->isr_tables_copy, &dsi->isr_tables, sizeof(dsi->isr_tables)); spin_unlock(&dsi->irq_lock); dsi_handle_isrs(&dsi->isr_tables_copy, irqstatus, vcstatus, ciostatus); ... } This could still race with a timeout in dsi_sync_vc_vp(), which creates a completion on the stack: drivers/gpu/drm/omapdrm/dss/dsi.c:dsi_sync_vc_vp() { DECLARE_COMPLETION_ONSTACK(completion); ... } [Severity: High] This is also a pre-existing issue, but this same vulnerability pattern seems to be present in the DISPC hardware interrupt demultiplexer. Does drivers/video/fbdev/omap2/omapfb/dss/dispc-compat.c:omap_dispc_irq_handler() need to be updated? It makes a similar lockless copy: drivers/video/fbdev/omap2/omapfb/dss/dispc-compat.c:omap_dispc_irq_handler() { ... memcpy(registered_isr, dispc_compat.registered_isr, sizeof(registered_isr)); spin_unlock(&dispc_compat.irq_lock); ... } This could race with dispc_mgr_disable_lcd_out(), which registers a stack-allocated completion as an ISR and waits for a timeout: drivers/video/fbdev/omap2/omapfb/dss/dispc-compat.c:dispc_mgr_disable_lcd_out() { DECLARE_COMPLETION_ONSTACK(framedone_compl); ... } > > - dsi_handle_isrs(&dsi->isr_tables_copy, irqstatus, vcstatus, ciostatus); > - > dsi_handle_irq_errors(dsidev, irqstatus, vcstatus, ciostatus); > > dsi_collect_irq_stats(dsidev, irqstatus, vcstatus, ciostatus); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
