[PATCH 00/11] USB: serial: io_edgeport: fixes and sync commands

[PATCH 00/11] USB: serial: io_edgeport: fixes and sync commands

[Johan Hovold]

This series fixes a NULL-deref triggered by syzbot, a memory corruption
and a UAF flagged by Sashiko, and another NULL-deref found through
inspection.

Also add the missing serialisation for MCR, drop the modem lines on
hangup and close, and switch to synchronous commands so that operations
are serialised as expected.

Johan


Johan Hovold (11):
  USB: serial: io_edgeport: fix NULL-deref on early status
  USB: serial: io_edgeport: fix potential fifo corruption
  USB: serial: io_edgeport: fix potential close() use-after-free
  USB: serial: io_edgeport: fix potential NULL-deref at open
  USB: serial: io_edgeport: serialise MCR accesses
  USB: serial: io_edgeport: drop redundant sanity checks
  USB: serial: io_edgeport: drop redundant open sanity checks
  USB: serial: io_edgeport: move initial termios update to open()
  USB: serial: io_edgeport: deassert DTR/RTS on close() and B0
  USB: serial: io_edgeport: replace atomic command allocations
  USB: serial: io_edgeport: switch to synchronous commands

 drivers/usb/serial/io_edgeport.c | 277 +++++++++++--------------------
 1 file changed, 93 insertions(+), 184 deletions(-)

-- 
2.55.0

[PATCH 01/11] USB: serial: io_edgeport: fix NULL-deref on early status

[Johan Hovold]

The driver submits the interrupt-in URB during probe or on first port
open, which on completion may submit the bulk-in URB before all ports
have been opened and their waitqueues have been initialised.

If a (malicious) device sends a status message for a port that has not
yet been opened this may lead to a NULL-pointer dereference in the
bulk-in completion handler when attempting to wakeup blocked tasks.

Move waitqueue initialisation from open() to port_probe() to address
this.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: syzbot+763813d65c7e18c1b6d5@syzkaller.appspotmail.com
Link: https://lore.kernel.org/6a8818d8.ae6ddae5.3da009.0031.GAE@google.com
Cc: stable@vger.kernel.org
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/io_edgeport.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
index eaa8f00716c8..cdf41e86ba44 100644
--- a/drivers/usb/serial/io_edgeport.c
+++ b/drivers/usb/serial/io_edgeport.c
@@ -877,11 +877,6 @@ static int edge_open(struct tty_struct *tty, struct usb_serial_port *port)
 		}
 	}
 
-	/* initialize our wait queues */
-	init_waitqueue_head(&edge_port->wait_open);
-	init_waitqueue_head(&edge_port->wait_chase);
-	init_waitqueue_head(&edge_port->wait_command);
-
 	/* initialize our port settings */
 	edge_port->txCredits = 0;	/* Can't send any data yet */
 	/* Must always set this bit to enable ints! */
@@ -2965,6 +2960,11 @@ static int edge_port_probe(struct usb_serial_port *port)
 		return -ENOMEM;
 
 	spin_lock_init(&edge_port->ep_lock);
+
+	init_waitqueue_head(&edge_port->wait_open);
+	init_waitqueue_head(&edge_port->wait_chase);
+	init_waitqueue_head(&edge_port->wait_command);
+
 	edge_port->port = port;
 
 	usb_set_serial_port_data(port, edge_port);
-- 
2.55.0

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

[Johan Hovold]

The device buffer size is determined during open() and the driver
allocates a TX FIFO of corresponding size.

Add the missing sanity check to prevent memory corruption in case a
malicious device sends a second open response with a different buffer
size.

This issue was flagged by Sashiko when reviewing a related fix.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Link: https://sashiko.dev/#/patchset/20260714101316.823942-1-shpark061104%40gmail.com?part=1
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/io_edgeport.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
index cdf41e86ba44..0cc4f41299e0 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
-- 
2.55.0

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

[Johan Hovold]

Add the missing locking when updating the port status during close() to
prevent a racing interrupt completion handler from trying to submit more
data for the port while its write URB and FIFO are being deallocated.

This issue was flagged by Sashiko when reviewing a related fix.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Link: https://sashiko.dev/#/patchset/20260714101316.823942-1-shpark061104%40gmail.com?part=1
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/io_edgeport.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
index 0cc4f41299e0..a2410c063ffe 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);
-- 
2.55.0

[PATCH 04/11] USB: serial: io_edgeport: fix potential NULL-deref at open

[Johan Hovold]

Do not mark the port as open until after the write FIFO has been
allocated and initialised to prevent a racing interrupt completion from
attempting to send stale data and dereferencing the FIFO buffer pointer.

Note that the open flag defaults to false and is cleared on close() so
there is no need to reinitialise it on every open().

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/io_edgeport.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
index a2410c063ffe..38f58ca115eb 100644
--- a/drivers/usb/serial/io_edgeport.c
+++ b/drivers/usb/serial/io_edgeport.c
@@ -889,9 +889,8 @@ static int edge_open(struct tty_struct *tty, struct usb_serial_port *port)
 
 	/* send a open port command */
 	edge_port->openPending = true;
-	edge_port->open        = false;
-	response = send_iosp_ext_cmd(edge_port, IOSP_CMD_OPEN_PORT, 0);
 
