[PATCH v2] serial: qcom-geni: Drop unsafe rx_buf realloc from setup_fifos()

[PATCH v2] serial: qcom-geni: Drop unsafe rx_buf realloc from setup_fifos()

[Praveen Talari]

The RX buffer is allocated once during probe using a fixed DMA_RX_BUF_SIZE
and is DMA-mapped for the lifetime of the port. However, setup_fifos()
attempts to reallocate rx_buf whenever the reported RX FIFO depth changes.
Since the DMA mapping is not re-established after reallocation, the buffer
pointer may change while the DMA engine continues using the stale DMA
address. This can result in RX DMA targeting memory that no longer
corresponds to the active buffer, leading to invalid DMA accesses and
potential memory corruption.

The RX FIFO depth is unrelated to the size of rx_buf. The buffer is
allocated independently using DMA_RX_BUF_SIZE and all RX DMA paths consume
it at that fixed size. As such, resizing the buffer based on FIFO depth
changes provides no functional benefit.

Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
Changes in v2:
- Updated correct mail id.
- Link to v1: https://patch.msgid.link/20260824-drop-unsafe-rx-buf-realloc-from-setup-fifos-v1-1-52d231c840e1@oss.qualcomm.com
---
 drivers/tty/serial/qcom_geni_serial.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 3633723acef8..75b2e0b77d05 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -1291,7 +1291,6 @@ static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
 static int setup_fifos(struct qcom_geni_serial_port *port)
 {
 	struct uart_port *uport;
-	u32 old_rx_fifo_depth = port->rx_fifo_depth;
 
 	uport = &port->uport;
 	port->tx_fifo_depth = geni_se_get_tx_fifo_depth(&port->se);
@@ -1300,19 +1299,6 @@ static int setup_fifos(struct qcom_geni_serial_port *port)
 	uport->fifosize =
 		(port->tx_fifo_depth * port->tx_fifo_width) / BITS_PER_BYTE;
 
-	if (port->rx_buf && (old_rx_fifo_depth != port->rx_fifo_depth) && port->rx_fifo_depth) {
-		/*
-		 * Use krealloc rather than krealloc_array because rx_buf is
-		 * accessed as 1 byte entries as well as 4 byte entries so it's
-		 * not necessarily an array.
-		 */
-		port->rx_buf = devm_krealloc(uport->dev, port->rx_buf,
-					     port->rx_fifo_depth * sizeof(u32),
-					     GFP_KERNEL);
-		if (!port->rx_buf)
-			return -ENOMEM;
-	}
-
 	return 0;
 }
 

---
base-commit: 903c1cf6dff9964e71eda98a39e2e5d442050472
change-id: 20260822-drop-unsafe-rx-buf-realloc-from-setup-fifos-2b82eb0cb5d4

Best regards,
--  
Praveen Talari <praveen.talari@oss.qualcomm.com>

Re: [PATCH v2] serial: qcom-geni: Drop unsafe rx_buf realloc from setup_fifos()

[sashiko-bot]

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

Pre-existing issues:
- [High] Preexisting invalid DMA unmap in probe error path.
- [High] Missing bounds check on hardware-reported RX length leads to out-of-bounds read and kernel memory leak.
--

--- Patch [1]: [PATCH v2] serial: qcom-geni: Drop unsafe rx_buf realloc from setup_fifos() ---
commit 03f147201f8b1d9aa9d2d010a8177ebcd9b90c6e
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>

serial: qcom-geni: Drop unsafe rx_buf realloc from setup_fifos()

The RX buffer is allocated once during probe using a fixed DMA_RX_BUF_SIZE
and is DMA-mapped for the lifetime of the port. However, setup_fifos()
attempts to reallocate rx_buf whenever the reported RX FIFO depth changes.
This commit drops this unsafe reallocation.

Link: https://patch.msgid.link/20260824-drop-unsafe-rx-buf-realloc-from-setup-fifos-v1-1-52d231c840e1@oss.qualcomm.com

> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 3633723acef8d..75b2e0b77d059 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
[ ... ]

drivers/tty/serial/qcom_geni_serial.c:qcom_geni_serial_probe() {
    ...
		port->rx_dma_addr = dma_map_single(pdev->dev.parent, port->rx_buf,
						   DMA_RX_BUF_SIZE, DMA_FROM_DEVICE);
		if (dma_mapping_error(pdev->dev.parent, port->rx_dma_addr)) {
			ret = -EIO;
			dev_err(&pdev->dev, "Failed to map RX DMA buffer: %d\n", ret);
			goto error;
		}
    ...
error:
	if (port->rx_dma_addr) {
		dma_unmap_single(pdev->dev.parent, port->rx_dma_addr,
				 DMA_RX_BUF_SIZE, DMA_FROM_DEVICE);
		port->rx_dma_addr = 0;
	}
    ...
}

[Severity: High]
This is a pre-existing issue, but does this error path incorrectly unmap an
invalid DMA address?

If dma_map_single() fails, it returns a mapping error (typically ~0ULL).
The code leaves this error value in port->rx_dma_addr and jumps to the error
label. Because port->rx_dma_addr evaluates to true, the cleanup block calls
dma_unmap_single() on this invalid address.

On systems with IOMMU or DMA debugging enabled, could this cause kernel warnings
or memory corruption when the IOMMU translation tables are accessed with an
invalid IOVA?

drivers/tty/serial/qcom_geni_serial.c:qcom_geni_serial_handle_rx_dma() {
    ...
	rx_in = readl(uport->membase + SE_DMA_RX_LEN_IN);
	if (!rx_in)
		dev_warn_ratelimited(uport->dev, "serial engine reports 0 RX bytes in!\n");
	else if (!drop) {
		trace_geni_serial_rx_data(uport->dev, port->rx_buf, rx_in);
		handle_rx_uart(uport, rx_in);
	}
    ...
}

[Severity: High]
This is a pre-existing issue, but is rx_in used without being bounded by
DMA_RX_BUF_SIZE?

The length is read directly from the SE_DMA_RX_LEN_IN register and passed to
handle_rx_uart() without verifying that it does not exceed the size of the DMA
buffer.

If the hardware device reports an incorrect length, could this lead to an
out-of-bounds read when copying from port->rx_buf and leak adjacent kernel
memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824-drop-unsafe-rx-buf-realloc-from-setup-fifos-v2-1-9a1c8f711e98@oss.qualcomm.com?part=1