Re: [PATCHv3 6/6] serial: mxs-auart: terminate DMA before releasing channels in exit

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Terminating DMA transfers prevents their completion callbacks from executing, permanently leaking DMA scatterlist mappings.
- [High] The MXS_AUART_DMA_ENABLED flag is cleared too late, allowing concurrent callbacks and transmit operations to trigger a use-after-free and NULL pointer dereference on DMA channels/buffers.
- [High] Missing port lock in `mxs_auart_settermios` causes a read-modify-write race on shared hardware state.
- [High] DMA mappings are leaked on error paths in `mxs_auart_dma_tx` and `mxs_auart_dma_prep_rx`.
--

commit 0b036d5417b4eadfea5ff0f7490fcbdee3bda8bf
Author: Rosen Penev <rosenp@gmail.com>

serial: mxs-auart: terminate DMA before releasing channels in exit

The commit adds dmaengine_terminate_sync() to safely abort pending transfers
before releasing channels and freeing buffers, fixing a potential
use-after-free.

> diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
> index 693b491f1e755..a853fb908591f 100644
> --- a/drivers/tty/serial/mxs-auart.c
> +++ b/drivers/tty/serial/mxs-auart.c
> @@ -868,10 +868,12 @@ static int mxs_auart_dma_prep_rx(struct mxs_auart_port *s)
>  static void mxs_auart_dma_exit_channel(struct mxs_auart_port *s)
>  {
>  	if (s->tx_dma_chan) {
> +		dmaengine_terminate_sync(s->tx_dma_chan);

[Severity: High]
This isn't a bug introduced by this patch, but does terminating the transfers
here lead to a leak of DMA scatterlist mappings?

The driver maps the DMA buffers using dma_map_sg() when preparing RX and TX
transfers, and only unmaps them inside the completion callbacks
(dma_rx_callback and dma_tx_callback).

Because dmaengine_terminate_sync() explicitly aborts pending transfers, the
dmaengine API guarantees that their completion callbacks will not be invoked.
As a result, the dma_unmap_sg() calls are skipped, leading to a persistent
leak of DMA mapping resources (e.g., IOVA space or SWIOTLB bounce buffers)
every time active transfers are terminated.

Furthermore, is it safe to call kfree() on memory while it is still actively
mapped via the DMA API?

>  		dma_release_channel(s->tx_dma_chan);
>  		s->tx_dma_chan = NULL;
>  	}
>  	if (s->rx_dma_chan) {
> +		dmaengine_terminate_sync(s->rx_dma_chan);
>  		dma_release_channel(s->rx_dma_chan);
>  		s->rx_dma_chan = NULL;
>  	}

[ ... ]

[Severity: High]
This is a pre-existing issue, but does clearing the MXS_AUART_DMA_ENABLED flag
after freeing the buffers open a window for a use-after-free and NULL pointer
dereference?

In mxs_auart_dma_exit(), mxs_auart_dma_exit_channel() is executed before the
MXS_AUART_DMA_ENABLED flag is cleared:

mxs_auart_dma_exit() {
    ...
    mxs_auart_dma_exit_channel(s);
    s->flags &= ~MXS_AUART_DMA_ENABLED;
}

Because this teardown is not protected by the port spinlock and can be invoked
dynamically via mxs_auart_settermios(), it can race with concurrent transmit
operations.

Specifically, mxs_auart_tx_chars() (called under the port lock) checks
auart_dma_enabled(s), which will incorrectly return true. It then attempts to
access the freed s->tx_dma_buf and initiates a transfer on the NULL
s->tx_dma_chan via mxs_auart_dma_tx().

Should the MXS_AUART_DMA_ENABLED flag be cleared under the port spinlock
before dismantling the DMA channels and freeing the buffers?


[Severity: High]
This is a pre-existing issue, but is there a read-modify-write race on shared
hardware state in mxs_auart_settermios()?

The serial core calls the driver's .set_termios callback without holding the
port spinlock. The mxs_auart_settermios() function reads the shared hardware
register REG_CTRL2, modifies it locally, and later writes the full 32-bit
value back without holding uart_port_lock(&s->port):

mxs_auart_settermios() {
    ...
    ctrl2 = mxs_read(s, REG_CTRL2);
    ...
    mxs_write(ctrl2, s, REG_CTRL2);
}

If a concurrent context like mxs_auart_start_tx() or mxs_auart_stop_tx()
(which correctly hold the port lock) uses hardware-atomic bit operations
to enable or disable the transmitter during this window,
mxs_auart_settermios() will overwrite REG_CTRL2 with a stale cached value.

Can this unintentionally undo concurrent hardware changes and lead to a stalled
transmitter or receiver?


[Severity: High]
This is a pre-existing issue, but do the error paths in mxs_auart_dma_tx()
and mxs_auart_dma_prep_rx() leak DMA mappings?

The driver maps its DMA buffers via dma_map_sg() in both functions. If the
subsequent calls to dmaengine_prep_slave_sg() fail (e.g., due to descriptor
exhaustion), the driver immediately returns an error without calling
dma_unmap_sg():

mxs_auart_dma_tx() {
    ...
    dma_map_sg(s->dev, sgl, 1, DMA_TO_DEVICE);
    desc = dmaengine_prep_slave_sg(channel, sgl,
        1, DMA_MEM_TO_DEV, DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
    if (!desc) {
        dev_err(s->dev, "step 2 error\n");
        return -EINVAL;
    }
    ...
}

Does this permanently leak the DMA mappings (such as IOMMU entries or SWIOTLB
bounce buffers) every time the preparation fails?

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