Re: [PATCH v2 1/2] USB: serial: fix break_ctl() NULL-pointer dereference

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch's attempt to fix it is flawed. The newly added lockless state check of USB_STATE_NOTATTACHED in `serial_break` leaves a Time-of-Check to Time-of-Use (TOCTOU) race window, failing to completely prevent Use-After-Free or NULL-pointer dereference vulnerabilities upon concurrent device disconnect.
- [High] This problem wasn't introduced by this patch, but while the patch aims to prevent calling into the driver after disconnect, it completely omits other TTY operations in the generic usb-serial driver, leaving them vulnerable to the same Use-After-Free crashes.
--

commit 35b2c0cc5ee711aab91cfbffe24f9565620f8f65
Author: Adriano Cordova <adrianox@gmail.com>
USB: serial: fix break_ctl() NULL-pointer dereference

This commit attempts to fix a NULL-pointer dereference by checking if the
USB device is detached before calling into the sub-driver's break_ctl
callback.

Link: https://syzkaller.appspot.com/bug?extid=473d7477c523b41d4046

> diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
> --- a/drivers/usb/serial/usb-serial.c
> +++ b/drivers/usb/serial/usb-serial.c
> @@ -532,6 +532,9 @@ static int serial_break(struct tty_struct *tty, int break_state)
>  {
>  	struct usb_serial_port *port = tty->driver_data;
>  
> +	if (port->serial->dev->state == USB_STATE_NOTATTACHED)
> +		return -ENODEV;
> +

[Severity: High]
This isn't a bug introduced by this patch, but does this lockless check
leave a Time-of-Check to Time-of-Use (TOCTOU) race window?

If the thread executing serial_break() in drivers/usb/serial/usb-serial.c
evaluates USB_STATE_NOTATTACHED to false and gets preempted, could a
concurrent usb_disconnect() finish port_remove() and free driver-private
data before the thread resumes?

>  	dev_dbg(&port->dev, "%s\n", __func__);
>  
>  	if (port->serial->type->break_ctl)
>  		return port->serial->type->break_ctl(tty, break_state);

[Severity: High]
This is a pre-existing issue, but does this still execute the sub-driver's
break_ctl() function on freed memory or a NULL pointer if the race condition
described above triggers during a concurrent TIOCSBRK or TIOCCBRK ioctl?

Additionally, this is a pre-existing issue, but while this patch aims to
prevent calling into the driver after disconnect in serial_break(), what
protects other TTY operations in the generic usb-serial driver from the
same Use-After-Free crashes?

For example, serial_tiocmget() unconditionally calls into sub-drivers
without checking for disconnect:

drivers/usb/serial/usb-serial.c:serial_tiocmget() {
    ...
	if (port->serial->type->tiocmget)
		return port->serial->type->tiocmget(tty);
    ...
}

Similar unprotected calls appear to happen in serial_ioctl(),
serial_tiocmset(), serial_throttle(), and serial_unthrottle(). Can these
also be triggered by an unprivileged user spamming concurrent TTY ioctls
(like TIOCMGET) while the physical device is unplugged?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903042429.130320-1-adrianox@gmail.com?part=1