Re: [PATCH v3 1/3] bus: mhi: ep: Add mhi_cntrl->flush_async() callback to flush the async read/write

[sashiko-bot@kernel.org]

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

New issues:
- [High] Lock gap in `mhi_ep_remove()` during `flush_async()` allows the host to restart the channel and bypass teardown synchronization.
- [High] Memory leak of TRE buffers and SKBs if `flush_async` cancels DMA operations.

Pre-existing issues:
- [High] Missing `flush_async()` in `mhi_ep_abort_transfer()`, `STOP_CHAN`, and `RESET_CHAN` leaves UAF and introduces NULL pointer dereference.
- [High] Inconsistent locking for `xfer_cb` leading to potential AB-BA deadlocks in client drivers.
- [High] Use-Before-Initialization race condition due to early `xfer_cb` publication in `probe()`.
- [High] Invalid Memory Free (`kfree()` used on a `kmem_cache` object).
- [Medium] Duplicate `-ENOTCONN` notifications sent to client driver.
--

--- Patch [1]: [PATCH v3 1/3] bus: mhi: ep: Add mhi_cntrl->flush_async() callback to flush the async read/write ---
commit 8e4fde6115727693f6dc8095e55dc40c2cd91987
Author: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

bus: mhi: ep: Add mhi_cntrl->flush_async() callback to flush the async read/write

This commit introduces a flush_async callback to the MHI endpoint controller
and calls it during device removal. This ensures that any pending asynchronous
DMA read or write operations are completed before disconnecting the channels.
The change aims to prevent use-after-free issues that occur when DMA
callbacks trigger after channel teardown.

> diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c
> index 0277e1ab11988..b94c571f01d88 100644
> --- a/drivers/bus/mhi/ep/main.c
> +++ b/drivers/bus/mhi/ep/main.c
> @@ -1620,6 +1621,22 @@ static void mhi_ep_remove(struct device *dev)
>  	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
>  		return;
>  
> +	/* Disable the channels to prevent new transfers */
> +	for (dir = 0; dir < 2; dir++) {
> +		mhi_chan = dir ? mhi_dev->ul_chan : mhi_dev->dl_chan;
> +
> +		if (!mhi_chan)
> +			continue;
> +
> +		mutex_lock(&mhi_chan->lock);
> +		mhi_chan->state = MHI_CH_STATE_DISABLED;
> +		mutex_unlock(&mhi_chan->lock);
> +	}
> +
> +	/* Flush in-flight transfers before notifying disconnect */
> +	if (mhi_cntrl->flush_async)
> +		mhi_cntrl->flush_async(mhi_cntrl);

[Severity: High]
Does dropping the channel locks while calling the blocking flush_async
callback leave a window where the host could restart the channel?

If a START_CHAN command is processed during this gap,
mhi_ep_process_cmd_ring() could reacquire the lock and set the state back to
MHI_CH_STATE_RUNNING. This would allow new DMA operations to be queued
before the xfer_cb is cleared later in this function, potentially leading to
a use-after-free.

[Severity: High]
If the controller implements flush_async() by cancelling pending transfers
(for example, via dmaengine_terminate_async()), could this cause a memory
leak?

Since the MHI EP core doesn't appear to track pending operations, abandoning
the buffers allocated from tre_buf_cache without invoking their completion
callbacks might leak both the TRE buffers and associated SKBs.

[Severity: High]
This is a pre-existing issue, but since flush_async() is being added here to
handle pending transfers, should it also be called when the host resets or
stops a channel?

During host commands like RESET_CHAN or STOP_CHAN, or during power down via
mhi_ep_abort_transfer(), channels are disabled without flushing pending DMA
transfers. For instance, in mhi_ep_process_cmd_ring() handling a RESET_CHAN,
mhi_ep_ring_reset() frees ring->ring_cache and sets it to NULL. If an
in-flight async DMA read subsequently completes, mhi_ep_read_completion()
will dereference this NULL ring_cache.

>  	/* Disconnect the channels associated with the driver */
>  	for (dir = 0; dir < 2; dir++) {
>  		mhi_chan = dir ? mhi_dev->ul_chan : mhi_dev->dl_chan;
> @@ -1635,7 +1652,6 @@ static void mhi_ep_remove(struct device *dev)
>  			mhi_chan->xfer_cb(mhi_chan->mhi_dev, &result);
>  		}

[Severity: Medium]
This is a pre-existing issue, but does this lead to duplicate -ENOTCONN
notifications being sent to the client driver during power down?

When mhi_ep_abort_transfer() is called, it sends an initial -ENOTCONN via
xfer_cb but leaves the callback set. Device destruction then synchronously
invokes mhi_ep_remove(), which finds the callback still valid and sends a
second -ENOTCONN notification.

[Severity: High]
This isn't a bug introduced by this patch, but is there an inconsistent
locking expectation for xfer_cb that could lead to deadlocks in client
drivers?

The callback is invoked with mhi_chan->lock held in mhi_ep_process_ch_ring(),
but it is called locklessly from the workqueue in mhi_ep_skb_completion().
If a client driver responds to a receive callback by queueing a new buffer
via mhi_ep_queue_skb() (which acquires mhi_chan->lock), it could self-deadlock
when called from the locked context.

[Severity: High]
This isn't a bug introduced by this patch, but could the xfer_cb be called
before the client driver has finished initializing?

In mhi_ep_probe(), xfer_cb is assigned to the channel before the client
driver's probe function completes:

mhi_ep_probe() {
    ...
    ul_chan->xfer_cb = mhi_drv->ul_xfer_cb;
    dl_chan->xfer_cb = mhi_drv->dl_xfer_cb;

    return mhi_drv->probe(mhi_dev, mhi_dev->id);
}

If the host sends an early doorbell interrupt, mhi_ep_ch_ring_worker() could
start processing the ring and invoke the callback on a client structure that
is not fully initialized.

[Severity: High]
This is a pre-existing issue, but is kfree() being used incorrectly on an
object allocated from a slab cache?

In mhi_ep_ch_ring_worker(), if the ring is stopped concurrently, the iterator
is freed using kfree():

mhi_ep_ch_ring_worker() {
    ...
    if (!ring->started) {
        mutex_unlock(&chan->lock);
        kfree(itr);
        continue;
    }
}

However, the iterator was allocated from the mhi_cntrl->ring_item_cache via
kmem_cache_zalloc(), so this might cause slab memory corruption.

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