[BUG] serial: amba-pl011, imx, fsl_lpuart: ->flush_buffer() unmaps the TX DMA buffer before the transfer has stopped

[BUG] serial: amba-pl011, imx, fsl_lpuart: ->flush_buffer() unmaps the TX DMA buffer before the transfer has stopped

[Samarth Narang]

Hi,

I found three serial drivers that unmap their TX DMA buffers in
->flush_buffer() without first ensuring that the outstanding DMA
transfer has stopped. Line numbers refer to mainline at 83f32c773255.

uart_flush_buffer() invokes ->flush_buffer() under the port lock.
The path is reachable through TCFLSH(TCOFLUSH), hangup, and close, so a
userspace process with access to the port can trigger it while TX DMA
is active.

Because the port lock is held, the flush hook cannot use
dmaengine_terminate_sync(). However, dmaengine_terminate_async() only
requests termination; it does not guarantee that DMA has stopped or
that its completion callback has finished. The DMAengine API requires
synchronization before memory still accessible to the descriptor is
released (include/linux/dmaengine.h and
Documentation/driver-api/dmaengine/client.rst).

I found the following cases:

1. amba-pl011

pl011_dma_flush_buffer() (lines 858-877) calls
dmaengine_terminate_async() and immediately unmaps dmatx.buf.

The unmap can therefore occur before the DMA transfer and its callback
have finished.

2. imx

imx_uart_flush_buffer() (lines 1716-1739) calls the deprecated
dmaengine_terminate_all() and, when dma_is_txing is set, immediately
unmaps tx_sgl at line 1729.

The imx-sdma termination path disables the channel but defers descriptor
cleanup to a worker that sleeps for 1-2 ms because the SDMA core may
still be running. Consequently, termination does not establish that
the controller has stopped accessing the mapping when the unmap occurs.

There is also a separate callback race. imx_uart_dma_tx_callback()
unconditionally unmaps tx_sgl at line 595. If the callback is already
running when the flush begins, for example while waiting to acquire
the port lock, it can subsequently unmap the same scatterlist again
and may attempt to start another transfer.

3. fsl_lpuart

lpuart_flush_buffer() (lines 601-614) unmaps tx_sgl at line 609
before calling dmaengine_terminate_async() at line 613.

In this case, the mapping is released before termination has even
been requested, while eDMA may still be reading the buffer.

Potential consequences

These paths can release a DMA mapping while the previous transfer
may still retain read access to its backing storage.

Without an IOMMU or SWIOTLB, a late read can race with subsequent
modification or reuse of the buffer. With an IOMMU, an unmapped IOVA
may subsequently be reassigned. With SWIOTLB bouncing, a released
bounce slot may be reused by another mapping. If the old transfer
continues reading after such reuse, data belonging to a different
operation could be transmitted over the serial interface.

LS1028A is one configuration worth examining for the SWIOTLB case:
fsl-edma uses a 32-bit DMA mask without FSL_EDMA_DRV_TCD64, while
supported systems may have RAM above 4 GiB. Whether the affected TX
buffers actually use bouncing, and whether a late read occurs after
slot reuse, still needs to be established.

A related issue in qcom-geni was fixed by e3c04834ae1a
("serial: qcom-geni: fix TX DMA buffer flush"), associated with
CVE-2026-74655.

Possible fix

Since ->flush_buffer() runs under the port lock and cannot sleep,
one approach would be to:

- request asynchronous termination and mark the current TX operation
  as flushing, without immediately unmapping its buffer;
- defer synchronization and mapping teardown to a sleepable context;
- ensure that a completion callback for the flushed operation cannot
  unmap the buffer again, advance stale TX state, or restart DMA; and
- ensure that shutdown, and suspend where applicable, drain the deferred
  cleanup before releasing the associated resources.

The callback and worker would need to coordinate ownership of the
mapping, and any work draining would need to occur outside locks
required by the callback.

I have compile-tested candidate patches for amba-pl011 and
fsl_lpuart using this general approach. I can send them for review,
or adapt them if the maintainers prefer a different synchronization
strategy.

These cases were identified using a research static analyzer for DMA
lifecycle ordering and subsequently reviewed against the driver
source. They are source-level lifetime concerns, not demonstrated
hardware exploits.

Regards,
Samarth Narang

Re: [BUG] serial: amba-pl011, imx, fsl_lpuart: ->flush_buffer() unmaps the TX DMA buffer before the transfer has stopped

[Greg Kroah-Hartman]

On Sat, Oct 31, 2026 at 12:54:25AM +0000, Samarth Narang wrote:
> I have compile-tested candidate patches for amba-pl011 and
> fsl_lpuart using this general approach. I can send them for review,
> or adapt them if the maintainers prefer a different synchronization
> strategy.

Please always just send patches, you do not need "permission" to do so.

thanks,

greg k-h

[PATCH 0/2] serial: keep the flushed TX buffer mapped until DMA has stopped

[Samarth Narang]

