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

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

Pre-existing issues:
- [High] The driver maps client-provided `msg->buf` directly for DMA without using `i2c_get_dma_safe_msg_buf()`, leading to potential memory corruption.
- [High] The driver ignores the return value of `dma_map_sg()`, risking DMA engine faults and memory corruption if mapping fails.
- [High] The `timeout` error path in `mxs_i2c_xfer_msg()` unmaps DMA buffers without terminating the active DMA channel, leading to potential use-after-unmap and double-unmap bugs.
--

--- Patch [1]: [PATCH] i2c: mxs: fix DMA channel leak on probe error ---
commit b8eb950d1bfe51582e811624063eaccb540a465b
Author: Ruoyu Wang <[email protected]>

i2c: mxs: fix DMA channel leak on probe error

This commit fixes a DMA channel leak when mxs_i2c_probe() fails during
controller reset or adapter registration. It ensures dma_release_channel()
is called on these error paths.

> diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
> --- a/drivers/i2c/busses/i2c-mxs.c
> +++ b/drivers/i2c/busses/i2c-mxs.c
[ ... ]
> @@ -197,6 +197,7 @@ static void mxs_i2c_dma_setup_xfer(struct i2c_adapter *adap,
>  
>  		/* Queue the DMA data transfer. */
>  		sg_init_one(&i2c->sg_io[0], &i2c->addr_data, 1);
>  		dma_map_sg(i2c->dev, &i2c->sg_io[0], 1, DMA_TO_DEVICE);

[Severity: High]
This isn't a bug introduced by this patch, but does this code ignore the
return value of dma_map_sg()?

If mapping fails under memory pressure or IOMMU fragmentation, dma_map_sg()
will return 0, and the unmapped scatterlist will be queued to the DMA engine,
which could lead to DMA hardware accessing invalid memory.

This pattern also occurs later in mxs_i2c_dma_setup_xfer() for both the read
and write transfer mapping paths.

[ ... ]
> @@ -227,6 +227,7 @@ static void mxs_i2c_dma_setup_xfer(struct i2c_adapter *adap,
>  
>  		/* Queue the DMA data transfer. */
>  		sg_init_one(&i2c->sg_io[1], msg->buf, msg->len);
>  		dma_map_sg(i2c->dev, &i2c->sg_io[1], 1, DMA_FROM_DEVICE);

[Severity: High]
This is a pre-existing issue, but is it safe to map the client-provided
msg->buf directly for DMA here?

The I2C subsystem allows clients to pass stack or vmalloc buffers for
standard transfers. Without allocating a bounce buffer via
i2c_get_dma_safe_msg_buf(), this could result in direct DMA into stack
memory and cause memory corruption.

This direct mapping also occurs in the write transfer path below.

[ ... ]
> @@ -629,6 +634,7 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg,
>  timeout:
>  	dev_dbg(i2c->dev, "Timeout!\n");
>  	mxs_i2c_dma_finish(i2c);

[Severity: High]
This is a pre-existing issue, but does the timeout handler fail to terminate
the pending DMA transaction?

Since dmaengine_terminate_sync() is not called after a timeout, the active
DMA engine might continue accessing the unmapped memory.

If it eventually finishes, mxs_i2c_dma_irq_callback() could run and call
mxs_i2c_dma_finish() a second time on the same buffers.

>  	ret = mxs_i2c_reset(i2c);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1