Re: [PATCH v3 3/3] PCI: epf-mhi: Implement mhi_cntrl->flush_async() to flush DMA read/write

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] The newly introduced `pci_epf_mhi_edma_flush_async` callback unconditionally accesses `dma_chan_rx` and `dma_chan_tx` without verification, leading to fatal crashes if DMA initialization fails or the PCIe link toggles.
- [High] `pci_epf_mhi_edma_flush_async()` misuses `dmaengine_synchronize()` by not terminating in-flight transfers, invoking undefined behavior and leading to Use-After-Free when hardware completes transfers after teardown.

Pre-existing issues:
- [High] Teardown paths in `pci_epf_mhi_unbind()`, `pci_epf_mhi_epc_deinit()`, and `pci_epf_mhi_link_down()` are vulnerable to a severe Use-After-Free (UAF) and Double-Free due to dangling pointers.
--

--- Patch [3]: [PATCH v3 3/3] PCI: epf-mhi: Implement mhi_cntrl->flush_async() to flush DMA read/write ---
commit e8cb88746eba8db4d5212c60aa7ddf4b58afafd5
Author: Manivannan Sadhasivam <mani@kernel.org>

PCI: epf-mhi: Implement mhi_cntrl->flush_async() to flush DMA read/write

The MHI core needs to make sure that all the current DMA transactions are
completed before removing the channels. So implement the
mhi_cntrl->flush_async() callback by first making sure all the in-flight
DMA operations are completed and then flushing the DMA workqueue.

> diff --git a/drivers/pci/endpoint/functions/pci-epf-mhi.c b/drivers/pci/endpoint/functions/pci-epf-mhi.c
> --- a/drivers/pci/endpoint/functions/pci-epf-mhi.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-mhi.c
> @@ -644,6 +644,15 @@ static int pci_epf_mhi_edma_write_async(struct mhi_ep_cntrl *mhi_cntrl,
>  	return ret;
>  }
>  
> +static void pci_epf_mhi_edma_flush_async(struct mhi_ep_cntrl *mhi_cntrl)
> +{
> +	struct pci_epf_mhi *epf_mhi = to_epf_mhi(mhi_cntrl);
> +
> +	dmaengine_synchronize(epf_mhi->dma_chan_rx);
> +	dmaengine_synchronize(epf_mhi->dma_chan_tx);

[Severity: High]
If pci_epf_mhi_dma_init() fails during epc_init(), the DMA channels are not
initialized, but the flush_async callback is still registered later in
pci_epf_mhi_link_up().  Will this unconditionally dereference dma_chan_rx
and dma_chan_tx and cause a crash if the link goes down?

Additionally, if the host toggles the PCIe link down then up,
pci_epf_mhi_link_down() destroys the channels and sets them to NULL, but the
subsequent link_up does not reinitialize them.  Does this also lead to a
NULL pointer dereference here?

[Severity: High]
Does dmaengine_synchronize() safely stop the hardware or wait for pending
descriptors to execute?

The DMA Engine API states that dmaengine_synchronize() synchronizes only the
termination of a channel and does not stop the hardware.

If dmaengine_terminate_async() is never called, will this return while
hardware DMA is still processing in the background?  When the active DMA
transactions eventually finish, the hardware might write to unmapped or freed
memory, potentially causing memory corruption and enqueuing a freed transfer
context to the dma_wq.

> +	flush_workqueue(epf_mhi->dma_wq);
> +}
> +
[ ... ]
> @@ -812,6 +821,7 @@ static int pci_epf_mhi_link_up(struct pci_epf *epf)
>  		mhi_cntrl->write_sync = pci_epf_mhi_edma_write;
>  		mhi_cntrl->read_async = pci_epf_mhi_edma_read_async;
>  		mhi_cntrl->write_async = pci_epf_mhi_edma_write_async;
> +		mhi_cntrl->flush_async = pci_epf_mhi_edma_flush_async;
>  	}
>  
>  	/* Register the MHI EP controller */

[Severity: High]
This is a pre-existing issue, but teardown paths appear vulnerable to a
double-free or use-after-free.

If the endpoint is unbound via pci_epf_mhi_unbind() after a link down event,
the teardown logic executes again:

pci_epf_mhi_unbind() {
    ...
    if (mhi_cntrl->mhi_dev) {
        mhi_ep_power_down(mhi_cntrl);
        if (info->flags & MHI_EPF_USE_DMA)
            pci_epf_mhi_dma_deinit(epf_mhi);
        mhi_ep_unregister_controller(mhi_cntrl);
    }
    ...
}

Since mhi_ep_unregister_controller() and pci_epf_mhi_dma_deinit() do not set
mhi_cntrl->mhi_dev to NULL or clear out the destroyed workqueues, will this
conditional evaluate to true on a dangling pointer and run the teardown
logic a second time, resulting in a double-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722-mhi-ep-flush-v3-0-d855e715264e@oss.qualcomm.com?part=3