Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues: - [High] Moving work cancellations to the beginning of the remove function creates a Use-After-Free race condition where DRM atomic hooks can re-queue the works. Pre-existing issues: - [Critical] Stack buffer overflow in `cdns_mhdp_hdcp_check_receviers` due to missing bounds check on `recv_num`. - [High] The asynchronous firmware loading callback `cdns_mhdp_fw_cb` can execute after device teardown has completed, causing a use-after-free, accessing unmapped memory, and locking a destroyed mutex. - [High] `cdns_mhdp_modeset_retry_fn` dereferences `mhdp->connector` without checking for NULL, leading to a kernel crash if the bridge is disabled before the work executes. - [Medium] Overwriting `platform_get_irq()` return value breaks deferred probing. -- --- Patch [1]: [PATCH] drm/bridge: cdns-mhdp8546: cancel work before teardown --- commit aa3183d71b3121eab38e232fed56925730e48ce8 Author: Hongyan Xu <[email protected]> Subject: drm/bridge: cdns-mhdp8546: cancel work before teardown This commit attempts to fix race conditions during module removal by masking interrupts and cancelling work items before tearing down the DRM bridge and hardware state. > diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c [ ... ] > @@ -2338,12 +2337,12 @@ static int cdns_mhdp_probe(struct platform_device > *pdev) > > writel(~0, mhdp->regs + CDNS_APB_INT_MASK); > > - irq = platform_get_irq(pdev, 0); > - ret = devm_request_threaded_irq(mhdp->dev, irq, NULL, > + mhdp->irq = platform_get_irq(pdev, 0); > + ret = devm_request_threaded_irq(mhdp->dev, mhdp->irq, NULL, > cdns_mhdp_irq_handler, IRQF_ONESHOT, > "mhdp8546", mhdp); > if (ret) { > - dev_err(dev, "cannot install IRQ %d\n", irq); > + dev_err(dev, "cannot install IRQ %d\n", mhdp->irq); > ret = -EIO; > goto plat_fini; > } [Severity: Medium] This is a pre-existing issue, but does overwriting the return value of platform_get_irq() break deferred probing? If platform_get_irq() returns -EPROBE_DEFER, the negative value causes devm_request_threaded_irq() to fail. The error path then unconditionally overwrites the error with -EIO instead of propagating -EPROBE_DEFER, which might permanently prevent the driver from probing if the IRQ provider is initialized later. [ ... ] > @@ -2406,6 +2405,19 @@ static void cdns_mhdp_remove(struct platform_device > *pdev) > unsigned long timeout = msecs_to_jiffies(100); > int ret; > > + spin_lock(&mhdp->start_lock); > + mhdp->bridge_attached = false; > + spin_unlock(&mhdp->start_lock); > + writel(~0, mhdp->regs + CDNS_APB_INT_MASK); > + devm_free_irq(mhdp->dev, mhdp->irq, mhdp); > + > + cancel_work_sync(&mhdp->hpd_work); > + cancel_work_sync(&mhdp->modeset_retry_work); [Severity: High] Can moving these work cancellations to the beginning of the remove function create a race condition where DRM atomic hooks re-queue the work? Because drm_bridge_remove() has not yet been called at this point, concurrent DRM operations might invoke cdns_mhdp_atomic_enable(). If an error occurs there, the code schedules the work again: cdns_mhdp_atomic_enable() { ... if (ret < 0) schedule_work(&mhdp->modeset_retry_work); ... } Since cdns_mhdp_atomic_enable() does not check the newly added mhdp->bridge_attached flag, could this re-queue the work after the cancellation completes, causing it to access freed memory once the remove function finishes? [Severity: High] This isn't a bug introduced by this patch, but while looking at modeset_retry_work, can the work function dereference a NULL pointer? If cdns_mhdp_atomic_enable() fails and queues mhdp->modeset_retry_work, and the display pipeline is then disabled before the work executes: cdns_mhdp_atomic_disable() { ... mhdp->connector = NULL; } The pending work is not cancelled. When cdns_mhdp_modeset_retry_fn() eventually executes, it behaves like this: cdns_mhdp_modeset_retry_fn() { conn = mhdp->connector; ... mutex_lock(&conn->dev->mode_config.mutex); ... } Will this crash when dereferencing conn->dev if the connector is NULL? > + if (mhdp->hdcp_supported) { > + cancel_delayed_work_sync(&mhdp->hdcp.check_work); > + cancel_work_sync(&mhdp->hdcp.prop_work); > + } > + > drm_bridge_remove(&mhdp->bridge); > > ret = wait_event_timeout(mhdp->fw_load_wq, [ ... ] > @@ -2432,10 +2444,6 @@ static void cdns_mhdp_remove(struct platform_device > *pdev) > > pm_runtime_put_sync(&pdev->dev); > pm_runtime_disable(&pdev->dev); > - > - cancel_work_sync(&mhdp->modeset_retry_work); > - flush_work(&mhdp->hpd_work); > - /* Ignoring mhdp->hdcp.check_work and mhdp->hdcp.prop_work here. */ > } [Severity: High] This is a pre-existing issue, but can the asynchronous firmware loading callback execute after the device teardown has completed? In cdns_mhdp_remove(), the driver waits for firmware loading with a timeout: cdns_mhdp_remove() { ... ret = wait_event_timeout(mhdp->fw_load_wq, mhdp->hw_state == MHDP_HW_READY, timeout); ... } If the firmware load takes longer than the 100ms timeout, the driver proceeds with teardown and devres frees the mhdp structure. When the firmware eventually loads, cdns_mhdp_fw_cb() executes: cdns_mhdp_fw_cb() { struct cdns_mhdp_device *mhdp = context; dev_dbg(mhdp->dev, "firmware callback\n"); ... } Does this result in a use-after-free by accessing the freed mhdp structure and unmapped memory? [Severity: Critical] This is a pre-existing issue, but does cdns_mhdp_hdcp_check_receviers() suffer from a stack buffer overflow? The function allocates a fixed-size stack array for receiver IDs: cdns_mhdp_hdcp_check_receviers() { u8 hdcp_rec_id[HDCP_MAX_RECEIVERS][HDCP_RECEIVER_ID_SIZE_BYTES]; ... } It then calls cdns_mhdp_hdcp_rx_id_valid(), which reads recv_num directly from the firmware mailbox and copies data into the caller's stack buffer without bounds checking: cdns_mhdp_hdcp_rx_id_valid() { ... *recv_num = rec_id_hdr[0]; ret = cdns_mhdp_secure_mailbox_recv_data(mhdp, hdcp_rx_id, 5 * *recv_num); ... } If a compromised sink device or firmware returns an unexpectedly large recv_num (which is an 8-bit value up to 255), does this overflow the 160-byte hdcp_rec_id array on the stack? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
