Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Race condition leading to uninitialized mutex usage in bridge attach path. Pre-existing issues: - [High] Use-After-Free of bridge resources on component removal while DRM device is open. -- --- Patch [2]: [PATCH v10 02/21] drm/bridge: synopsys: dw-dp: Fix incorrect resource lifetimes in bind callback --- commit add1fb610af93986b5b39118795cddc285343d20 Author: Sebastian Reichel <[email protected]> drm/bridge: synopsys: dw-dp: Fix incorrect resource lifetimes in bind callback Currently the driver requests resources in the bind function which are freed on driver removal instead of unbind. This introduces a probe helper to fix the discrepancy and keeps interrupts disabled while detached to prevent spurious worker wakeups. Fixes: 86eecc3a9c2e ("drm/bridge: synopsys: Add DW DPTX Controller support library") Reported-by: Sashiko <[email protected]> > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c > b/drivers/gpu/drm/bridge/synopsys/dw-dp.c > index 112a499113095..f8ca464d43717 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c > @@ -1826,10 +1826,19 @@ static int dw_dp_bridge_attach(struct drm_bridge > *bridge, > dp->aux.name = dev_name(dev); > dp->aux.transfer = dw_dp_aux_transfer; > > + enable_irq(dp->irq); > + > ret = drm_dp_aux_register(&dp->aux); [Severity: High] Can this race with the interrupt handler? If the display is already plugged in and the interrupt line is asserted, dw_dp_irq() might fire immediately after enable_irq() is called. This schedules the dp->hpd_work worker. The worker function dw_dp_hpd_work() can then execute before drm_dp_aux_register() completes. It calls dw_dp_link_retrain(), which in turn calls drm_dp_dpcd_read_link_status(). That function acquires the aux->hw_mutex lock. Since hw_mutex is exclusively initialized inside drm_dp_aux_init(), which is called by drm_dp_aux_register(), could the worker attempt to lock an uninitialized mutex here? > - if (ret) > + if (ret) { > dev_err(dev, "Aux register failed: %d\n", ret); > + goto err_disable_irq; > + } > + > + return 0; > > +err_disable_irq: > + disable_irq(dp->irq); > + cancel_work_sync(&dp->hpd_work); > return ret; > } > > @@ -1838,6 +1847,8 @@ static void dw_dp_bridge_detach(struct drm_bridge > *bridge) > struct dw_dp *dp = bridge_to_dp(bridge); > > drm_dp_aux_unregister(&dp->aux); > + disable_irq(dp->irq); > + cancel_work_sync(&dp->hpd_work); > } [Severity: High] This is a pre-existing issue, but does this lead to a use-after-free on component removal? The dw_dp structure is allocated using devm_drm_bridge_alloc in dw_dp_probe(), tying its lifetime to the component platform device. If the component is unbound, its devm resources are immediately freed when the remove function finishes. However, if a userspace application holds the DRM device file descriptor open, the overarching DRM device is not torn down immediately. When userspace eventually closes the file descriptor, drm_encoder_cleanup() traverses the bridge chain and calls dw_dp_bridge_detach(). Does this cause dw_dp_bridge_detach() to access the already freed dp pointer when it tries to unregister the aux channel, disable the irq, and cancel the work? [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260806-synopsys-dw-dp-improvements-v10-0-d84a26c30...@collabora.com?part=2
