Re: [PATCH v2] i2c: mxs: fix DMA channel leak on probe error

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] On a DMA transfer timeout, the active DMA bounce buffer is improperly freed before unmapping and terminating the DMA engine, leading to a use-after-free and potential memory corruption.
- [High] The DMA completion callback signals the waiting thread before unmapping the DMA buffer, causing a race condition where the CPU may read stale data and the buffer may be freed before unmapping completes.
--

--- Patch [1]: [PATCH v2] i2c: mxs: fix DMA channel leak on probe error ---
commit 3a30f1ffa0258833921bc6e721cb282f66cc45c7
Author: Ruoyu Wang <ruoyuw560@gmail.com>
i2c: mxs: fix DMA channel leak on probe error

This commit fixes a DMA channel leak in mxs_i2c_probe() by using
devm_dma_request_chan(). This ensures the device core releases the
channel on probe failure and driver detach, removing the need for
manual release in the remove callback.

> diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
> index 4e07babea9c3f..eee4fdcd9df31 100644
> --- a/drivers/i2c/busses/i2c-mxs.c
> +++ b/drivers/i2c/busses/i2c-mxs.c
> @@ -839,7 +839,7 @@ static int mxs_i2c_probe(struct platform_device *pdev)
>  	}
>  
>  	/* Setup the DMA */
> -	i2c->dmach = dma_request_chan(dev, "rx-tx");
> +	i2c->dmach = devm_dma_request_chan(dev, "rx-tx");

[Severity: High]
This is a pre-existing issue, but does the timeout path in
mxs_i2c_xfer_msg() free the active DMA bounce buffer before
terminating the DMA engine?

In mxs_i2c_xfer_msg(), if the completion times out:

		time_left = wait_for_completion_timeout(&i2c->cmd_complete,
						msecs_to_jiffies(1000));
		i2c_put_dma_safe_msg_buf(dma_buf, msg, true);
		if (!time_left)
			goto timeout;

The bounce buffer is freed by i2c_put_dma_safe_msg_buf(). The code
then jumps to the timeout label, which calls mxs_i2c_dma_finish() to
unmap the scatterlist but does not call dmaengine_terminate_sync().

Could this leave the DMA hardware active and able to write into the
freed memory?

Additionally, a late DMA completion could cause
mxs_i2c_dma_irq_callback() to execute, resulting in a second call to
mxs_i2c_dma_finish() and a double-unmap of the scatterlist.

[Severity: High]
This is also a pre-existing issue, but does the DMA completion
callback signal the waiting thread before unmapping the DMA buffer?

Looking at mxs_i2c_dma_irq_callback():

static void mxs_i2c_dma_irq_callback(void *param)
{
	struct mxs_i2c_dev *i2c = param;

	complete(&i2c->cmd_complete);
	mxs_i2c_dma_finish(i2c);
}

By calling complete() before mxs_i2c_dma_finish(), the waiting thread
in mxs_i2c_xfer_msg() is awakened and proceeds to call
i2c_put_dma_safe_msg_buf().

Since the unmap has not occurred yet, the CPU cache may not be
invalidated. Could this result in the CPU reading stale cached data
during the memcpy from the bounce buffer instead of the actual data
received from the bus?

>  	if (IS_ERR(i2c->dmach)) {
>  		return dev_err_probe(dev, PTR_ERR(i2c->dmach),
>  				     "Failed to request dma\n");

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260815151720.3757460-1-ruoyuw560@gmail.com?part=1