Re: [PATCHv3 5/6] serial: mxs-auart: clamp RX DMA count to buffer size

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Out-of-bounds array write to the global `auart_port` array during error handling and removal due to `pdev->id` being `-1`.
- [High] The driver frees DMA buffers and releases channels without first terminating active DMA transfers, leading to potential memory corruption or use-after-free in asynchronous callbacks.
- [High] If `mxs_auart_dma_tx()` fails, the error is ignored, leaving `MXS_AUART_DMA_TX_SYNC` set forever and locking up TX. Additionally, popped FIFO data is permanently lost.
- [Medium] Attempting to free an unallocated GPIO IRQ on error rollback triggers a kernel warning.
- [Medium] DMA mappings are leaked in `mxs_auart_dma_prep_rx()` and `mxs_auart_dma_tx()` if the DMA engine descriptor preparation fails, and `dma_map_sg()` return values are unchecked.
--

commit 5c41f469d68450e9f6b63b5ed62412e5d03637ef
Author: Rosen Penev <rosenp@gmail.com>

serial: mxs-auart: clamp RX DMA count to buffer size

In dma_rx_callback(), the rx count from the hardware status register
is passed directly to tty_insert_flip_string() without any bounds check.
Since rx_dma_buf is allocated with the transmit size, a hardware fault or
compromised peripheral reporting a large count would cause an out-of-bounds
read. This clamps the count before use to prevent leaking kernel memory.

> diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
> index 693b491f1e755..7e5348e5d63f4 100644
> --- a/drivers/tty/serial/mxs-auart.c
> +++ b/drivers/tty/serial/mxs-auart.c
> @@ -819,7 +819,7 @@ static void dma_rx_callback(void *arg)
>  	stat &= ~(AUART_STAT_OERR | AUART_STAT_BERR |
>  			AUART_STAT_PERR | AUART_STAT_FERR);
>  
> -	count = stat & AUART_STAT_RXCOUNT_MASK;
> +	count = min_t(u32, stat & AUART_STAT_RXCOUNT_MASK, UART_XMIT_SIZE);
>  	tty_insert_flip_string(port, s->rx_dma_buf, count);

[Severity: High]
This is a pre-existing issue, but does this callback risk a use-after-free
if a dma transfer is in progress during shutdown?

When the uart is shut down, mxs_auart_dma_exit_channel() frees the buffers:

mxs_auart_dma_exit_channel() {
    ...
    kfree(s->tx_dma_buf);
    kfree(s->rx_dma_buf);
    ...
}

It appears this happens without calling dmaengine_terminate_sync() or
dmaengine_terminate_all(). Could a hardware dma controller continue writing
to the freed memory, or a pending tasklet invoke dma_rx_callback() and
dereference s->rx_dma_buf after it is freed?

>  
>  	mxs_write(stat, s, REG_STAT);

[Severity: High]
This is a pre-existing issue, but can the global auart_port array be
corrupted during error handling or removal?

When instantiated via device tree, pdev->id is implicitly -1. In
mxs_auart_probe(), the port is safely registered using its device tree
alias id:

mxs_auart_probe() {
    ...
    auart_port[s->port.line] = s;
    ...
}

However, the cleanup path and mxs_auart_remove() attempt to clear the
array entry using pdev->id:

mxs_auart_probe() {
    ...
out_free_qpio_irq:
    auart_port[pdev->id] = NULL;
}

Could this result in an out-of-bounds write to the array at index -1,
corrupting adjacent memory?

[Severity: Medium]
This is a pre-existing issue, but does the rollback loop in
mxs_auart_request_gpio_irq() attempt to free an unallocated irq?

When request_irq() fails, the loop increments before exiting:

mxs_auart_request_gpio_irq() {
    ...
    for (i = 0; !err && i < UART_GPIO_MAX; i++) {
        ...
        err = request_irq(irq[i], ...);
    }
    ...
}

The subsequent cleanup loop then decrements:

    while (err && (i-- > 0))
        if (irq[i] >= 0)
            free_irq(irq[i], s);

If request_irq() fails for index N, the for loop leaves i at N + 1. The
cleanup loop's first iteration operates on i = N, calling free_irq() on the
interrupt that just failed to request. Does this trigger a warning for
trying to free an already-free irq?

[Severity: Medium]
This is a pre-existing issue, but do mxs_auart_dma_prep_rx() and
mxs_auart_dma_tx() leak dma mappings on failure?

In both functions, dma_map_sg() is called without checking its return value.
If dmaengine_prep_slave_sg() subsequently fails, the functions return
early:

mxs_auart_dma_prep_rx() {
    ...
    dma_map_sg(...);
    desc = dmaengine_prep_slave_sg(...);
    if (!desc) {
        dev_err(s->dev, "step 2 error\n");
        return -1;
    }
    ...
}

Should these failure paths call dma_unmap_sg() to prevent leaking mapping
entries and address space?

[Severity: High]
This is a pre-existing issue, but does ignoring the return value of
mxs_auart_dma_tx() risk permanently locking up transmit?

In mxs_auart_tx_chars(), data is popped from the fifo before calling
mxs_auart_dma_tx():

mxs_auart_tx_chars() {
    ...
    i = kfifo_out(&port->state->xmit.fifo, ...);
    if (i) {
        mxs_auart_dma_tx(s, i);
    } else {
    ...
}

If mxs_auart_dma_tx() fails and returns an error, mxs_auart_tx_chars()
ignores the return value and leaves the transmit sync flag set.
Will subsequent calls to mxs_auart_tx_chars() return immediately,
locking up the transmit path? Is the data popped from the fifo also
permanently lost?

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