Re: [PATCH 1/5] tty: serial: samsung: fix TX kfifo underflow on flush

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] serial: samsung: use-after-free on tx_desc in s3c24xx_serial_flush_buffer()
- [High] tty: serial: samsung: ABA race on tx_in_progress in s3c24xx_serial_tx_dma_complete()
--

commit 73ae60b92dcc88f73c03b4939f2ea2f4d964ab3a
Author: Samarth Narang <samarth.colleges@gmail.com>

tty: serial: samsung: fix TX kfifo underflow on flush

This commit adds a flush_buffer() hook to samsung_tty to terminate any active
TX DMA transfer when the transmit buffer is flushed, and updates
s3c24xx_serial_tx_dma_complete() to skip advancing the kfifo if the transfer
was already retired.

> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index 640f85e2d636b..2bea7ef078d2d 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -317,6 +317,20 @@ static void s3c24xx_serial_stop_tx(struct uart_port *port)
>  	ourport->tx_mode = 0;
>  }
>  
> +static void s3c24xx_serial_flush_buffer(struct uart_port *port)
> +{
> +	struct s3c24xx_uart_port *ourport = to_ourport(port);
> +	struct s3c24xx_uart_dma *dma = ourport->dma;
> +
> +	if (!dma || !dma->tx_chan || ourport->tx_in_progress != S3C24XX_TX_DMA)
> +		return;
> +
> +	/* The kfifo has been reset: the completion must not advance it */
> +	dmaengine_terminate_async(dma->tx_chan);
> +	async_tx_ack(dma->tx_desc);

[Severity: High]
Can calling async_tx_ack(dma->tx_desc) after dmaengine_terminate_async() in
drivers/tty/serial/samsung_tty.c:s3c24xx_serial_flush_buffer() cause a
use-after-free write on dma->tx_desc when flushing the TTY transmit buffer
while a TX DMA transfer is active (ourport->tx_in_progress == S3C24XX_TX_DMA)?

dmaengine_terminate_async(dma->tx_chan) invokes the DMA controller's
device_terminate_all() callback synchronously before returning. On Samsung
platforms using PL330, drivers/dma/pl330.c:pl330_terminate_all() returns all
channel descriptors back to the controller-wide descriptor pool:

	list_splice_tail_init(&pch->submitted_list, &pl330->desc_pool);
	list_splice_tail_init(&pch->work_list, &pl330->desc_pool);
	list_splice_tail_init(&pch->completed_list, &pl330->desc_pool);

where another channel can immediately reallocate the descriptor via
pl330_get_desc(). Similarly, on platforms using AMBA PL08x,
drivers/dma/amba-pl08x.c:pl08x_terminate_all() frees queued descriptors via
pl08x_free_txd_list() -> vchan_dma_desc_free_list() -> kfree(txd) before
dmaengine_terminate_async() returns.

When s3c24xx_serial_flush_buffer() then calls async_tx_ack(dma->tx_desc), it
writes DMA_CTRL_ACK to dma->tx_desc->flags on a descriptor that has already
been recycled or freed. In addition, if s3c24xx_serial_tx_dma_complete() is
running concurrently on another CPU, it also calls async_tx_ack(dma->tx_desc)
before acquiring port->lock, racing with s3c24xx_serial_flush_buffer()
terminating and freeing dma->tx_desc under port->lock.

> +	ourport->tx_in_progress = 0;
> +}
> +
>  static void s3c24xx_serial_start_next_tx(struct s3c24xx_uart_port *ourport);
>  
>  static void s3c24xx_serial_tx_dma_complete(void *args)
> @@ -339,6 +353,12 @@ static void s3c24xx_serial_tx_dma_complete(void *args)
>  
>  	uart_port_lock_irqsave(port, &flags);
>  
> +	/* Already retired by stop_tx or flush_buffer */
> +	if (ourport->tx_in_progress != S3C24XX_TX_DMA) {
> +		uart_port_unlock_irqrestore(port, flags);
> +		return;
> +	}

[Severity: High]
What happens if a new TX DMA transfer starts after
drivers/tty/serial/samsung_tty.c:s3c24xx_serial_flush_buffer() clears
ourport->tx_in_progress, but before an already-running
s3c24xx_serial_tx_dma_complete() acquires port->lock?

In drivers/tty/serial/samsung_tty.c:s3c24xx_serial_tx_dma_complete(), the
completion callback computes count from dma->tx_bytes_requested and
state.residue before taking port->lock:

	dmaengine_tx_status(dma->tx_chan, dma->tx_cookie, &state);
	count = dma->tx_bytes_requested - state.residue;
	async_tx_ack(dma->tx_desc);

	dma_sync_single_for_cpu(dmaengine_get_dma_device(dma->tx_chan),
				dma->tx_transfer_addr, dma->tx_size,
				DMA_TO_DEVICE);

	uart_port_lock_irqsave(port, &flags);

Because s3c24xx_serial_flush_buffer() uses dmaengine_terminate_async(), it
does not wait for an in-flight completion callback to finish before clearing
ourport->tx_in_progress = 0 and releasing port->lock in uart_flush_buffer().

If a subsequent or woken uart_write() of at least ourport->min_dma_size bytes
acquires port->lock before the stale s3c24xx_serial_tx_dma_complete() does,
__uart_start() -> s3c24xx_serial_start_tx() -> s3c24xx_serial_start_next_tx()
-> s3c24xx_serial_start_tx_dma() will see ourport->tx_in_progress == 0 and the
reset kfifo tail aligned to a cache line, overwrite the dma->tx_* fields, and
set ourport->tx_in_progress = S3C24XX_TX_DMA for the new transfer:

	dma->tx_bytes_requested = dma->tx_size;

	ourport->tx_in_progress = S3C24XX_TX_DMA;
	dma->tx_cookie = dmaengine_submit(dma->tx_desc);
	dma_async_issue_pending(dma->tx_chan);

When the first transfer's s3c24xx_serial_tx_dma_complete() then acquires
port->lock, ourport->tx_in_progress is S3C24XX_TX_DMA again. Wouldn't it
bypass the ourport->tx_in_progress != S3C24XX_TX_DMA check, call
uart_xmit_advance(port, count) with the first transfer's byte count on the
newly written kfifo (underflowing xmit_fifo if the new write is smaller than
count), and clear ourport->tx_in_progress = 0 while the second DMA transfer is
still active?

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