Re: [PATCH 2/3] tty: serial: Add UART driver for Cortina-Access platform

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] Global array allocation, port slot selection, and cleanup lack synchronization, leading to race conditions, memory leaks, and UAF.
- [High] Kernel panic in `cortina_console_setup()` due to NULL pointer dereference when called for an unprobed UART index.
- [High] `cortina_poll_put_char()` uses `jiffies` for timeout calculations, resulting in an infinite loop and system hang.
- [High] Read-modify-write operations on the CFG register are performed without holding the port lock, leading to TOCTOU state corruption.
- [High] The console write function disables hardware interrupts and then takes a sleeping spinlock, triggering a lockdep splat on PREEMPT_RT.
- [Medium] The RX interrupt handler incorrectly applies a single, one-time error status to all characters drained from the FIFO.
- [Medium] Failed probes permanently leak port slots from the global `cortina_uart_ports` array.
- [Medium] The wakeup source initialized during probe is never disabled, causing a memory leak when the driver is unbound or fails to probe.
- [Medium] The `cortina_uart_shutdown` callback performs read-modify-write operations on the Interrupt Enable (IE) register without holding the port lock.
- [Low] The console write function transmits an incorrect LF-CR (`\n\r`) sequence instead of the standard CR-LF (`\r\n`).
--

commit fd0d487faa14545601b1e17f9d5e8a68bcd53bfd
Author: Jason Li <jason.lee651024@gmail.com>
Subject: tty: serial: Add UART driver for Cortina-Access platform

This commit introduces a new UART driver for the Cortina-Access SoC platform,
adding support for the CAXXXX family along with earlycon and console
functionality.

> diff --git a/drivers/tty/serial/serial_cortina-access.c b/drivers/tty/serial/serial_cortina-access.c
> new file mode 100644
> index 0000000000000..f25eae987ccdc
> --- /dev/null
> +++ b/drivers/tty/serial/serial_cortina-access.c

[ ... ]

> +static void cortina_uart_shutdown(struct uart_port *port)
> +{
> +	cortina_uart_stop_tx(port);
> +	cortina_uart_stop_rx(port);

[Severity: Medium]
Could this corrupt the IE register state? The serial core calls the shutdown
callback without holding the port lock, but the stop_tx and stop_rx functions
perform read-modify-write operations on the IE register.

> +	free_irq(port->irq, port);
> +}

[ ... ]

