Re: [PATCHv2 2/4] serial: mxs-auart: rework clock handling in mxs_get_clks and probe

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unconditional clock enablement in `probe` for non-ASM9260 devices causes a resource leak on driver unbind and defeats dynamic clock gating.

Pre-existing issues:
- [Critical] Out-of-bounds array write in error and remove paths due to mismatched array indexing (`pdev->id` vs `s->port.line`).
- [Critical] Missing bounds check on hardware-provided RX count leads to an out-of-bounds read and information leak.
- [High] Missing explicit termination of in-flight DMA transfers causes Use-After-Free during shutdown/error.
--

commit 610c2d30ee113ae6d3222a8a5d42e3dc565ef441
Author: Rosen Penev <rosenp@gmail.com>

serial: mxs-auart: rework clock handling in mxs_get_clks and probe

This patch reworks the clock handling in the mxs-auart driver. It uses
devm_clk_get_enabled() for the AHB clock so its enable/disable lifetime
is managed by the driver model. It also moves the mod clock preparation
and enablement out of mxs_get_clks() and into the probe function.

> diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
> index 693b491f1e755..8d050ceb72d2f 100644
> --- a/drivers/tty/serial/mxs-auart.c
> +++ b/drivers/tty/serial/mxs-auart.c

[ ... ]

> @@ -1604,17 +1592,21 @@ static int mxs_auart_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	ret = clk_prepare_enable(s->clk);
> +	if (ret)
> +		return ret;
> +

[Severity: High]
This patch moves clk_prepare_enable(s->clk) from the ASM9260-specific portion
of mxs_get_clks() to mxs_auart_probe(), which causes it to execute for all
device variants.

However, mxs_auart_remove() was not updated to unconditionally disable the
clock. Does this leak the clock reference for non-ASM9260 devices on unbind?

Additionally, by holding a baseline clock reference from probe to remove,
might the clock's reference count never reach zero when the UART port is
closed? This would unconditionally keep the clock running and defeat the
driver's dynamic clock gating logic in mxs_auart_startup() and
mxs_auart_shutdown(), potentially preventing SoC deep sleep states.

[ ... ]

> @@ -1697,10 +1686,8 @@ static void mxs_auart_remove(struct platform_device *pdev)
>  	auart_port[pdev->id] = NULL;

[Severity: Critical]
This is a pre-existing issue, but I noticed a potential out-of-bounds array
write here.

During registration in mxs_auart_probe(), the allocated port pointer is
saved using the device tree alias index:

    auart_port[s->port.line] = s;

However, in the out_free_qpio_irq error path and here in mxs_auart_remove(),
the code clears the slot using auart_port[pdev->id] = NULL.

For devices instantiated from a Device Tree, pdev->id is typically -1
(PLATFORM_DEVID_NONE). Could writing to index -1 cause an out-of-bounds
memory write in the BSS segment immediately preceding the auart_port array?
Also, does this leave the actual active pointer dangling instead of clearing
it?

>  	mxs_auart_free_gpio_irq(s);
>  	iounmap(s->port.membase);
> -	if (is_asm9260_auart(s)) {
> +	if (is_asm9260_auart(s))
>  		clk_disable_unprepare(s->clk);
> -		clk_disable_unprepare(s->clk_ahb);
> -	}
>  }

[Severity: High]
This is a pre-existing issue, but I noticed a potential use-after-free
during shutdown or error handling in mxs_auart_dma_exit_channel():

    if (s->tx_dma_chan) {
        dma_release_channel(s->tx_dma_chan);
        s->tx_dma_chan = NULL;
    }
    ...
    kfree(s->tx_dma_buf);
    kfree(s->rx_dma_buf);

The driver fails to call dmaengine_terminate_sync() before releasing the
channels. Since releasing a channel does not safely abort active transfers,
if an asynchronous DMA transfer is in flight, could the DMA controller
continue operating and execute the DMA callback after the buffers have been
freed?

[Severity: Critical]
This is also a pre-existing issue, but does dma_rx_callback() lack a bounds
check on the hardware-provided RX count?

    count = stat & AUART_STAT_RXCOUNT_MASK;
    tty_insert_flip_string(port, s->rx_dma_buf, count);

The mask is 0xFFFF, allowing values up to 65535. The driver passes this
unfiltered count to tty_insert_flip_string(), which copies count bytes
from s->rx_dma_buf. 

Since s->rx_dma_buf is allocated with a fixed size of UART_XMIT_SIZE
(4096 bytes), could a hardware fault or a compromised peripheral reporting
a count larger than 4096 cause an out-of-bounds read, potentially copying
up to 61KB of kernel memory to userspace?

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