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

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 <[email protected]>

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