This series addresses the DMA buffer lifetime issues in amba-pl011 and
fsl_lpuart reported in the parent thread, where flush_buffer() unmaps
the TX buffer before the terminated DMA transfer is guaranteed to have
stopped accessing it. In fsl_lpuart, the buffer is unmapped even before
termination is requested.

Since flush_buffer() runs under the port lock and cannot wait for DMA
termination, both patches request termination while holding the lock and
defer unmapping to a work item that first calls dmaengine_synchronize().
The TX completion callback leaves the cleanup of flushed transfers to
this work item, while shutdown, and suspend in the case of fsl_lpuart,
waits for the work to finish before releasing the TX DMA state.

Both patches have only been compile-tested with arm64 defconfig, the
relevant drivers enabled, and W=1, with no new warnings. I do not have
access to PL011 or LPUART hardware with DMA support, so testing by
someone with suitable hardware would be appreciated.

The imx driver also needs a fix for the issue reported in the same
thread, but requires a different approach because its TX completion
callback unmaps the scatterlist unconditionally. I will send that patch
separately.

Samarth Narang (2):
  serial: amba-pl011: keep the flushed TX buffer mapped until DMA stops
  tty: serial: fsl_lpuart: don't unmap TX buffer before DMA has stopped

 drivers/tty/serial/amba-pl011.c | 54 +++++++++++++++++++++++++++++++--
 drivers/tty/serial/fsl_lpuart.c | 53 ++++++++++++++++++++++++++++----
 2 files changed, 98 insertions(+), 9 deletions(-)


base-commit: 36aad21d50d1af3f6954bb0c988257a908202477
-- 
2.34.1

[PATCH 1/2] serial: amba-pl011: keep the flushed TX buffer mapped until DMA stops

[Samarth Narang]

