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

[PATCH] 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 <ptalari@qualcomm.com>
---
Signed-off-by: Praveen Talari <praveen.talari@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] 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] ida_free() is called before uart_remove_one_port() in qcom_geni_serial_remove().
- [High] The RX DMA length reported by the hardware is not validated against the allocated buffer size, which can lead to out-of-bounds memory reads and information leaks.
--

--- Patch [1]: [PATCH] serial: qcom-geni: Drop unsafe rx_buf realloc from setup_fifos() ---
commit 78bd555a9d69466b1db67c194aa2b0540f9577ee
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 size and is
DMA-mapped for the lifetime of the port. This removes the rx_buf
reallocation from setup_fifos() to prevent the DMA engine from accessing
freed memory when the buffer pointer changes.

> 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
[ ... ]
> @@ -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;
>  }

[Severity: High]
This is a pre-existing issue, but is it possible for a concurrent driver probe
to allocate the same line ID and call uart_add_one_port() while
uart_remove_one_port() is still executing?

Looking at qcom_geni_serial_remove() in drivers/tty/serial/qcom_geni_serial.c:

	ida_free(&port_ida, uport->line);
	uart_remove_one_port(drv, &port->uport);

Since ida_free() is called before completely unregistering the port, it
appears a race condition could occur if the device is unbound. A concurrent
probe could reuse the freed IDA while the removal thread is waiting for the
port mutex, which might incorrectly tear down the newly added port's state
and result in a use-after-free.

[Severity: High]
This is a pre-existing issue, but does the hardware-reported DMA length get
validated against the allocated buffer size?

Looking at qcom_geni_serial_handle_rx_dma() in
drivers/tty/serial/qcom_geni_serial.c, the length is read directly from the
device:

	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);
	}

This unvalidated rx_in length is then passed to handle_rx_uart():

	ret = tty_insert_flip_string(tport, port->rx_buf, bytes);

If a compromised hardware device or firmware reports an rx_in value larger
than the buffer size (DMA_RX_BUF_SIZE), could this lead to an out-of-bounds
read from port->rx_buf and expose kernel heap data to user-space?

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

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

[Greg Kroah-Hartman]

On Mon, Aug 24, 2026 at 04:15:14PM +0530, Praveen Talari wrote:
> 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 <ptalari@qualcomm.com>
> ---
> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>

Why is this signed-off-by twice, with different email addresses?

confused,

greg k-h

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

[Praveen Talari]

Hi

On 24-08-2026 18:26, Greg Kroah-Hartman wrote:
> On Mon, Aug 24, 2026 at 04:15:14PM +0530, Praveen Talari wrote:
>> 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 <ptalari@qualcomm.com>
>> ---
>> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
> Why is this signed-off-by twice, with different email addresses?

Apologies, that was a mistake while updating the commit message. Only 
praveen.talari@oss.qualcomm.com should be present for upstream 
submissions. I'll fix it in the next revision

Thanks,

Praveen Talari

>
> confused,
>
> greg k-h