> +static void cortina_uart_set_termios(struct uart_port *port,
> +				     struct ktermios *termios,
> +				     const struct ktermios *old)
> +{
> +	unsigned long flags;
> +	int baud;
> +	unsigned int reg_v, sample_freq = 0;
> +
> +	baud = uart_get_baud_rate(port, termios, old, 0, 230400);
> +	reg_v = readl(port->membase + CFG);

[Severity: High]
Is the CFG register read vulnerable to TOCTOU corruption here? The CFG
register is read into a local variable outside the port lock, but it is 
written inside the lock later. If another context acquires the lock and
modifies CFG concurrently, it seems this could overwrite and lose those
changes.

> +	/* mask off the baud settings */
> +	reg_v &= 0xff;

[ ... ]

> +static void cortina_access_power(struct uart_port *port, unsigned int state,
> +				 unsigned int oldstate)
> +{
> +	unsigned int reg_v;
> +
> +	reg_v = readl(port->membase + CFG);

[Severity: High]
Does this need to hold the port lock? The CFG register is read and written
here with no locking, which might race with other register accesses.

> +	switch (state) {
> +	case UART_PM_STATE_ON:
> +		reg_v |= CFG_UART_EN;
> +		break;

[ ... ]

> +#ifdef CONFIG_CONSOLE_POLL
> +static int cortina_poll_get_char(struct uart_port *port)
> +{
> +	if (readl(port->membase + INFO) & INFO_RX_EMPTY)
> +		return NO_POLL_CHAR;
> +
> +	return readl(port->membase + RX_DAT);
> +}
> +
> +static void cortina_poll_put_char(struct uart_port *port, unsigned char c)
> +{
> +	unsigned long time_out;
> +
> +	time_out = jiffies + usecs_to_jiffies(TX_TIMEOUT);
> +
> +	while (time_before(jiffies, time_out) &&
> +	       (readl(port->membase + INFO) & INFO_TX_FULL))
> +		cpu_relax();

[Severity: High]
Will this loop hang the system? Since this polling function is called from the
kgdb framework with local hardware interrupts disabled, jiffies will not
increment. Should atomic polling loops use udelay and an iteration counter
instead?

> +
> +	/* Give up if FIFO stuck! */
> +	if (readl(port->membase + INFO) & INFO_TX_FULL)
> +		return;

[ ... ]

> +static inline void cortina_uart_interrupt_rx_chars(struct uart_port *port,
> +						   unsigned long status)
> +{

[ ... ]

> +	/* Read characters while FIFO is not empty */
> +	do {
> +		flg = TTY_NORMAL;
> +		port->icount.rx++;
> +		ch = readl(port->membase + RX_DAT);
> +		if (status & RX_PARITY_ERRE) {

[Severity: Medium]
Could this artificially inflate error statistics? The status argument is read
once before the loop, but it is evaluated against every character drained from
the FIFO. If an error bit was set initially, it looks like every subsequent
valid character in the batch is flagged as an error.

> +			port->icount.parity++;
> +			flg = TTY_PARITY;
> +		}

[ ... ]

> +#ifdef CONFIG_SERIAL_CORTINA_ACCESS_CONSOLE
> +static void cortina_console_write(struct console *co, const char *s,
> +				  unsigned int count)
> +{
> +	struct uart_port *port;
> +	struct cortina_uart_port *pca_port;
> +	unsigned int i, previous;
> +	unsigned long flags;
> +	int locked;
> +
> +	pca_port = cortina_uart_get_port(co->index);
> +	port = &pca_port->uart;
> +
> +	local_irq_save(flags);
> +	if (port->sysrq) {
> +		locked = 0;
> +	} else if (oops_in_progress) {
> +		locked = spin_trylock(&port->lock);
> +	} else {
> +		spin_lock(&port->lock);

[Severity: High]
Will this cause a sleeping-while-atomic lockdep warning on PREEMPT_RT?
Disabling hardware interrupts and then acquiring a sleeping lock (like
spinlock_t on RT kernels) is not allowed. Should this use
uart_port_lock_irqsave instead?

> +		locked = 1;
> +	}

[ ... ]

> +	/* Write all the chars */
> +	for (i = 0; i < count; i++) {
> +		/* Wait for the TX buffer to be empty */
> +		while (!(readl(port->membase + INFO) & INFO_TX_EMPTY))
> +			cpu_relax();
> +
> +		writel(*s, port->membase + TX_DAT);
> +
> +		/* CR/LF handling */
> +		if (*s++ == '\n') {

[Severity: Low]
Does this transmit an incorrect LF-CR sequence instead of the standard CR-LF?
It writes the current character and then appends a carriage return if the
character was a newline. Could this be simplified by using uart_console_write?

> +			while (!(readl(port->membase + INFO) & INFO_TX_EMPTY))
> +				cpu_relax();
> +			writel('\r', port->membase + TX_DAT);
> +		}
> +	}

[ ... ]

> +static int __init cortina_console_setup(struct console *co, char *options)
> +{
> +	struct uart_port *port;
> +	struct cortina_uart_port *pca_port;
> +	int baud = 115200;
> +	int bits = 8;
> +	int parity = 'n';
> +	int flow = 'n';
> +
> +	if (co->index < 0 || co->index >= UART_NR)
> +		return -ENODEV;
> +
> +	pca_port = cortina_uart_get_port(co->index);
> +	port = &pca_port->uart;
> +
> +	if (options)
> +		uart_parse_options(options, &baud, &parity, &bits, &flow);
> +
> +	return uart_set_options(port, co, baud, parity, bits, flow);

[Severity: High]
Can this cause a kernel panic if a console is requested on an unprobed port
index (e.g. via the command line)? It looks like this fetches a potentially
zero-initialized port from the global array and passes it to uart_set_options,
which will unconditionally dereference the port operations.

> +}

[ ... ]

> +static int serial_cortina_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct cortina_uart_port *port;
> +	struct resource *res;
> +	struct clk *pclk_info;
> +	int uart_idx;
> +	int irq;
> +	int ret;
> +
> +	if (!cortina_uart_ports) {
> +		cortina_uart_ports = kcalloc(UART_NR, sizeof(*cortina_uart_ports),
> +					     GFP_KERNEL);

[Severity: High]
Is there a race condition with this global array allocation? The
cortina_uart_ports array is allocated and searched without any locking. Could
concurrent probing of multiple instances lead to a double allocation and
memory leak?

> +		if (!cortina_uart_ports)
> +			return -ENOMEM;
> +	}
> +
> +	port = cortina_uart_ports;
> +	for (uart_idx = 0; uart_idx < UART_NR; ++uart_idx) {
> +		/* Find first empty slot */
> +		if (strlen(port->name) == 0)
> +			break;
> +		port++;
> +	}
> +
> +	if (uart_idx >= UART_NR)
> +		return -ENODEV;
> +
> +	snprintf(port->name, sizeof(port->name),
> +		 "Cortina-Access UART%d", uart_idx);

[Severity: Medium]
If a subsequent initialization step fails in the probe function, does this
permanently leak the port slot? The name is assigned to claim the slot, but
it doesn't appear to be cleared on the error paths.

[ ... ]

> +	if (of_property_read_bool(np, "wakeup-source"))
> +		port->may_wakeup = true;
> +	if (of_property_read_bool(np, "break-indicator"))
> +		port->has_bi = true;
> +
> +	if (port->may_wakeup)
> +		device_init_wakeup(&pdev->dev, true);

[Severity: Medium]
Does this leak the wakeup source memory? device_init_wakeup is called to
enable the wakeup source, but it is never disabled on the probe error path or
in the removal callback.

> +
> +	ret = uart_add_one_port(&cortina_uart_driver, &port->uart);

[ ... ]

> +static void serial_cortina_remove(struct platform_device *pdev)
> +{
> +	struct cortina_uart_port *pca_port = platform_get_drvdata(pdev);
> +
> +	if (pca_port) {
> +		memset(pca_port->name, 0, CA_UART_NAME_LEN);
> +		uart_remove_one_port(&cortina_uart_driver, &pca_port->uart);

[Severity: High]
Does clearing the slot name before unregistering the port open a race
condition? If the name is cleared early, a concurrent probe might hijack the
slot and overwrite the port state during active teardown.

> +	}
> +
> +	platform_set_drvdata(pdev, NULL);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610112821.3030099-1-jason.li@cortina-access.com?part=2