Re: [PATCH v2 1/2] serial: wk2xxx: Add WK2xxx SPI UART driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] Requesting the threaded IRQ before UART ports are fully initialized causes a NULL pointer dereference on early interrupts.
- [High] Unprotected lockless access to the UART transmit kfifo and `x_char` in `wk2xxx_handle_tx()`.
- [High] Race condition in polling worker initialization and cancellation can permanently stall polling.
- [High] SPI transfer buffers embedded in `struct wk2xxx_port` violate DMA cache coherency rules.
- [High] Early return in `wk2xxx_handle_tx` after sending `x_char` stalls pending transmission.
- [High] Improper teardown order in `wk2xxx_remove()` flushes the kworker before unregistering the port, leading to Use-After-Free.
- [High] Data race on the shared SPI transmit buffer `s->spi_tx` without lock protection.
- [High] Kernel panic via division by zero if `clock-frequency` device tree property is 0.
- [Medium] Data race on port state and statistics due to missing `port->lock` acquisition.
- [Medium] Improper teardown order in `wk2xxx_remove()` stalls the TX flushing process during unbind.
--

commit 6a58cb664a5c2176dca76b8cff9410c1dd264818
Author: zjzhao <zjzhao@edatec.cn>

serial: wk2xxx: Add WK2xxx SPI UART driver
    
Add a driver for the WK2xxx SPI to UART bridge ICs (WK2124, WK2132,
WK2168, WK2202 and WK2204) from WKmic (Chengdu Weikai Microelectronics).
Each IC exposes two or four full-duplex UART channels with 256-byte
RX/TX FIFOs through a single SPI slave interface and one interrupt line.

> diff --git a/drivers/tty/serial/wk2xxx.c b/drivers/tty/serial/wk2xxx.c
> index 0000000000000..77f288e452340
> --- /dev/null
> +++ b/drivers/tty/serial/wk2xxx.c
> @@ -176,11 +176,11 @@ struct wk2xxx_port {
>  	atomic_t			open_ports;
>  	/*
>  	 * Shared SPI transfer buffers. All SPI accesses are serialized by
>  	 * s->reg_lock, so these are never used concurrently.
>  	 */
>  	u8				spi_tx[WK2XXX_MAX_SPI_LEN + 1];
>  	u8				spi_rx[WK2XXX_MAX_SPI_LEN + 1];
>  	struct wk2xxx_one		p[];
>  };

[Severity: High]
Does embedding these buffers directly without ____cacheline_aligned risk
cacheline tearing during DMA operations?

When the SPI subsystem maps these buffers for DMA transfers on
non-cache-coherent architectures, cache invalidation during unmap could
destroy adjacent fields in the same cache line. If there are concurrent
CPU writes to neighboring fields like open_ports or polling, could they
be silently overwritten by stale cache data?

[ ... ]