+	response = send_iosp_ext_cmd(edge_port, IOSP_CMD_OPEN_PORT, 0);
 	if (response < 0) {
 		dev_err(dev, "%s - error sending open port command\n", __func__);
 		edge_port->openPending = false;
@@ -901,8 +900,7 @@ static int edge_open(struct tty_struct *tty, struct usb_serial_port *port)
 	/* now wait for the port to be completely opened */
 	wait_event_timeout(edge_port->wait_open, !edge_port->openPending,
 								OPEN_TIMEOUT);
-
-	if (!edge_port->open) {
+	if (edge_port->openPending) {
 		/* open timed out */
 		dev_dbg(dev, "%s - open timeout\n", __func__);
 		edge_port->openPending = false;
@@ -930,6 +928,10 @@ static int edge_open(struct tty_struct *tty, struct usb_serial_port *port)
 		return -ENOMEM;
 	}
 
+	spin_lock_irq(&edge_port->ep_lock);
+	edge_port->open = true;
+	spin_unlock_irq(&edge_port->ep_lock);
+
 	dev_dbg(dev, "%s - Initialize TX fifo to %d bytes\n",
 		__func__, edge_port->maxTxCredits);
 
@@ -1092,7 +1094,6 @@ static void edge_close(struct usb_serial_port *port)
 	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);
 
@@ -1824,7 +1825,6 @@ static void process_rcvd_status(struct edgeport_serial *edge_serial,
 
 		/* we have completed the open */
 		edge_port->openPending = false;
-		edge_port->open = true;
 		wake_up(&edge_port->wait_open);
 		return;
 	}
-- 
2.55.0

[PATCH 05/11] USB: serial: io_edgeport: serialise MCR accesses

[Johan Hovold]

Add a mutex to serialise modem control register updates to prevent
concurrent tasks from interfering with each other.

Note that the register updates are currently asynchronous so a task may
read back a value which has not yet taken effect.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/io_edgeport.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
index 38f58ca115eb..0b50c6b99b0a 100644
--- a/drivers/usb/serial/io_edgeport.c
+++ b/drivers/usb/serial/io_edgeport.c
@@ -168,6 +168,7 @@ struct edgeport_port {
 	struct urb		*write_urb;		/* write URB for this port */
 	bool			write_in_progress;	/* 'true' while a write URB is outstanding */
 	spinlock_t		ep_lock;
+	struct mutex		mutex;
 
 	__u8			shadowLCR;		/* last LCR value received */
 	__u8			shadowMCR;		/* last MCR value received */
@@ -884,7 +885,10 @@ static int edge_open(struct tty_struct *tty, struct usb_serial_port *port)
 	spin_unlock_irq(&edge_port->ep_lock);
 
 	/* Must always set this bit to enable ints! */
+	mutex_lock(&edge_port->mutex);
 	edge_port->shadowMCR = MCR_MASTER_IE;
+	mutex_unlock(&edge_port->mutex);
+
 	edge_port->chaseResponsePending = false;
 
 	/* send a open port command */
@@ -1397,9 +1401,11 @@ static void edge_throttle(struct tty_struct *tty)
 
 	/* if we are implementing RTS/CTS, toggle that line */
 	if (C_CRTSCTS(tty)) {
+		mutex_lock(&edge_port->mutex);
 		edge_port->shadowMCR &= ~MCR_RTS;
 		status = send_cmd_write_uart_register(edge_port, MCR,
 							edge_port->shadowMCR);
+		mutex_unlock(&edge_port->mutex);
 		if (status != 0)
 			return;
 	}
@@ -1434,9 +1440,11 @@ static void edge_unthrottle(struct tty_struct *tty)
 	}
 	/* if we are implementing RTS/CTS, toggle that line */
 	if (C_CRTSCTS(tty)) {
+		mutex_lock(&edge_port->mutex);
 		edge_port->shadowMCR |= MCR_RTS;
 		send_cmd_write_uart_register(edge_port, MCR,
 						edge_port->shadowMCR);
+		mutex_unlock(&edge_port->mutex);
 	}
 }
 
@@ -1501,6 +1509,7 @@ static int edge_tiocmset(struct tty_struct *tty,
 	struct edgeport_port *edge_port = usb_get_serial_port_data(port);
 	unsigned int mcr;
 
+	mutex_lock(&edge_port->mutex);
 	mcr = edge_port->shadowMCR;
 	if (set & TIOCM_RTS)
 		mcr |= MCR_RTS;
@@ -1519,6 +1528,7 @@ static int edge_tiocmset(struct tty_struct *tty,
 	edge_port->shadowMCR = mcr;
 
 	send_cmd_write_uart_register(edge_port, MCR, edge_port->shadowMCR);
+	mutex_unlock(&edge_port->mutex);
 
 	return 0;
 }
@@ -1532,6 +1542,7 @@ static int edge_tiocmget(struct tty_struct *tty)
 	unsigned int mcr;
 
 	msr = edge_port->shadowMSR;
+	mutex_lock(&edge_port->mutex);
 	mcr = edge_port->shadowMCR;
 	result = ((mcr & MCR_DTR)	? TIOCM_DTR: 0)	  /* 0x002 */
 		  | ((mcr & MCR_RTS)	? TIOCM_RTS: 0)   /* 0x004 */
@@ -1539,6 +1550,7 @@ static int edge_tiocmget(struct tty_struct *tty)
 		  | ((msr & EDGEPORT_MSR_CD)	? TIOCM_CAR: 0)   /* 0x040 */
 		  | ((msr & EDGEPORT_MSR_RI)	? TIOCM_RI:  0)   /* 0x080 */
 		  | ((msr & EDGEPORT_MSR_DSR)	? TIOCM_DSR: 0);  /* 0x100 */
+	mutex_unlock(&edge_port->mutex);
 
 	return result;
 }