pl011_dma_flush_buffer() is called with the port lock held and currently
unmaps the TX buffer immediately after requesting termination with
dmaengine_terminate_async(). Since asynchronous termination does not
guarantee that the transfer or its completion callback has finished, the
DMA controller may continue reading from the buffer after its mapping
has been released. Commit f6a196477184 ("serial: pl011: Fix DMA
->flush_buffer()") introduced asynchronous termination under the port
lock, but did not add the required synchronization before unmapping.

On systems using an IOMMU or SWIOTLB, outstanding DMA reads may access
an IOVA or bounce buffer that has already been reused, potentially
transmitting data from another mapping. On systems without either, the
outstanding transfer may instead race with the next refill of dmatx.buf.
This can be triggered by flushing the TTY while TX DMA is active.

Since flush_buffer() cannot sleep, defer the unmap to a work item that
calls dmaengine_synchronize() before releasing the mapping. Keep
dmatx.queued set to prevent buffer reuse, disable TX DMA requests, and
let the work item restart transmission if data remains pending. The
completion callback leaves flushed transfers for deferred cleanup, while
shutdown waits for the work to finish before tearing down TX DMA.

Fixes: f6a196477184 ("serial: pl011: Fix DMA ->flush_buffer()")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Samarth Narang <samarth.colleges@gmail.com>
---
 drivers/tty/serial/amba-pl011.c | 54 +++++++++++++++++++++++++++++++--
 1 file changed, 51 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index e6f652905ca6..863a050cf654 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -41,6 +41,7 @@
 #include <linux/sizes.h>
 #include <linux/io.h>
 #include <linux/acpi.h>
+#include <linux/workqueue.h>
 
 #define UART_NR			14
 
@@ -302,6 +303,8 @@ struct pl011_dmatx_data {
 	size_t			len;
 	char			*buf;
 	bool			queued;
+	bool			flushing;
+	struct work_struct	flush_work;
 };
 
 enum pl011_rs485_tx_state {
@@ -609,6 +612,12 @@ static void pl011_dma_tx_callback(void *data)
 	u16 dmacr;
 
 	uart_port_lock_irqsave(&uap->port, &flags);
+	if (uap->dmatx.flushing) {
+		/* pl011_dma_tx_flush_work() retires the flushed buffer */
+		uart_port_unlock_irqrestore(&uap->port, flags);
+		return;
+	}
+
 	if (uap->dmatx.queued)
 		dma_unmap_single(dmaengine_get_dma_device(dmatx->chan), dmatx->dma,
 				 dmatx->len, DMA_TO_DEVICE);
@@ -867,13 +876,47 @@ __acquires(&uap->port.lock)
 
 	dmaengine_terminate_async(uap->dmatx.chan);
 
-	if (uap->dmatx.queued) {
-		dma_unmap_single(dmaengine_get_dma_device(uap->dmatx.chan), uap->dmatx.dma,
-				 uap->dmatx.len, DMA_TO_DEVICE);
+	/*
+	 * The terminated transfer may still be reading dmatx.buf: keep it
+	 * mapped, and queued set so that nothing refills it, until
+	 * pl011_dma_tx_flush_work() has synchronized the channel.
+	 */
+	if (uap->dmatx.queued && !uap->dmatx.flushing) {
+		uap->dmatx.flushing = true;
+		uap->dmacr &= ~UART011_TXDMAE;
+		pl011_write(uap->dmacr, uap, REG_DMACR);
+		schedule_work(&uap->dmatx.flush_work);
+	}
+}
+
+/*
+ * flush_buffer runs under the port lock and can only request termination.
+ * Unmap the flushed TX buffer here, once the terminated transfer and its
+ * completion callback are known to have finished, then restart TX.
+ */
+static void pl011_dma_tx_flush_work(struct work_struct *work)
+{
+	struct uart_amba_port *uap =
+	    container_of(work, struct uart_amba_port, dmatx.flush_work);
+	struct tty_port *tport = &uap->port.state->port;
+	unsigned long flags;
+
+	dmaengine_synchronize(uap->dmatx.chan);
+
+	uart_port_lock_irqsave(&uap->port, &flags);
+	if (uap->dmatx.flushing) {
+		dma_unmap_single(dmaengine_get_dma_device(uap->dmatx.chan),
+				 uap->dmatx.dma, uap->dmatx.len, DMA_TO_DEVICE);
+		uap->dmatx.flushing = false;
 		uap->dmatx.queued = false;
 		uap->dmacr &= ~UART011_TXDMAE;
 		pl011_write(uap->dmacr, uap, REG_DMACR);
+		if (!uart_tx_stopped(&uap->port) &&
+		    !kfifo_is_empty(&tport->xmit_fifo) &&
+		    pl011_dma_tx_refill(uap) <= 0)
+			pl011_start_tx_pio(uap);
 	}
+	uart_port_unlock_irqrestore(&uap->port, flags);
 }
 
 static void pl011_dma_rx_callback(void *data);
@@ -1177,6 +1220,8 @@ static void pl011_dma_startup(struct uart_amba_port *uap)
 
 	/* The DMA buffer is now the FIFO the TTY subsystem can use */
 	uap->port.fifosize = PL011_DMA_BUFFER_SIZE;
+	uap->dmatx.flushing = false;
+	INIT_WORK(&uap->dmatx.flush_work, pl011_dma_tx_flush_work);
 	uap->using_tx_dma = true;
 
 	if (!uap->dmarx.chan)
@@ -1236,6 +1281,9 @@ static void pl011_dma_shutdown(struct uart_amba_port *uap)
 	if (!(uap->using_tx_dma || uap->using_rx_dma))
 		return;
 
+	if (uap->using_tx_dma)
+		flush_work(&uap->dmatx.flush_work);
+
 	/* Disable RX and TX DMA */
 	while (pl011_read(uap, REG_FR) & uap->vendor->fr_busy)
 		cpu_relax();
-- 
2.34.1

[PATCH 2/2] tty: serial: fsl_lpuart: don't unmap TX buffer before DMA has stopped

[Samarth Narang]

lpuart_flush_buffer() unmaps the TX scatterlist before calling
dmaengine_terminate_async(), allowing eDMA to continue reading from the
buffer after its mapping has been released. Reversing these operations
would not be sufficient, since asynchronous termination does not
guarantee that DMA has stopped and dmaengine_synchronize() is required
before releasing memory accessed by the descriptor.

The issue can be triggered through TCFLSH(TCOFLUSH), hangup, or close
while TX DMA is active. On systems using SWIOTLB, outstanding DMA reads
may access a bounce slot that has already been reassigned to another
device, potentially transmitting its contents over the serial line.

Since flush_buffer() runs under the port lock and cannot sleep, request
termination there while keeping the scatterlist mapped and
dma_tx_in_progress set to prevent reuse. Defer unmapping to a work item
that first calls dmaengine_synchronize(), have the TX completion
callback ignore flushed transfers, and ensure that shutdown and suspend
wait for the work to finish before resetting the TX DMA state.

Fixes: 6250cc30c4c4 ("tty: serial: fsl_lpuart: Use scatter/gather DMA for Tx")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Samarth Narang <samarth.colleges@gmail.com>
---
 drivers/tty/serial/fsl_lpuart.c | 53 +++++++++++++++++++++++++++++----
 1 file changed, 47 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index c8575c965203..8343351199eb 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -26,6 +26,7 @@
 #include <linux/serial_core.h>
 #include <linux/slab.h>
 #include <linux/tty_flip.h>
+#include <linux/workqueue.h>
 
 /* All registers are 8-bit width */
 #define UARTBDH			0x00
@@ -280,6 +281,8 @@ struct lpuart_port {
 	unsigned int		dma_tx_bytes;
 	unsigned int		dma_rx_bytes;
 	bool			dma_tx_in_progress;
+	bool			dma_tx_flushing;
+	struct work_struct	dma_tx_flush_work;
 	unsigned int		dma_rx_timeout;
 	struct timer_list	lpuart_timer;
 	struct scatterlist	rx_sgl, tx_sgl[2];
@@ -530,7 +533,7 @@ static void lpuart_dma_tx_complete(void *arg)
 	unsigned long flags;
 
 	uart_port_lock_irqsave(&sport->port, &flags);
-	if (!sport->dma_tx_in_progress) {
+	if (!sport->dma_tx_in_progress || sport->dma_tx_flushing) {
 		uart_port_unlock_irqrestore(&sport->port, flags);
 		return;
 	}
@@ -558,6 +561,36 @@ static void lpuart_dma_tx_complete(void *arg)
 	uart_port_unlock_irqrestore(&sport->port, flags);
 }
 
+/*
+ * flush_buffer runs under the port lock and can only request termination.
+ * Unmap the TX scatterlist here, once the terminated transfer and its
+ * completion callback are known to have finished.
+ */
+static void lpuart_dma_tx_flush_work(struct work_struct *work)
+{
+	struct lpuart_port *sport = container_of(work, struct lpuart_port,
+						 dma_tx_flush_work);
+	struct dma_chan *chan = sport->dma_tx_chan;
+	unsigned long flags;
+
+	dmaengine_synchronize(chan);
+
+	uart_port_lock_irqsave(&sport->port, &flags);
+	if (sport->dma_tx_flushing) {
+		dma_unmap_sg(dmaengine_get_dma_device(chan), &sport->tx_sgl[0],
+			     sport->dma_tx_nents, DMA_TO_DEVICE);
+		sport->dma_tx_flushing = false;
+		sport->dma_tx_in_progress = false;
+	}
+	/* As in the TX callback: no restart while shutdown waits on dma_wait */
+	if (!waitqueue_active(&sport->dma_wait) &&
+	    !lpuart_stopped_or_empty(&sport->port))
+		lpuart_dma_tx(sport);
+	uart_port_unlock_irqrestore(&sport->port, flags);
+
+	wake_up(&sport->dma_wait);
+}
+
 static dma_addr_t lpuart_dma_datareg_addr(struct lpuart_port *sport)
 {
 	switch (sport->port.iotype) {
@@ -605,12 +638,16 @@ static void lpuart_flush_buffer(struct uart_port *port)
 	u32 fifo;
 
 	if (sport->lpuart_dma_tx_use) {
-		if (sport->dma_tx_in_progress) {
-			dma_unmap_sg(dmaengine_get_dma_device(chan), &sport->tx_sgl[0],
-				sport->dma_tx_nents, DMA_TO_DEVICE);
-			sport->dma_tx_in_progress = false;
-		}
 		dmaengine_terminate_async(chan);
+		/*
+		 * The transfer may still be reading the TX scatterlist: keep it
+		 * mapped (and dma_tx_in_progress set) until the flush work has
+		 * synchronized the channel.
+		 */
+		if (sport->dma_tx_in_progress && !sport->dma_tx_flushing) {
+			sport->dma_tx_flushing = true;
+			schedule_work(&sport->dma_tx_flush_work);
+		}
 	}
 
 	if (lpuart_is_32(sport)) {
@@ -1760,6 +1797,8 @@ static void lpuart_tx_dma_startup(struct lpuart_port *sport)
 		goto err;
 
 	init_waitqueue_head(&sport->dma_wait);
+	INIT_WORK(&sport->dma_tx_flush_work, lpuart_dma_tx_flush_work);
+	sport->dma_tx_flushing = false;
 	sport->lpuart_dma_tx_use = true;
 	if (lpuart_is_32(sport)) {
 		uartbaud = lpuart32_read(&sport->port, UARTBAUD);
@@ -1926,6 +1965,7 @@ static void lpuart_dma_shutdown(struct lpuart_port *sport)
 	}
 
 	if (sport->lpuart_dma_tx_use) {
+		flush_work(&sport->dma_tx_flush_work);
 		if (wait_event_interruptible_timeout(sport->dma_wait,
 			!sport->dma_tx_in_progress, msecs_to_jiffies(300)) <= 0) {
 			sport->dma_tx_in_progress = false;
@@ -3179,6 +3219,7 @@ static int lpuart_suspend(struct device *dev)
 				writeb(temp, sport->port.membase + UARTCR5);
 			}
 			uart_port_unlock_irqrestore(&sport->port, flags);
+			flush_work(&sport->dma_tx_flush_work);
 			sport->dma_tx_in_progress = false;
 			dmaengine_terminate_sync(sport->dma_tx_chan);
 		}
-- 
2.34.1

Re: [PATCH 1/2] serial: amba-pl011: keep the flushed TX buffer mapped until DMA stops

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] serial: amba-pl011: race and TX restart in pl011_dma_shutdown()
- [High] serial: amba-pl011: TX DMA re-enabled while dmatx.flushing is true
- [Medium] serial: amba-pl011: pl011_dma_tx_flush_work() bypasses pl011_start_tx()
--

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

serial: amba-pl011: keep the flushed TX buffer mapped until DMA stops

This commit defers unmapping a flushed TX DMA buffer from
pl011_dma_flush_buffer() to a work item that calls dmaengine_synchronize()
before releasing the mapping and restarting transmission. It keeps
dmatx.queued set while dmatx.flushing is active and flushes the work item in
pl011_dma_shutdown().

> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index e6f652905ca6f..863a050cf654e 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c

[ ... ]

> @@ -867,13 +876,47 @@ __acquires(&uap->port.lock)
>  
>  	dmaengine_terminate_async(uap->dmatx.chan);
>  
> -	if (uap->dmatx.queued) {
> -		dma_unmap_single(dmaengine_get_dma_device(uap->dmatx.chan), uap->dmatx.dma,
> -				 uap->dmatx.len, DMA_TO_DEVICE);
> +	/*
> +	 * The terminated transfer may still be reading dmatx.buf: keep it
> +	 * mapped, and queued set so that nothing refills it, until
> +	 * pl011_dma_tx_flush_work() has synchronized the channel.
> +	 */
> +	if (uap->dmatx.queued && !uap->dmatx.flushing) {
> +		uap->dmatx.flushing = true;
> +		uap->dmacr &= ~UART011_TXDMAE;
> +		pl011_write(uap->dmacr, uap, REG_DMACR);
> +		schedule_work(&uap->dmatx.flush_work);
> +	}
> +}

[Severity: High]
Can UART011_TXDMAE be re-enabled in REG_DMACR while uap->dmatx.flushing is
still true and before pl011_dma_tx_flush_work() finishes
dmaengine_synchronize()?

When uart_flush_buffer() calls pl011_dma_flush_buffer(), uap->dmatx.queued is
kept true while UART011_TXDMAE is cleared. Immediately after
pl011_dma_flush_buffer() returns, uart_flush_buffer() drops uap->port.lock and
calls tty_port_tty_wakeup(). If a subsequent write or wakeup calls
pl011_start_tx() -> pl011_dma_tx_start() before pl011_dma_tx_flush_work()
runs, pl011_dma_tx_start() sees uap->dmatx.queued set and UART011_TXDMAE clear:

drivers/tty/serial/amba-pl011.c:pl011_dma_tx_start() {
	...
		if (!uap->dmatx.queued) {
			...
		} else if (!(uap->dmacr & UART011_TXDMAE)) {
			uap->dmacr |= UART011_TXDMAE;
			pl011_write(uap->dmacr, uap, REG_DMACR);
		}
		return ret;
}

Similarly, if a TX interrupt fires (for example after sending an x_char) and
calls pl011_dma_tx_irq(), it also checks uap->dmatx.queued without checking
uap->dmatx.flushing:

drivers/tty/serial/amba-pl011.c:pl011_dma_tx_irq() {
	...
	if (uap->dmatx.queued) {
		uap->dmacr |= UART011_TXDMAE;
		pl011_write(uap->dmacr, uap, REG_DMACR);
		uap->im &= ~UART011_TXIM;
		pl011_write(uap->im, uap, REG_IMSC);
		return true;
	}
}

In both cases, re-enabling UART011_TXDMAE causes the UART to assert TX DMA
requests to the DMA controller for the terminated transfer and pull stale
flushed bytes from dmatx.buf into the TX FIFO, and a second call to
pl011_dma_flush_buffer() while uap->dmatx.flushing is already set will not
clear UART011_TXDMAE again. Should pl011_dma_tx_start() and
pl011_dma_tx_irq() check uap->dmatx.flushing before setting UART011_TXDMAE?

> +
> +/*
> + * flush_buffer runs under the port lock and can only request termination.
> + * Unmap the flushed TX buffer here, once the terminated transfer and its
> + * completion callback are known to have finished, then restart TX.
> + */
> +static void pl011_dma_tx_flush_work(struct work_struct *work)
> +{
> +	struct uart_amba_port *uap =
> +	    container_of(work, struct uart_amba_port, dmatx.flush_work);
> +	struct tty_port *tport = &uap->port.state->port;
> +	unsigned long flags;
> +
> +	dmaengine_synchronize(uap->dmatx.chan);
> +
> +	uart_port_lock_irqsave(&uap->port, &flags);
> +	if (uap->dmatx.flushing) {
> +		dma_unmap_single(dmaengine_get_dma_device(uap->dmatx.chan),
> +				 uap->dmatx.dma, uap->dmatx.len, DMA_TO_DEVICE);
> +		uap->dmatx.flushing = false;
>  		uap->dmatx.queued = false;
>  		uap->dmacr &= ~UART011_TXDMAE;
>  		pl011_write(uap->dmacr, uap, REG_DMACR);
> +		if (!uart_tx_stopped(&uap->port) &&
> +		    !kfifo_is_empty(&tport->xmit_fifo) &&
> +		    pl011_dma_tx_refill(uap) <= 0)
> +			pl011_start_tx_pio(uap);
>  	}
> +	uart_port_unlock_irqrestore(&uap->port, flags);
>  }

[Severity: Medium]
Does calling pl011_dma_tx_refill() and pl011_start_tx_pio() directly in
pl011_dma_tx_flush_work() bypass the RS485 state machine and UART011_TXIM
masking normally handled by pl011_start_tx()?

In pl011_start_tx(), when SER_RS485_ENABLED is set and uap->rs485_tx_state is
not SEND, the driver calls pl011_rs485_tx_start() and waits for
WAIT_AFTER_RTS before starting DMA or PIO:

drivers/tty/serial/amba-pl011.c:pl011_start_tx() {
	if ((uap->port.rs485.flags & SER_RS485_ENABLED) &&
	    uap->rs485_tx_state != SEND) {
		/* trigger_start_tx starts TX once delay_rts_before_send passed */
		if (uap->rs485_tx_state == WAIT_AFTER_RTS)
			return;

		pl011_rs485_tx_start(uap);
		if (uap->rs485_tx_state == WAIT_AFTER_RTS)
			return;
	}

	if (!pl011_dma_tx_start(uap))
		pl011_start_tx_pio(uap);
}

In addition, when pl011_dma_tx_refill() > 0 succeeds in pl011_dma_tx_start(),
it clears UART011_TXIM in uap->im and writes REG_IMSC, which is skipped when
pl011_dma_tx_flush_work() calls pl011_dma_tx_refill() directly. Should
pl011_dma_tx_flush_work() call pl011_start_tx(&uap->port) instead?

[ ... ]

> @@ -1236,6 +1281,9 @@ static void pl011_dma_shutdown(struct uart_amba_port *uap)
>  	if (!(uap->using_tx_dma || uap->using_rx_dma))
>  		return;
>  
> +	if (uap->using_tx_dma)
> +		flush_work(&uap->dmatx.flush_work);
> +
>  	/* Disable RX and TX DMA */
>  	while (pl011_read(uap, REG_FR) & uap->vendor->fr_busy)
>  		cpu_relax();

[Severity: Critical]
Can flushing dmatx.flush_work at the start of pl011_dma_shutdown() allow TX
to restart during shutdown or race with a concurrent uart_flush_buffer()?

First, pl011_shutdown() calls pl011_disable_interrupts(uap) before calling
pl011_dma_shutdown(uap). When flush_work(&uap->dmatx.flush_work) runs at the
start of pl011_dma_shutdown() while tport->xmit_fifo is non-empty and
!uart_tx_stopped(&uap->port) (such as during uart_suspend_port()),
pl011_dma_tx_flush_work() will either start a new DMA transfer via
pl011_dma_tx_refill() and advance xmit_fifo right before
dmaengine_terminate_sync() discards it, or call pl011_start_tx_pio() and
re-enable UART011_TXIM in REG_IMSC after pl011_disable_interrupts() masked
all interrupts.

Second, after flush_work(&uap->dmatx.flush_work) returns,
pl011_dma_shutdown() spins on REG_FR and calls dmaengine_terminate_sync()
without holding uap->port.lock while uap->using_tx_dma and uap->dmatx.queued
are still true:

drivers/tty/serial/amba-pl011.c:pl011_dma_shutdown() {
	...
	if (uap->using_tx_dma) {
		/* In theory, this should already be done by pl011_dma_flush_buffer */
		dmaengine_terminate_sync(uap->dmatx.chan);
		if (uap->dmatx.queued) {
			dma_unmap_single(dmaengine_get_dma_device(uap->dmatx.chan),
					 uap->dmatx.dma, uap->dmatx.len,
					 DMA_TO_DEVICE);
			uap->dmatx.queued = false;
		}

		kfree(uap->dmatx.buf);
		uap->using_tx_dma = false;
	}
}

Because uart_flush_buffer() only takes uap->port.lock, a concurrent
uart_flush_buffer() -> pl011_dma_flush_buffer() after flush_work() returns
can see uap->using_tx_dma and uap->dmatx.queued still set, mark
uap->dmatx.flushing = true, and schedule uap->dmatx.flush_work again. Once
pl011_dma_shutdown() unmaps uap->dmatx.dma, frees uap->dmatx.buf, and
pl011_shutdown() disables the UART clock, the newly scheduled
pl011_dma_tx_flush_work() can unmap uap->dmatx.dma a second time, write
REG_DMACR with the clock disabled, and access the freed uap->dmatx.buf.

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

Re: [PATCH 2/2] tty: serial: fsl_lpuart: don't unmap TX buffer before DMA has stopped

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] tty: serial: fsl_lpuart: race scheduling dma_tx_flush_work after flush_work()
- [High] tty: serial: fsl_lpuart: lpuart_dma_tx_flush_work() restarts DMA on teardown
--

commit 45f7dcf3d930bb1e96c498ca260fc93b635e30f0
Author: Samarth Narang <samarth.colleges@gmail.com>

tty: serial: fsl_lpuart: don't unmap TX buffer before DMA has stopped

This commit defers TX DMA scatterlist unmapping from lpuart_flush_buffer() to
a work item that runs dmaengine_synchronize() before releasing the mapping,
and flushes that work item in lpuart_dma_shutdown() and lpuart_suspend().

> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
> index c8575c9652035..8343351199eb5 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c

[ ... ]

> @@ -558,6 +561,36 @@ static void lpuart_dma_tx_complete(void *arg)
>  	uart_port_unlock_irqrestore(&sport->port, flags);
>  }
>  
> +/*
> + * flush_buffer runs under the port lock and can only request termination.
> + * Unmap the TX scatterlist here, once the terminated transfer and its
> + * completion callback are known to have finished.
> + */
> +static void lpuart_dma_tx_flush_work(struct work_struct *work)
> +{
> +	struct lpuart_port *sport = container_of(work, struct lpuart_port,
> +						 dma_tx_flush_work);
> +	struct dma_chan *chan = sport->dma_tx_chan;
> +	unsigned long flags;
> +
> +	dmaengine_synchronize(chan);
> +
> +	uart_port_lock_irqsave(&sport->port, &flags);
> +	if (sport->dma_tx_flushing) {
> +		dma_unmap_sg(dmaengine_get_dma_device(chan), &sport->tx_sgl[0],
> +			     sport->dma_tx_nents, DMA_TO_DEVICE);
> +		sport->dma_tx_flushing = false;
> +		sport->dma_tx_in_progress = false;
> +	}
> +	/* As in the TX callback: no restart while shutdown waits on dma_wait */
> +	if (!waitqueue_active(&sport->dma_wait) &&
> +	    !lpuart_stopped_or_empty(&sport->port))
> +		lpuart_dma_tx(sport);

[Severity: High]
Can lpuart_dma_tx_flush_work() still restart TX DMA and leak the scatterlist
mapping on sport->tx_sgl during shutdown or suspend?

In lpuart_dma_shutdown(), flush_work(&sport->dma_tx_flush_work) is called
before wait_event_interruptible_timeout() adds a waiter to sport->dma_wait:

	if (sport->lpuart_dma_tx_use) {
		flush_work(&sport->dma_tx_flush_work);
		if (wait_event_interruptible_timeout(sport->dma_wait,
			!sport->dma_tx_in_progress, msecs_to_jiffies(300)) <= 0) {
			sport->dma_tx_in_progress = false;
			dmaengine_terminate_sync(sport->dma_tx_chan);
		}
		sport->lpuart_dma_tx_use = false;
	}

In lpuart_suspend(), flush_work(&sport->dma_tx_flush_work) is also called
with no waiter on sport->dma_wait:

		if (sport->lpuart_dma_tx_use) {
			...
			uart_port_unlock_irqrestore(&sport->port, flags);
			flush_work(&sport->dma_tx_flush_work);
			sport->dma_tx_in_progress = false;
			dmaengine_terminate_sync(sport->dma_tx_chan);
		}

Because waitqueue_active(&sport->dma_wait) is false while flush_work() runs
in both paths, if xmit_fifo is non-empty (for example, if bytes were queued
after uart_flush_buffer() called tty_port_tty_wakeup() while
dma_tx_in_progress was held true by a pending flush, or when suspending an
active wakeup-enabled port where uart_suspend_port() returns early),
lpuart_dma_tx_flush_work() will call lpuart_dma_tx(sport) and map
sport->tx_sgl after the UART transmitter and TX DMA request bits have already
been disabled in hardware.

Since hardware TX DMA is already disabled, that newly submitted transfer
cannot complete: lpuart_dma_shutdown() will wait for the 300ms timeout (or
return immediately on a pending signal) and then call
dmaengine_terminate_sync(sport->dma_tx_chan) without unmapping sport->tx_sgl
before uart_shutdown() frees xmit_buf, and lpuart_suspend() will likewise
clear sport->dma_tx_in_progress and call dmaengine_terminate_sync() without
unmapping sport->tx_sgl.

> +	uart_port_unlock_irqrestore(&sport->port, flags);
> +
> +	wake_up(&sport->dma_wait);
> +}

[ ... ]

> @@ -1760,6 +1797,8 @@ static void lpuart_tx_dma_startup(struct lpuart_port *sport)
>  		goto err;
>  
>  	init_waitqueue_head(&sport->dma_wait);
> +	INIT_WORK(&sport->dma_tx_flush_work, lpuart_dma_tx_flush_work);
> +	sport->dma_tx_flushing = false;
>  	sport->lpuart_dma_tx_use = true;
>  	if (lpuart_is_32(sport)) {
>  		uartbaud = lpuart32_read(&sport->port, UARTBAUD);
> @@ -1926,6 +1965,7 @@ static void lpuart_dma_shutdown(struct lpuart_port *sport)
>  	}
>  
>  	if (sport->lpuart_dma_tx_use) {
> +		flush_work(&sport->dma_tx_flush_work);
>  		if (wait_event_interruptible_timeout(sport->dma_wait,
>  			!sport->dma_tx_in_progress, msecs_to_jiffies(300)) <= 0) {
>  			sport->dma_tx_in_progress = false;
> @@ -3179,6 +3219,7 @@ static int lpuart_suspend(struct device *dev)
>  				writeb(temp, sport->port.membase + UARTCR5);
>  			}
>  			uart_port_unlock_irqrestore(&sport->port, flags);
> +			flush_work(&sport->dma_tx_flush_work);
>  			sport->dma_tx_in_progress = false;
>  			dmaengine_terminate_sync(sport->dma_tx_chan);
>  		}

[Severity: Critical]
What prevents lpuart_flush_buffer() from scheduling sport->dma_tx_flush_work
again after flush_work(&sport->dma_tx_flush_work) has already returned in
lpuart_dma_shutdown() or lpuart_suspend()?

uart_flush_buffer() only takes sport->port.lock and does not hold
state->port.mutex or check tty_port_initialized(), so it can run concurrently
via tty_ioctl(TCFLSH) -> tty_perform_flush() or line discipline signal
handling (isig() -> tty_driver_flush_buffer()):

	if (sport->lpuart_dma_tx_use) {
		dmaengine_terminate_async(chan);
		if (sport->dma_tx_in_progress && !sport->dma_tx_flushing) {
			sport->dma_tx_flushing = true;
			schedule_work(&sport->dma_tx_flush_work);
		}
	}

In lpuart_dma_shutdown(), after flush_work(&sport->dma_tx_flush_work)
returns, the task sleeps in wait_event_interruptible_timeout() without
holding sport->port.lock while sport->lpuart_dma_tx_use is still true and
(if an un-flushed TX DMA transfer was active when lpuart_shutdown() or
lpuart32_shutdown() disabled hardware TX) sport->dma_tx_in_progress is true
and sport->dma_tx_flushing is false.

If lpuart_flush_buffer() runs during that wait and schedules
sport->dma_tx_flush_work, and wait_event_interruptible_timeout() is
interrupted by a signal or times out before the work runs,
lpuart_dma_shutdown() proceeds to call
dma_release_channel(sport->dma_tx_chan) without flushing
sport->dma_tx_flush_work again, allowing lpuart_dma_tx_flush_work() to
dereference the released sport->dma_tx_chan.

Similarly, in lpuart_suspend() on an active wakeup-enabled port,
sport->lpuart_dma_tx_use remains true across suspend and
sport->dma_tx_in_progress is cleared outside sport->port.lock after
flush_work(). If sport->dma_tx_flush_work is queued after flush_work() in
either path, a subsequent open or resume calling lpuart_tx_dma_startup()
will also execute INIT_WORK(&sport->dma_tx_flush_work, ...) while the work
item is still pending or running.

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

Re: [PATCH 0/2] serial: keep the flushed TX buffer mapped until DMA has stopped

[Greg Kroah-Hartman]

On Sat, Oct 31, 2026 at 12:37:47PM +0000, Samarth Narang wrote:
> This series addresses the DMA buffer lifetime issues in amba-pl011 and
> fsl_lpuart reported in the parent thread, where flush_buffer() unmaps
> the TX buffer before the terminated DMA transfer is guaranteed to have
> stopped accessing it. In fsl_lpuart, the buffer is unmapped even before
> termination is requested.
> 
> Since flush_buffer() runs under the port lock and cannot wait for DMA
> termination, both patches request termination while holding the lock and
> defer unmapping to a work item that first calls dmaengine_synchronize().
> The TX completion callback leaves the cleanup of flushed transfers to
> this work item, while shutdown, and suspend in the case of fsl_lpuart,
> waits for the work to finish before releasing the TX DMA state.
> 
> Both patches have only been compile-tested with arm64 defconfig, the
> relevant drivers enabled, and W=1, with no new warnings. I do not have
> access to PL011 or LPUART hardware with DMA support, so testing by
> someone with suitable hardware would be appreciated.
> 
> The imx driver also needs a fix for the issue reported in the same
> thread, but requires a different approach because its TX completion
> callback unmaps the scatterlist unconditionally. I will send that patch
> separately.

Again, these need to be tested on real hardware.  Not to mention the
bugs that sashiko just found in the changes as well.

thanks,

greg k-h

Re: [PATCH 0/2] serial: keep the flushed TX buffer mapped until DMA has stopped

[Samarth Narang]

Thanks Greg, I will address the findings Sashiko found, would
appreciate someone with hardware giving a stamp of approval!

On Sat, Oct 31, 2026 at 9:00 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Sat, Oct 31, 2026 at 12:37:47PM +0000, Samarth Narang wrote:
> > This series addresses the DMA buffer lifetime issues in amba-pl011 and
> > fsl_lpuart reported in the parent thread, where flush_buffer() unmaps
> > the TX buffer before the terminated DMA transfer is guaranteed to have
> > stopped accessing it. In fsl_lpuart, the buffer is unmapped even before
> > termination is requested.
> >
> > Since flush_buffer() runs under the port lock and cannot wait for DMA
> > termination, both patches request termination while holding the lock and
> > defer unmapping to a work item that first calls dmaengine_synchronize().
> > The TX completion callback leaves the cleanup of flushed transfers to
> > this work item, while shutdown, and suspend in the case of fsl_lpuart,
> > waits for the work to finish before releasing the TX DMA state.
> >
> > Both patches have only been compile-tested with arm64 defconfig, the
> > relevant drivers enabled, and W=1, with no new warnings. I do not have
> > access to PL011 or LPUART hardware with DMA support, so testing by
> > someone with suitable hardware would be appreciated.
> >
> > The imx driver also needs a fix for the issue reported in the same
> > thread, but requires a different approach because its TX completion
> > callback unmaps the scatterlist unconditionally. I will send that patch
> > separately.
>
> Again, these need to be tested on real hardware.  Not to mention the
> bugs that sashiko just found in the changes as well.
>
> thanks,
>
> greg k-h