> @@ -382,9 +382,9 @@ static int wk2xxx_fifo_read(struct wk2xxx_port *s, unsigned int portno,
>  
>  	if (len == 0 || len > WK2XXX_MAX_SPI_LEN)
>  		return -EINVAL;
>  
>  	memset(tx, 0, WK2XXX_MAX_SPI_LEN + 1);
>  	tx[0] = wk2xxx_port_addr(portno, WK2XXX_SPI_FIFO_READ);
>  
>  	guard(mutex)(&s->reg_lock);

[Severity: High]
Is it safe to modify the shared s->spi_tx buffer before acquiring the
s->reg_lock mutex?

If multiple threads execute wk2xxx_fifo_read or wk2xxx_fifo_write
concurrently, it seems they would mutate this unprotected buffer
simultaneously, corrupting the SPI commands.

[ ... ]

> @@ -487,9 +487,9 @@ static void wk2xxx_handle_tx(struct uart_port *port)
>  
>  	guard(mutex)(&one->tx_lock);
>  
>  	if (unlikely(port->x_char)) {
>  		wk2xxx_port_reg_write(s, portno, WK2XXX_FDAT_REG, port->x_char);
>  		port->icount.tx++;
>  		port->x_char = 0;
>  		return;
>  	}

[Severity: High]
Does this code modify port->x_char and port->icount.tx without holding
the required port->lock?

[Severity: High]
Also, does this early return stall pending transmission if an x_char is
sent?

By returning immediately, it bypasses the logic at the end of the function
that checks the xmit_fifo and re-enables the hardware transmit interrupt
(WK2XXX_SIER_TFTRIG_IEN_BIT), which might strand the remaining data in
the kfifo.

> @@ -511,8 +511,8 @@ static void wk2xxx_handle_tx(struct uart_port *port)
>  		txlen = WK2XXX_MAX_TX_CHARS;
>  
>  	to_send = kfifo_out_linear_ptr(&tport->xmit_fifo, &tail, txlen);
>  	sent = 0;
>  	while (to_send) {
>  		unsigned int chunk = min_t(unsigned int, to_send,
>  					   WK2XXX_MAX_SPI_LEN);

[Severity: High]
Is it safe to access and advance the serial core kfifo without holding
port->lock?

If the serial core concurrently executes an operation like
uart_flush_buffer (which calls kfifo_reset), the kfifo internal
state could be corrupted.

[ ... ]

> @@ -582,3 +582,3 @@ static void wk2xxx_handle_rx(struct uart_port *port)
>  	}
>  
>  	port->icount.rx += rxlen;

[Severity: Medium]
Is it safe to increment the receive byte counter without locking? This could
lead to lost updates if there are concurrent readers or writers.

[ ... ]

> @@ -819,4 +819,4 @@ static void wk2xxx_calc_divisor(unsigned long clk, unsigned int baud,
>  	unsigned int div, rem;
>  
>  	div = clk / (baud * 16);
>  	if (div == 0)

[Severity: High]
Could baud be zero here and cause a division by zero panic?

If clock-frequency is set to 0 in the device tree, uart_get_baud_rate
will fall back to returning 0, resulting in baud being 0 here.

[ ... ]

> @@ -849,6 +849,6 @@ static void wk2xxx_set_termios(struct uart_port *port, struct ktermios *termios,
>  
>  	/* Configure flow control. */
>  	port->status &= ~(UPSTAT_AUTOCTS | UPSTAT_AUTORTS);
>  	if (termios->c_cflag & CRTSCTS) {
>  		fwcr = WK2XXX_FWCR_FWM_RTS_CTS;
>  		port->status |= UPSTAT_AUTOCTS | UPSTAT_AUTORTS;
>  	}

[Severity: Medium]
Is it safe to modify port->status, port->read_status_mask, and
port->ignore_status_mask before the spinlock is acquired via
guard(uart_port_lock_irqsave) later in the function?

[ ... ]

> @@ -883,4 +883,4 @@ static int wk2xxx_startup(struct uart_port *port)
>  
>  	/* Start the shared polling loop when the first port is opened. */
>  	if (s->polling && atomic_inc_return(&s->open_ports) == 1)
>  		kthread_queue_delayed_work(&s->kworker, &s->poll_work,
>  					   msecs_to_jiffies(WK2XXX_POLL_PERIOD_MS));

[Severity: High]
Could this sequence race with port shutdown and permanently stall polling?

If port A shuts down concurrently with port B starting up, port B can queue
the delayed work just before port A executes
kthread_cancel_delayed_work_sync in wk2xxx_shutdown. This would leave
the polling loop cancelled even though open_ports is 1.

[ ... ]

> @@ -1054,5 +1054,5 @@ static int wk2xxx_probe(struct spi_device *spi)
>  		 */
>  		ret = request_threaded_irq(spi->irq, NULL, wk2xxx_irq,
>  					   IRQF_TRIGGER_LOW | IRQF_SHARED |
>  					   IRQF_ONESHOT, dev_name(dev), s);
>  		if (ret)

[Severity: High]
Can requesting the IRQ here before the UART ports are fully initialized cause
a NULL pointer dereference on early interrupts?

If the UART device has a pending interrupt when the driver is probed, the
handler wk2xxx_irq will execute and traverse down to
tty_flip_buffer_push(&port->state->port) in wk2xxx_handle_rx.
Because port->state is not initialized until uart_add_one_port is
called later, this results in a NULL pointer dereference.

[ ... ]

> @@ -1115,4 +1115,4 @@ static int wk2xxx_probe(struct spi_device *spi)
>  	/* Crystal clock; allow an optional DT override. */
>  	uartclk = devtype->crystal_freq;
>  	if (device_property_read_u32(dev, "clock-frequency", &clock_freq) == 0)
>  		uartclk = clock_freq;

[Severity: High]
Does this code apply the clock-frequency from the device tree without
validating it?

If the property is maliciously or accidentally set to 0, or a value less
than 16, it can lead to a division by zero panic in wk2xxx_calc_divisor.

[ ... ]

> @@ -1243,10 +1243,10 @@ static void wk2xxx_remove(struct spi_device *spi)
>  	 */
>  	if (s->irq_requested)
>  		free_irq(spi->irq, s);
>  
>  	if (s->polling)
>  		kthread_cancel_delayed_work_sync(&s->poll_work);
>  
>  	kthread_flush_worker(&s->kworker);
>  
>  	for (i = 0; i < s->devtype->nr_uart; i++) {
>  		uart_remove_one_port(&wk2xxx_uart, &s->p[i].port);

[Severity: Medium]
Does freeing the IRQ before unregistering the port stall the TX flushing
process during unbind?

When uart_remove_one_port is executed, it attempts to wait for the
transmit FIFO to drain. Since the driver relies on hardware transmit interrupts
to wake the worker and send chunks of data from the software kfifo, prematurely
freeing the IRQ disables this mechanism and the teardown sequence will hang
until internal timeouts trigger.

[Severity: High]
Also, does flushing the worker before unregistering the port lead to a
use-after-free?

While the worker is flushed, userspace or the TTY layer can still queue new
transmit works because the port remains fully registered. Once
uart_remove_one_port finishes, the port xmit_fifo buffer is
freed. A late tx_work that was queued during or immediately after the
flush will then execute and attempt to access the destroyed kfifo.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904093341.100826-1-zjzhao@edatec.cn?part=1