@@ -2477,12 +2489,14 @@ static void change_port_settings(struct tty_struct *tty,
 		return;
 
 	/* set up the MCR register and send it to the EdgePort */
+	mutex_lock(&edge_port->mutex);
 	edge_port->shadowMCR = MCR_MASTER_IE;
 	if (cflag & CBAUD)
 		edge_port->shadowMCR |= (MCR_DTR | MCR_RTS);
 
 	status = send_cmd_write_uart_register(edge_port, MCR,
 						edge_port->shadowMCR);
+	mutex_unlock(&edge_port->mutex);
 	if (status != 0)
 		return;
 
@@ -2974,6 +2988,7 @@ static int edge_port_probe(struct usb_serial_port *port)
 		return -ENOMEM;
 
 	spin_lock_init(&edge_port->ep_lock);
+	mutex_init(&edge_port->mutex);
 
 	init_waitqueue_head(&edge_port->wait_open);
 	init_waitqueue_head(&edge_port->wait_chase);
-- 
2.55.0

[PATCH 06/11] USB: serial: io_edgeport: drop redundant sanity checks

[Johan Hovold]

The serial and port driver data is allocated at probe() before
registering the ports and will never be NULL when open(), close(),
write(), throttle() and unthrottle() are called.

Drop the redundant driver data sanity checks.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/io_edgeport.c | 32 ++++----------------------------
 1 file changed, 4 insertions(+), 28 deletions(-)

diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
index 0b50c6b99b0a..b8e78c749f9c 100644
--- a/drivers/usb/serial/io_edgeport.c
+++ b/drivers/usb/serial/io_edgeport.c
@@ -818,20 +818,13 @@ static void edge_bulk_out_cmd_callback(struct urb *urb)
 static int edge_open(struct tty_struct *tty, struct usb_serial_port *port)
 {
 	struct edgeport_port *edge_port = usb_get_serial_port_data(port);
+	struct usb_serial *serial = port->serial;
+	struct edgeport_serial *edge_serial = usb_get_serial_data(serial);
 	struct device *dev = &port->dev;
-	struct usb_serial *serial;
-	struct edgeport_serial *edge_serial;
 	int response;
 
-	if (edge_port == NULL)
-		return -ENODEV;
-
 	/* see if we've set up our endpoint info yet (can't set it up
 	   in edge_startup as the structures were not set up at that time.) */
-	serial = port->serial;
-	edge_serial = usb_get_serial_data(serial);
-	if (edge_serial == NULL)
-		return -ENODEV;
 	if (edge_serial->interrupt_in_buffer == NULL) {
 		struct usb_serial_port *port0 = serial->port[0];
 
@@ -1058,15 +1051,10 @@ static void block_until_tx_empty(struct edgeport_port *edge_port)
  *****************************************************************************/
 static void edge_close(struct usb_serial_port *port)
 {
-	struct edgeport_serial *edge_serial;
-	struct edgeport_port *edge_port;
+	struct edgeport_serial *edge_serial = usb_get_serial_data(port->serial);
+	struct edgeport_port *edge_port = usb_get_serial_port_data(port);
 	int status;
 
-	edge_serial = usb_get_serial_data(port->serial);
-	edge_port = usb_get_serial_port_data(port);
-	if (edge_serial == NULL || edge_port == NULL)
-		return;
-
 	/* block until tx is empty */
 	block_until_tx_empty(edge_port);
 
@@ -1130,9 +1118,6 @@ static int edge_write(struct tty_struct *tty, struct usb_serial_port *port,
 	int secondhalf;
 	unsigned long flags;
 
-	if (edge_port == NULL)
-		return -ENODEV;
-
 	/* get a pointer to the Tx fifo */
 	fifo = &edge_port->txfifo;
 
@@ -1383,9 +1368,6 @@ static void edge_throttle(struct tty_struct *tty)
 	struct edgeport_port *edge_port = usb_get_serial_port_data(port);
 	int status;
 
-	if (edge_port == NULL)
-		return;
-
 	if (!edge_port->open) {
 		dev_dbg(&port->dev, "%s - port not opened\n", __func__);
 		return;
@@ -1423,9 +1405,6 @@ static void edge_unthrottle(struct tty_struct *tty)
 	struct edgeport_port *edge_port = usb_get_serial_port_data(port);
 	int status;
 
-	if (edge_port == NULL)
-		return;
-
 	if (!edge_port->open) {
 		dev_dbg(&port->dev, "%s - port not opened\n", __func__);
 		return;
@@ -1460,9 +1439,6 @@ static void edge_set_termios(struct tty_struct *tty,
 {
 	struct edgeport_port *edge_port = usb_get_serial_port_data(port);
 
-	if (edge_port == NULL)
-		return;
-
 	if (!edge_port->open) {
 		dev_dbg(&port->dev, "%s - port not opened\n", __func__);
 		return;
-- 
2.55.0

[PATCH 07/11] USB: serial: io_edgeport: drop redundant open sanity checks

[Johan Hovold]

The throttle(), unthrottle() and set_termios() callbacks will not be
called unless the port is open so drop the redundant sanity checks.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/io_edgeport.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
index b8e78c749f9c..cccdcfb203e1 100644
--- a/drivers/usb/serial/io_edgeport.c
+++ b/drivers/usb/serial/io_edgeport.c
@@ -1368,11 +1368,6 @@ static void edge_throttle(struct tty_struct *tty)
 	struct edgeport_port *edge_port = usb_get_serial_port_data(port);
 	int status;
 
-	if (!edge_port->open) {
-		dev_dbg(&port->dev, "%s - port not opened\n", __func__);
-		return;
-	}
-
 	/* if we are implementing XON/XOFF, send the stop character */
 	if (I_IXOFF(tty)) {
 		unsigned char stop_char = STOP_CHAR(tty);
@@ -1405,11 +1400,6 @@ static void edge_unthrottle(struct tty_struct *tty)
 	struct edgeport_port *edge_port = usb_get_serial_port_data(port);
 	int status;
 
-	if (!edge_port->open) {
-		dev_dbg(&port->dev, "%s - port not opened\n", __func__);
-		return;
-	}
-
 	/* if we are implementing XON/XOFF, send the start character */
 	if (I_IXOFF(tty)) {
 		unsigned char start_char = START_CHAR(tty);
@@ -1439,11 +1429,6 @@ static void edge_set_termios(struct tty_struct *tty,
 {
 	struct edgeport_port *edge_port = usb_get_serial_port_data(port);
 
-	if (!edge_port->open) {
-		dev_dbg(&port->dev, "%s - port not opened\n", __func__);
-		return;
-	}
-
 	/* change the port settings to the new ones specified */
 	change_port_settings(tty, edge_port, old_termios);
 }
-- 
2.55.0

[PATCH 08/11] USB: serial: io_edgeport: move initial termios update to open()

[Johan Hovold]

Explicitly update the terminal settings on open() instead of doing so
from the interrupt completion handler when receiving an open command
response, which in theory can be received after a failed open attempt.

Note that this will also allow for switching to synchronous commands.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/io_edgeport.c | 56 ++++++--------------------------
 1 file changed, 10 insertions(+), 46 deletions(-)

diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
index cccdcfb203e1..acaac342f43f 100644
--- a/drivers/usb/serial/io_edgeport.c
+++ b/drivers/usb/serial/io_edgeport.c
@@ -282,8 +282,7 @@ static void handle_new_lsr(struct edgeport_port *edge_port, __u8 lsrData,
 static int  send_iosp_ext_cmd(struct edgeport_port *edge_port, __u8 command,
 				__u8 param);
 static int  calc_baud_rate_divisor(struct device *dev, int baud_rate, int *divisor);
-static void change_port_settings(struct tty_struct *tty,
-				struct edgeport_port *edge_port,
+static void edge_set_termios(struct tty_struct *tty, struct usb_serial_port *port,
 				const struct ktermios *old_termios);
 static int  send_cmd_write_uart_register(struct edgeport_port *edge_port,
 				__u8 regNum, __u8 regValue);
@@ -925,6 +924,9 @@ static int edge_open(struct tty_struct *tty, struct usb_serial_port *port)
 		return -ENOMEM;
 	}
 
+	if (tty)
+		edge_set_termios(tty, port, NULL);
+
 	spin_lock_irq(&edge_port->ep_lock);
 	edge_port->open = true;
 	spin_unlock_irq(&edge_port->ep_lock);
@@ -1418,22 +1420,6 @@ static void edge_unthrottle(struct tty_struct *tty)
 }
 
 
-/*****************************************************************************
- * SerialSetTermios
- *	this function is called by the tty driver when it wants to change
- * the termios structure
- *****************************************************************************/
-static void edge_set_termios(struct tty_struct *tty,
-			     struct usb_serial_port *port,
-			     const struct ktermios *old_termios)
-{
-	struct edgeport_port *edge_port = usb_get_serial_port_data(port);
-
-	/* change the port settings to the new ones specified */
-	change_port_settings(tty, edge_port, old_termios);
-}
-
-
 /*****************************************************************************
  * get_lsr_info - get line status register info
  *
@@ -1727,7 +1713,6 @@ static void process_rcvd_status(struct edgeport_serial *edge_serial,
 {
 	struct usb_serial_port *port;
 	struct edgeport_port *edge_port;
-	struct tty_struct *tty;
 	struct device *dev;
 	__u8 code = edge_serial->rxStatusCode;
 	unsigned long flags;
@@ -1787,15 +1772,6 @@ static void process_rcvd_status(struct edgeport_serial *edge_serial,
 		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
-		   in sync with any further termios calls */
-		tty = tty_port_tty_get(&edge_port->port->port);
-		if (tty) {
-			change_port_settings(tty,
-				edge_port, &tty->termios);
-			tty_kref_put(tty);
-		}
-
 		/* we have completed the open */
 		edge_port->openPending = false;
 		wake_up(&edge_port->wait_open);
@@ -2307,18 +2283,12 @@ static int send_cmd_write_uart_register(struct edgeport_port *edge_port,
 }
 
 
-/*****************************************************************************
- * change_port_settings
- *	This routine is called to set the UART on the device to match the
- *	specified new settings.
- *****************************************************************************/
-
-static void change_port_settings(struct tty_struct *tty,
-	struct edgeport_port *edge_port, const struct ktermios *old_termios)
+static void edge_set_termios(struct tty_struct *tty, struct usb_serial_port *port,
+		const struct ktermios *old_termios)
 {
-	struct device *dev = &edge_port->port->dev;
-	struct edgeport_serial *edge_serial =
-			usb_get_serial_data(edge_port->port->serial);
+	struct edgeport_serial *edge_serial = usb_get_serial_data(port->serial);
+	struct edgeport_port *edge_port = usb_get_serial_port_data(port);
+	struct device *dev = &port->dev;
 	int baud;
 	unsigned cflag;
 	__u8 mask = 0xff;
@@ -2329,12 +2299,6 @@ static void change_port_settings(struct tty_struct *tty,
 	__u8 txFlow;
 	int status;
 
-	if (!edge_port->open &&
-	    !edge_port->openPending) {
-		dev_dbg(dev, "%s - port not opened\n", __func__);
-		return;
-	}
-
 	cflag = tty->termios.c_cflag;
 
 	switch (cflag & CSIZE) {
@@ -2470,7 +2434,7 @@ static void change_port_settings(struct tty_struct *tty,
 
 	dev_dbg(dev, "%s - baud rate = %d\n", __func__, baud);
 	status = send_cmd_write_baud_rate(edge_port, baud);
-	if (status == -1) {
+	if (status == -1 && old_termios) {
 		/* Speed change was not possible - put back the old speed */
 		baud = tty_termios_baud_rate(old_termios);
 		tty_encode_baud_rate(tty, baud, baud);
-- 
2.55.0

[PATCH 09/11] USB: serial: io_edgeport: deassert DTR/RTS on close() and B0

[Johan Hovold]

The modem control signals DTR and RTS should be dropped on close() if
HUPCL is set and when requesting B0, but the driver is currently only
asserting the signals on open().

Implement the missing dtr_rts() callback to handle open() and close()
while letting set_termios() handle B0 transitions.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/io_edgeport.c | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
index acaac342f43f..fd983ebd1b6d 100644
--- a/drivers/usb/serial/io_edgeport.c
+++ b/drivers/usb/serial/io_edgeport.c
@@ -1567,6 +1567,23 @@ static int edge_break(struct tty_struct *tty, int break_state)
 }
 
 
+static void edge_dtr_rts(struct usb_serial_port *port, int on)
+{
+	struct edgeport_port *edge_port = usb_get_serial_port_data(port);
+
+	mutex_lock(&edge_port->mutex);
+
+	if (on)
+		edge_port->shadowMCR |= MCR_DTR | MCR_RTS;
+	else
+		edge_port->shadowMCR &= ~(MCR_DTR | MCR_RTS);
+
+	send_cmd_write_uart_register(edge_port, MCR, edge_port->shadowMCR);
+
+	mutex_unlock(&edge_port->mutex);
+}
+
+
 /*****************************************************************************
  * process_rcvd_data
  *	this function handles the data received on the bulk in pipe.
@@ -2415,9 +2432,12 @@ static void edge_set_termios(struct tty_struct *tty, struct usb_serial_port *por
 
 	/* set up the MCR register and send it to the EdgePort */
 	mutex_lock(&edge_port->mutex);
-	edge_port->shadowMCR = MCR_MASTER_IE;
-	if (cflag & CBAUD)
-		edge_port->shadowMCR |= (MCR_DTR | MCR_RTS);
+	if (cflag & CBAUD) {
+		if (old_termios && (old_termios->c_cflag & CBAUD) == B0)
+			edge_port->shadowMCR |= MCR_DTR | MCR_RTS;
+	} else {
+		edge_port->shadowMCR &= ~(MCR_DTR | MCR_RTS);
+	}
 
 	status = send_cmd_write_uart_register(edge_port, MCR,
 						edge_port->shadowMCR);
@@ -2959,6 +2979,7 @@ static struct usb_serial_driver edgeport_2port_device = {
 	.tiocmset		= edge_tiocmset,
 	.tiocmiwait		= usb_serial_generic_tiocmiwait,
 	.get_icount		= usb_serial_generic_get_icount,
+	.dtr_rts		= edge_dtr_rts,
 	.write			= edge_write,
 	.write_room		= edge_write_room,
 	.chars_in_buffer	= edge_chars_in_buffer,
@@ -2993,6 +3014,7 @@ static struct usb_serial_driver edgeport_4port_device = {
 	.tiocmset		= edge_tiocmset,
 	.tiocmiwait		= usb_serial_generic_tiocmiwait,
 	.get_icount		= usb_serial_generic_get_icount,
+	.dtr_rts		= edge_dtr_rts,
 	.write			= edge_write,
 	.write_room		= edge_write_room,
 	.chars_in_buffer	= edge_chars_in_buffer,
@@ -3027,6 +3049,7 @@ static struct usb_serial_driver edgeport_8port_device = {
 	.tiocmset		= edge_tiocmset,
 	.tiocmiwait		= usb_serial_generic_tiocmiwait,
 	.get_icount		= usb_serial_generic_get_icount,
+	.dtr_rts		= edge_dtr_rts,
 	.write			= edge_write,
 	.write_room		= edge_write_room,
 	.chars_in_buffer	= edge_chars_in_buffer,
@@ -3061,6 +3084,7 @@ static struct usb_serial_driver epic_device = {
 	.tiocmset		= edge_tiocmset,
 	.tiocmiwait		= usb_serial_generic_tiocmiwait,
 	.get_icount		= usb_serial_generic_get_icount,
+	.dtr_rts		= edge_dtr_rts,
 	.write			= edge_write,
 	.write_room		= edge_write_room,
 	.chars_in_buffer	= edge_chars_in_buffer,
-- 
2.55.0

[PATCH 10/11] USB: serial: io_edgeport: replace atomic command allocations

[Johan Hovold]

With the line settings no longer being updated from the interrupt
completion handler there is no need for command URBs and buffers to be
allocated atomically.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/io_edgeport.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
index fd983ebd1b6d..fbb8249bb5b7 100644
--- a/drivers/usb/serial/io_edgeport.c
+++ b/drivers/usb/serial/io_edgeport.c
@@ -2079,7 +2079,7 @@ static int send_iosp_ext_cmd(struct edgeport_port *edge_port,
 	int             length = 0;
 	int             status = 0;
 
-	buffer = kmalloc(10, GFP_ATOMIC);
+	buffer = kmalloc(10, GFP_KERNEL);
 	if (!buffer)
 		return -ENOMEM;
 
@@ -2114,7 +2114,7 @@ static int write_cmd_usb(struct edgeport_port *edge_port,
 	usb_serial_debug_data(dev, __func__, length, buffer);
 
 	/* Allocate our next urb */
-	urb = usb_alloc_urb(0, GFP_ATOMIC);
+	urb = usb_alloc_urb(0, GFP_KERNEL);
 	if (!urb)
 		return -ENOMEM;
 
@@ -2128,8 +2128,7 @@ static int write_cmd_usb(struct edgeport_port *edge_port,
 			buffer, length, edge_bulk_out_cmd_callback, edge_port);
 
 	edge_port->commandPending = true;
-	status = usb_submit_urb(urb, GFP_ATOMIC);
-
+	status = usb_submit_urb(urb, GFP_KERNEL);
 	if (status) {
 		/* something went wrong */
 		dev_err(dev, "%s - usb_submit_urb(write command) failed, status = %d\n",
@@ -2186,7 +2185,7 @@ static int send_cmd_write_baud_rate(struct edgeport_port *edge_port,
 	}
 
 	/* Alloc memory for the string of commands. */
-	cmdBuffer =  kmalloc(0x100, GFP_ATOMIC);
+	cmdBuffer = kmalloc(0x100, GFP_KERNEL);
 	if (!cmdBuffer)
 		return -ENOMEM;
 
@@ -2280,7 +2279,7 @@ static int send_cmd_write_uart_register(struct edgeport_port *edge_port,
 	}
 
 	/* Alloc memory for the string of commands. */
-	cmdBuffer = kmalloc(0x10, GFP_ATOMIC);
+	cmdBuffer = kmalloc(0x10, GFP_KERNEL);
 	if (cmdBuffer == NULL)
 		return -ENOMEM;
 
-- 
2.55.0

[PATCH 11/11] USB: serial: io_edgeport: switch to synchronous commands

[Johan Hovold]

With the line settings no longer being updated from the interrupt
completion handler command requests can be made synchronous as is
generally expected.

Note that the five second timeout matches the timeout used by the
original (ifdeffed) synchronous handling (see commmit 421ae6e3db69
("USB: io_edgeport: remove unused defines")).

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/io_edgeport.c | 86 ++++----------------------------
 1 file changed, 9 insertions(+), 77 deletions(-)

diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
index fbb8249bb5b7..3f04898a7cf2 100644
--- a/drivers/usb/serial/io_edgeport.c
+++ b/drivers/usb/serial/io_edgeport.c
@@ -52,6 +52,7 @@
 #define MAX_NAME_LEN		64
 
 #define OPEN_TIMEOUT		(5*HZ)		/* 5 seconds */
+#define EDGE_CMD_TIMEOUT	(5 * HZ)
 
 static const struct usb_device_id edgeport_2port_id_table[] = {
 	{ USB_DEVICE(USB_VENDOR_ID_ION, ION_DEVICE_ID_EDGEPORT_2) },
@@ -181,7 +182,6 @@ struct edgeport_port {
 
 	bool			open;
 	bool			openPending;
-	bool			commandPending;
 	bool			closePending;
 	bool			chaseResponsePending;
 
@@ -262,9 +262,6 @@ static const struct divisor_table_entry divisor_table[] = {
 	{   230400,	1},
 };
 
-/* Number of outstanding Command Write Urbs */
-static atomic_t CmdUrbs = ATOMIC_INIT(0);
-
 
 /* function prototypes */
 
@@ -766,44 +763,6 @@ static void edge_bulk_out_data_callback(struct urb *urb)
 }
 
 
-/*****************************************************************************
- * BulkOutCmdCallback
- *	this is the callback function for when we have finished sending a
- *	command	on the bulk out endpoint.
- *****************************************************************************/
-static void edge_bulk_out_cmd_callback(struct urb *urb)
-{
-	struct edgeport_port *edge_port = urb->context;
-	struct device *dev = &urb->dev->dev;
-	int status = urb->status;
-
-	atomic_dec(&CmdUrbs);
-	dev_dbg(dev, "%s - FREE URB %p (outstanding %d)\n", __func__, urb,
-		atomic_read(&CmdUrbs));
-
-
-	/* clean up the transfer buffer */
-	kfree(urb->transfer_buffer);
-
-	/* Free the command urb */
-	usb_free_urb(urb);
-
-	if (status) {
-		dev_dbg(dev, "%s - nonzero write bulk status received: %d\n",
-			__func__, status);
-		return;
-	}
-
-	/* tell the tty driver that something has changed */
-	if (edge_port->open)
-		tty_port_tty_wakeup(&edge_port->port->port);
-
-	/* we have completed the command */
-	edge_port->commandPending = false;
-	wake_up(&edge_port->wait_command);
-}
-
-
 /*****************************************************************************
  * Driver tty interface functions
  *****************************************************************************/
@@ -2105,49 +2064,22 @@ static int send_iosp_ext_cmd(struct edgeport_port *edge_port,
 static int write_cmd_usb(struct edgeport_port *edge_port,
 					unsigned char *buffer, int length)
 {
-	struct edgeport_serial *edge_serial =
-				usb_get_serial_data(edge_port->port->serial);
+	struct usb_serial *serial = edge_port->port->serial;
+	struct edgeport_serial *edge_serial = usb_get_serial_data(serial);
 	struct device *dev = &edge_port->port->dev;
-	int status = 0;
-	struct urb *urb;
+	int status;
 
 	usb_serial_debug_data(dev, __func__, length, buffer);
 
-	/* Allocate our next urb */
-	urb = usb_alloc_urb(0, GFP_KERNEL);
-	if (!urb)
-		return -ENOMEM;
-
-	atomic_inc(&CmdUrbs);
-	dev_dbg(dev, "%s - ALLOCATE URB %p (outstanding %d)\n",
-		__func__, urb, atomic_read(&CmdUrbs));
-
-	usb_fill_bulk_urb(urb, edge_serial->serial->dev,
-			usb_sndbulkpipe(edge_serial->serial->dev,
-					edge_serial->bulk_out_endpoint),
-			buffer, length, edge_bulk_out_cmd_callback, edge_port);
-
-	edge_port->commandPending = true;
-	status = usb_submit_urb(urb, GFP_KERNEL);
+	status = usb_bulk_msg(serial->dev,
+			usb_sndbulkpipe(serial->dev, edge_serial->bulk_out_endpoint),
+			buffer, length, NULL, EDGE_CMD_TIMEOUT);
 	if (status) {
-		/* something went wrong */
-		dev_err(dev, "%s - usb_submit_urb(write command) failed, status = %d\n",
-			__func__, status);
-		usb_free_urb(urb);
-		atomic_dec(&CmdUrbs);
+		dev_err(dev, "failed to send command: %d\n", status);
 		return status;
 	}
 
-#if 0
-	wait_event(&edge_port->wait_command, !edge_port->commandPending);
-
-	if (edge_port->commandPending) {
-		/* command timed out */
-		dev_dbg(dev, "%s - command timed out\n", __func__);
-		status = -EINVAL;
-	}
-#endif
-	return status;
+	return 0;
 }
 
 
-- 
2.55.0

Re: [PATCH 10/11] USB: serial: io_edgeport: replace atomic command allocations

[sashiko-bot]

> With the line settings no longer being updated from the interrupt
> completion handler there is no need for command URBs and buffers to be
> allocated atomically.
> 
> Signed-off-by: Johan Hovold <johan@kernel.org>

Sashiko has reviewed this patch and found no issues.

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

Re: [PATCH 01/11] USB: serial: io_edgeport: fix NULL-deref on early status

[sashiko-bot]

> The driver submits the interrupt-in URB during probe or on first port
> open, which on completion may submit the bulk-in URB before all ports
> have been opened and their waitqueues have been initialised.
> 
> If a (malicious) device sends a status message for a port that has not
> yet been opened this may lead to a NULL-pointer dereference in the
> bulk-in completion handler when attempting to wakeup blocked tasks.
> 
> Move waitqueue initialisation from open() to port_probe() to address
> this.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: syzbot+763813d65c7e18c1b6d5@syzkaller.appspotmail.com
> Link: https://lore.kernel.org/6a8818d8.ae6ddae5.3da009.0031.GAE@google.com
> Cc: stable@vger.kernel.org
> Signed-off-by: Johan Hovold <johan@kernel.org>

Sashiko has reviewed this patch and found no issues.

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

Re: [PATCH 09/11] USB: serial: io_edgeport: deassert DTR/RTS on close() and B0

[sashiko-bot]

> The modem control signals DTR and RTS should be dropped on close() if
> HUPCL is set and when requesting B0, but the driver is currently only
> asserting the signals on open().
> 
> Implement the missing dtr_rts() callback to handle open() and close()
> while letting set_termios() handle B0 transitions.
> 
> Signed-off-by: Johan Hovold <johan@kernel.org>

Sashiko has reviewed this patch and found no issues.

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

Re: [PATCH 11/11] USB: serial: io_edgeport: switch to synchronous commands

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] usb: serial: io_edgeport: command buffer leak in write_cmd_usb() callers
- [Medium] usb: serial: io_edgeport: jiffies timeout passed to usb_bulk_msg()
--

commit 76948cdcd262d6f36f2d2e2ca6b395a61167a6ea
Author: Johan Hovold <johan@kernel.org>

USB: serial: io_edgeport: switch to synchronous commands

This commit converts Edgeport command writes in write_cmd_usb() from
asynchronous URB submissions to synchronous usb_bulk_msg() calls and
removes the edge_bulk_out_cmd_callback() completion handler.

> diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
> index fbb8249bb5b74..3f04898a7cf2d 100644
> --- a/drivers/usb/serial/io_edgeport.c
> +++ b/drivers/usb/serial/io_edgeport.c
> @@ -52,6 +52,7 @@
>  #define MAX_NAME_LEN		64
>  
>  #define OPEN_TIMEOUT		(5*HZ)		/* 5 seconds */
> +#define EDGE_CMD_TIMEOUT	(5 * HZ)

[Severity: Medium]
Should this timeout be defined in milliseconds (5000) rather than jiffies?

write_cmd_usb() passes this value to usb_bulk_msg(), which forwards it to
usb_start_wait_urb() in drivers/usb/core/message.c:

	expire = (timeout > 0) ? msecs_to_jiffies(timeout) : MAX_SCHEDULE_TIMEOUT;

Because usb_bulk_msg() expects milliseconds, passing (5 * HZ) results in a
500 ms timeout when HZ is 100 or 1250 ms when HZ is 250 instead of the
intended 5 seconds.

[ ... ]

> @@ -766,44 +763,6 @@ static void edge_bulk_out_data_callback(struct urb *urb)
>  }
>  
>  
> -/*****************************************************************************
> - * BulkOutCmdCallback
> - *	this is the callback function for when we have finished sending a
> - *	command	on the bulk out endpoint.
> - *****************************************************************************/
> -static void edge_bulk_out_cmd_callback(struct urb *urb)
> -{
> -	struct edgeport_port *edge_port = urb->context;
> -	struct device *dev = &urb->dev->dev;
> -	int status = urb->status;
> -
> -	atomic_dec(&CmdUrbs);
> -	dev_dbg(dev, "%s - FREE URB %p (outstanding %d)\n", __func__, urb,
> -		atomic_read(&CmdUrbs));
> -
> -
> -	/* clean up the transfer buffer */
> -	kfree(urb->transfer_buffer);
> -
> -	/* Free the command urb */
> -	usb_free_urb(urb);
> -
> -	if (status) {
> -		dev_dbg(dev, "%s - nonzero write bulk status received: %d\n",
> -			__func__, status);
> -		return;
> -	}
> -
> -	/* tell the tty driver that something has changed */
> -	if (edge_port->open)
> -		tty_port_tty_wakeup(&edge_port->port->port);
> -
> -	/* we have completed the command */
> -	edge_port->commandPending = false;
> -	wake_up(&edge_port->wait_command);
> -}
> -
> -
>  /*****************************************************************************
>   * Driver tty interface functions
>   *****************************************************************************/
> @@ -2105,49 +2064,22 @@ static int send_iosp_ext_cmd(struct edgeport_port *edge_port,
>  static int write_cmd_usb(struct edgeport_port *edge_port,
>  					unsigned char *buffer, int length)
>  {
> -	struct edgeport_serial *edge_serial =
> -				usb_get_serial_data(edge_port->port->serial);
> +	struct usb_serial *serial = edge_port->port->serial;
> +	struct edgeport_serial *edge_serial = usb_get_serial_data(serial);
>  	struct device *dev = &edge_port->port->dev;
> -	int status = 0;
> -	struct urb *urb;
> +	int status;
>  
>  	usb_serial_debug_data(dev, __func__, length, buffer);
>  
> -	/* Allocate our next urb */
> -	urb = usb_alloc_urb(0, GFP_KERNEL);
> -	if (!urb)
> -		return -ENOMEM;
> -
> -	atomic_inc(&CmdUrbs);
> -	dev_dbg(dev, "%s - ALLOCATE URB %p (outstanding %d)\n",
> -		__func__, urb, atomic_read(&CmdUrbs));
> -
> -	usb_fill_bulk_urb(urb, edge_serial->serial->dev,
> -			usb_sndbulkpipe(edge_serial->serial->dev,
> -					edge_serial->bulk_out_endpoint),
> -			buffer, length, edge_bulk_out_cmd_callback, edge_port);
> -
> -	edge_port->commandPending = true;
> -	status = usb_submit_urb(urb, GFP_KERNEL);
> +	status = usb_bulk_msg(serial->dev,
> +			usb_sndbulkpipe(serial->dev, edge_serial->bulk_out_endpoint),
> +			buffer, length, NULL, EDGE_CMD_TIMEOUT);
>  	if (status) {
> -		/* something went wrong */
> -		dev_err(dev, "%s - usb_submit_urb(write command) failed, status = %d\n",
> -			__func__, status);
> -		usb_free_urb(urb);
> -		atomic_dec(&CmdUrbs);
> +		dev_err(dev, "failed to send command: %d\n", status);
>  		return status;
>  	}
>  
> -#if 0
> -	wait_event(&edge_port->wait_command, !edge_port->commandPending);
> -
> -	if (edge_port->commandPending) {
> -		/* command timed out */
> -		dev_dbg(dev, "%s - command timed out\n", __func__);
> -		status = -EINVAL;
> -	}
> -#endif
> -	return status;
> +	return 0;
>  }

[Severity: High]
Does this leak the kmalloc()-allocated command buffer on every successful
command submission?

Previously, edge_bulk_out_cmd_callback() freed urb->transfer_buffer when the
URB completed. The three callers of write_cmd_usb() in
drivers/usb/serial/io_edgeport.c — send_iosp_ext_cmd(),
send_cmd_write_baud_rate(), and send_cmd_write_uart_register() — only free
their buffer (buffer or cmdBuffer) when write_cmd_usb() returns an error:

drivers/usb/serial/io_edgeport.c:send_iosp_ext_cmd() {
	buffer = kmalloc(10, GFP_KERNEL);
	if (!buffer)
		return -ENOMEM;
	...
	status = write_cmd_usb(edge_port, buffer, length);
	if (status) {
		/* something bad happened, let's free up the memory */
		kfree(buffer);
	}

	return status;
}

send_cmd_write_baud_rate() and send_cmd_write_uart_register() follow the
same pattern with cmdBuffer. Since usb_bulk_msg() does not free the caller's
transfer buffer, nothing frees buffer or cmdBuffer when write_cmd_usb()
returns 0.

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

Re: [PATCH 06/11] USB: serial: io_edgeport: drop redundant sanity checks

[sashiko-bot]

> The serial and port driver data is allocated at probe() before
> registering the ports and will never be NULL when open(), close(),
> write(), throttle() and unthrottle() are called.
> 
> Drop the redundant driver data sanity checks.
> 
> Signed-off-by: Johan Hovold <johan@kernel.org>

Sashiko has reviewed this patch and found no issues.

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

Re: [PATCH 08/11] USB: serial: io_edgeport: move initial termios update to open()

[sashiko-bot]

> Explicitly update the terminal settings on open() instead of doing so
> from the interrupt completion handler when receiving an open command
> response, which in theory can be received after a failed open attempt.
> 
> Note that this will also allow for switching to synchronous commands.
> 
> Signed-off-by: Johan Hovold <johan@kernel.org>

Sashiko has reviewed this patch and found no issues.

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

Re: [PATCH 05/11] USB: serial: io_edgeport: serialise MCR accesses

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: serial: io_edgeport: change_port_settings() locks mutex in atomic context
--

commit 12f657a4474c4b2a9d60008b5bffca174179c50e
Author: Johan Hovold <johan@kernel.org>

USB: serial: io_edgeport: serialise MCR accesses

This commit adds a mutex to struct edgeport_port to serialise updates and
reads of shadowMCR across port operations.

> diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
> index 38f58ca115ebd..0b50c6b99b0a8 100644
> --- a/drivers/usb/serial/io_edgeport.c
> +++ b/drivers/usb/serial/io_edgeport.c
[ ... ]
> @@ -2477,12 +2489,14 @@ static void change_port_settings(struct tty_struct *tty,
>  		return;
>  
>  	/* set up the MCR register and send it to the EdgePort */
> +	mutex_lock(&edge_port->mutex);
>  	edge_port->shadowMCR = MCR_MASTER_IE;
>  	if (cflag & CBAUD)
>  		edge_port->shadowMCR |= (MCR_DTR | MCR_RTS);
>  
>  	status = send_cmd_write_uart_register(edge_port, MCR,
>  						edge_port->shadowMCR);
> +	mutex_unlock(&edge_port->mutex);

[Severity: High]
Can this sleep in atomic context when a port is opened?

When edge_open() sends an open command to the device and waits on
edge_port->wait_open, the open response is handled in the bulk-in urb
completion callback while holding edge_serial->es_lock with interrupts
disabled:

drivers/usb/serial/io_edgeport.c:
edge_bulk_in_callback()
    spin_lock_irqsave(&edge_serial->es_lock, flags);
    ...
    process_rcvd_data(edge_serial, data, urb->actual_length);
        -> process_rcvd_status()
            if (code == IOSP_STATUS_OPEN_RSP) {
                ...
                tty = tty_port_tty_get(&edge_port->port->port);
                if (tty) {
                    change_port_settings(tty,
                        edge_port, &tty->termios);
                    tty_kref_put(tty);
                }

Calling mutex_lock(&edge_port->mutex) inside change_port_settings() acquires
a sleepable mutex under spin_lock_irqsave() in hardirq or softirq context.

A subsequent commit in this series moves the initial termios update from
process_rcvd_status() to edge_open(), but having this patch ordered before
that change leaves an intermediate bisection window where opening a port
triggers a sleeping-in-atomic bug, and could also cause a regression when
this commit is backported to stable trees without the follow-up refactoring.

Should the change moving the initial termios update into edge_open() come
before this patch and be included as a prerequisite for stable?

>  	if (status != 0)
>  		return;
>  
[ ... ]

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

Re: [PATCH 04/11] USB: serial: io_edgeport: fix potential NULL-deref at open

[sashiko-bot]

> Do not mark the port as open until after the write FIFO has been
> allocated and initialised to prevent a racing interrupt completion from
> attempting to send stale data and dereferencing the FIFO buffer pointer.
> 
> Note that the open flag defaults to false and is cleared on close() so
> there is no need to reinitialise it on every open().
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Signed-off-by: Johan Hovold <johan@kernel.org>

Sashiko has reviewed this patch and found no issues.

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

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

[sashiko-bot]

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

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

[sashiko-bot]

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

Re: [PATCH 07/11] USB: serial: io_edgeport: drop redundant open sanity checks

[sashiko-bot]

> The throttle(), unthrottle() and set_termios() callbacks will not be
> called unless the port is open so drop the redundant sanity checks.
> 
> Signed-off-by: Johan Hovold <johan@kernel.org>

Sashiko has reviewed this patch and found no issues.

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

Re: [PATCH 05/11] USB: serial: io_edgeport: serialise MCR accesses

[Johan Hovold]

On Wed, Oct 21, 2026 at 02:26:55PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] usb: serial: io_edgeport: change_port_settings() locks mutex in atomic context

> A subsequent commit in this series moves the initial termios update from
> process_rcvd_status() to edge_open(), but having this patch ordered
> before that change leaves an intermediate bisection window where opening
> a port triggers a sleeping-in-atomic bug, and could also cause a
> regression when this commit is backported to stable trees without the
> follow-up refactoring.

Indeed, and that was why this one was ordered after the termios change
until today when I picked up this series that got put on the backburner
months ago...

Will fix in v2.

Johan

Re: [PATCH 11/11] USB: serial: io_edgeport: switch to synchronous commands

[Johan Hovold]

On Wed, Oct 21, 2026 at 02:23:15PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] usb: serial: io_edgeport: command buffer leak in write_cmd_usb() callers
> - [Medium] usb: serial: io_edgeport: jiffies timeout passed to usb_bulk_msg()

This one clearly wasn't ready...

Johan