Re: [PATCH 03/11] USB: serial: io_edgeport: fix potential close() use-after-free

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: serial: io_edgeport: race between edge_close() and edge_write()
--

commit 611925474c1210ae646660fb9dfc233968cca260
Author: Johan Hovold <johan@kernel.org>

USB: serial: io_edgeport: fix potential close() use-after-free

This commit adds spinlock protection around clearing edge_port->open in
edge_close() so that a racing interrupt completion handler does not submit
more data while the port's write_urb and txfifo are being freed.

Link: https://sashiko.dev/#/patchset/20260714101316.823942-1-shpark061104%40gmail.com?part=1

> diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
> index 0cc4f41299e03..a2410c063ffe8 100644
> --- a/drivers/usb/serial/io_edgeport.c
> +++ b/drivers/usb/serial/io_edgeport.c
> @@ -1089,7 +1089,9 @@ static void edge_close(struct usb_serial_port *port)
>  
>  	/* port->close = true; */
>  	edge_port->closePending = false;
> +	spin_lock_irq(&edge_port->ep_lock);
>  	edge_port->open = false;
> +	spin_unlock_irq(&edge_port->ep_lock);
>  	edge_port->openPending = false;
>  
>  	usb_kill_urb(edge_port->write_urb);

[Severity: High]
While holding ep_lock around setting edge_port->open = false prevents
send_more_port_data() from racing with the deallocation of write_urb and
txfifo.fifo in edge_close(), could edge_write() still race with this
cleanup and dereference edge_port->txfifo.fifo while or after it is freed?

In drivers/usb/serial/io_edgeport.c, edge_close() does not zero
edge_port->txCredits (which was replenished during
block_until_chase_response()) and frees edge_port->txfifo.fifo outside
ep_lock:

	spin_lock_irq(&edge_port->ep_lock);
	edge_port->open = false;
	spin_unlock_irq(&edge_port->ep_lock);
	edge_port->openPending = false;

	usb_kill_urb(edge_port->write_urb);
	...
	kfree(edge_port->txfifo.fifo);
	edge_port->txfifo.fifo = NULL;

Meanwhile, edge_write() acquires ep_lock without checking edge_port->open
and copies data into fifo->fifo whenever edge_port->txCredits > fifo->count:

	spin_lock_irqsave(&edge_port->ep_lock, flags);

	/* calculate number of bytes to put in fifo */
	copySize = min_t(unsigned int, count,
				(edge_port->txCredits - fifo->count));
	...
	if (copySize == 0) {
		dev_dbg(&port->dev, "%s - copySize = Zero\n", __func__);
		goto finish_write;
	}
	...
	memcpy(&fifo->fifo[fifo->head], data, firsthalf);

During tty_port_close() -> tty_port_shutdown() -> edge_close(), before the
line discipline and flip-buffer workqueue are flushed or closed, concurrent
calls to edge_write() can still occur in two ways:

1. edge_interrupt_callback() checks edge_port->open outside ep_lock and
   calls tty_port_tty_wakeup() -> tty_wakeup() -> ld->ops->write_wakeup(),
   which in framing line disciplines (such as n_hdlc, ppp_async, slip,
   n_gsm, or hci_ldisc) synchronously invokes tty->ops->write() ->
   serial_write() -> edge_write().

2. Incoming receive data pushed via process_rcvd_data() -> edge_tty_recv() ->
   tty_flip_buffer_push() before edge_port->open becomes false is processed
   asynchronously by the flush_to_ldisc() workqueue during edge_close(). In
   n_tty, this can call edge_write() via n_tty_check_throttle() ->
   tty_throttle_safe() -> serial_throttle() -> edge_throttle() when ixoff
   is enabled (especially once the follow-up commit "usb: serial:
   io_edgeport: drop redundant open sanity checks" removes the unlocked
   !edge_port->open check from edge_throttle() and edge_unthrottle()), or
   via n_tty_receive_char_closing() -> process_echoes() -> tty_put_char()
   when ixon is enabled with pending echoes.

Should edge_write() also check edge_port->open under ep_lock (or should
edge_close() zero txCredits / free txfifo.fifo under ep_lock) to avoid a
use-after-free write or null pointer dereference on fifo->fifo?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021141154.2324793-1-johan@kernel.org?part=3