Re: [PATCH 02/11] USB: serial: io_edgeport: fix potential fifo corruption

[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 in process_rcvd_status() allows FIFO corruption
--

commit 3c973c9c441546b1507dc5bafcc938334e398bb6
Author: Johan Hovold <johan@kernel.org>

USB: serial: io_edgeport: fix potential fifo corruption

This commit initializes maxTxCredits to zero when opening a port and rejects
subsequent open responses in process_rcvd_status() if maxTxCredits is already
non-zero, preventing a device from changing the buffer size after the tx fifo
has been allocated.

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 cdf41e86ba448..0cc4f41299e03 100644
> --- a/drivers/usb/serial/io_edgeport.c
> +++ b/drivers/usb/serial/io_edgeport.c
> @@ -878,7 +878,11 @@ static int edge_open(struct tty_struct *tty, struct usb_serial_port *port)
>  	}
>  
>  	/* initialize our port settings */
> +	spin_lock_irq(&edge_port->ep_lock);
>  	edge_port->txCredits = 0;	/* Can't send any data yet */
> +	edge_port->maxTxCredits = 0;
> +	spin_unlock_irq(&edge_port->ep_lock);
> +
>  	/* Must always set this bit to enable ints! */
>  	edge_port->shadowMCR = MCR_MASTER_IE;
>  	edge_port->chaseResponsePending = false;
> @@ -1750,6 +1754,7 @@ static void process_rcvd_status(struct edgeport_serial *edge_serial,
>  	struct tty_struct *tty;
>  	struct device *dev;
>  	__u8 code = edge_serial->rxStatusCode;
> +	unsigned long flags;
>  
>  	/* switch the port pointer to the one being currently talked about */
>  	if (edge_serial->rxPort >= edge_serial->serial->num_ports)
> @@ -1793,10 +1798,17 @@ static void process_rcvd_status(struct edgeport_serial *edge_serial,
>  	}
>  
>  	if (code == IOSP_STATUS_OPEN_RSP) {
> +		spin_lock_irqsave(&edge_port->ep_lock, flags);
> +		/* reject stray open responses to avoid FIFO corruption */
> +		if (edge_port->maxTxCredits != 0) {
> +			spin_unlock_irqrestore(&edge_port->ep_lock, flags);
> +			return;
> +		}
>  		edge_port->txCredits = GET_TX_BUFFER_SIZE(byte3);
>  		edge_port->maxTxCredits = edge_port->txCredits;
>  		dev_dbg(dev, "%s - Port %u Open Response Initial MSR = %02x TxBufferSize = %d\n",
>  			__func__, edge_serial->rxPort, byte2, edge_port->txCredits);
> +		spin_unlock_irqrestore(&edge_port->ep_lock, flags);
>  		handle_new_msr(edge_port, byte2);
>  
>  		/* send the current line settings to the port so we are

[Severity: High]
Could an unsolicited IOSP_STATUS_OPEN_RSP message race with edge_open() in
drivers/usb/serial/io_edgeport.c and still corrupt edge_port->txfifo?

In process_rcvd_status(), edge_port->ep_lock only covers the check and
assignment of edge_port->maxTxCredits, without checking
edge_port->openPending, and the lock is dropped before clearing
edge_port->openPending:

	if (code == IOSP_STATUS_OPEN_RSP) {
		spin_lock_irqsave(&edge_port->ep_lock, flags);
		/* reject stray open responses to avoid FIFO corruption */
		if (edge_port->maxTxCredits != 0) {
			spin_unlock_irqrestore(&edge_port->ep_lock, flags);
			return;
		}
		edge_port->txCredits = GET_TX_BUFFER_SIZE(byte3);
		edge_port->maxTxCredits = edge_port->txCredits;
		...
		spin_unlock_irqrestore(&edge_port->ep_lock, flags);
		handle_new_msr(edge_port, byte2);
		...
		/* we have completed the open */
		edge_port->openPending = false;
		edge_port->open = true;
		wake_up(&edge_port->wait_open);
		return;
	}

Meanwhile, in edge_open(), edge_port->maxTxCredits is cleared under
edge_port->ep_lock before edge_port->openPending is set outside the lock, and
after wait_event_timeout() returns, edge_port->maxTxCredits is read without
holding edge_port->ep_lock:

	spin_lock_irq(&edge_port->ep_lock);
	edge_port->txCredits = 0;	/* Can't send any data yet */
	edge_port->maxTxCredits = 0;
	spin_unlock_irq(&edge_port->ep_lock);
	...
	edge_port->openPending = true;
	edge_port->open        = false;
	response = send_iosp_ext_cmd(edge_port, IOSP_CMD_OPEN_PORT, 0);
	...
	wait_event_timeout(edge_port->wait_open, !edge_port->openPending,
								OPEN_TIMEOUT);
	...
	edge_port->txfifo.size	= edge_port->maxTxCredits;
	edge_port->txfifo.fifo	= kmalloc(edge_port->maxTxCredits, GFP_KERNEL);

If an unsolicited IOSP_STATUS_OPEN_RSP arrives on the bulk-in endpoint (which
can already be active from edge_startup(), another open port, or the
interrupt urb submitted earlier in edge_open()) and runs the ep_lock section
in process_rcvd_status() just before edge_open() takes ep_lock:

1. edge_open() resets edge_port->maxTxCredits = 0 under ep_lock and sets
   edge_port->openPending = true.
2. The in-flight process_rcvd_status() finishes handle_new_msr() and
   change_port_settings(), then sets edge_port->openPending = false and
   edge_port->open = true, waking edge_open().
3. edge_open() observes edge_port->maxTxCredits == 0, sets
   edge_port->txfifo.size = 0, and calls kmalloc(0, GFP_KERNEL), storing
   ZERO_SIZE_PTR in edge_port->txfifo.fifo.
4. Because edge_port->maxTxCredits was reset to 0, a second
   IOSP_STATUS_OPEN_RSP passes the edge_port->maxTxCredits != 0 check in
   process_rcvd_status() and sets edge_port->txCredits and
   edge_port->maxTxCredits to a non-zero value (or races between setting
   edge_port->txfifo.size and calling kmalloc()).
5. Subsequent edge_write() calls then compute a non-zero copySize from
   edge_port->txCredits while edge_port->txfifo.size is 0, causing memcpy()
   to dereference ZERO_SIZE_PTR or overflow edge_port->txfifo.fifo.

In this commit, an unsolicited IOSP_STATUS_OPEN_RSP on a closed port also
sets edge_port->open = true while edge_port->txfifo.fifo and
edge_port->write_urb are null. Follow-up commits ("USB: serial: io_edgeport:
fix potential NULL-deref at open" and "USB: serial: io_edgeport: move initial
termios update to open()") move setting edge_port->open = true and calling
change_port_settings() into edge_open(), but the unsynchronized
openPending and maxTxCredits race remains present.

Should edge_port->openPending also be synchronized under edge_port->ep_lock
and checked in process_rcvd_status() before accepting IOSP_STATUS_OPEN_RSP?

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