USB: serial: fix port tear down use-after-free

When addressing a port probe ordering issue in digi_acceleport recently I realised that we have a corresponding general problem during disconnect, but I didn't have time to address it straight away.

Now syzbot has managed to trigger one of its symptoms and people have already started submitting incomplete band-aids so here is a proper fix.

When testing the fix I also noticed that we have an ordering issue when deregistering drivers that can result in similar use-after-free.

Johan

Johan Hovold (2):
USB: serial: fix port tear down use-after-free
USB: serial: fix driver deregistration order

 drivers/usb/serial/usb-serial.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

2.54.0

Some drivers for multiport devices access port driver data from completion handlers of shared URBs submitted at attach() or first open() and stopped at last close() or disconnect(), respectively.

A simple NULL check before accessing the driver data makes sure that a port state container has at least been allocated, but a completion handler can still race with port tear down.

Reorder the disconnect handling so that ports are not deregistered (and their driver data freed) until after all ports have been hung up and the driver disconnect() callback has run so that all I/O has been stopped.

---
 drivers/usb/serial/usb-serial.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

index 17edc057a311..a4fbc849c0fa 100644
@@ -1191,12 +1191,17 @@ static void usb_serial_disconnect(struct usb_interface *interface)
 		usb_serial_port_poison_urbs(port);
 		wake_up_interruptible(&port->port.delta_msr_wait);
 		cancel_work_sync(&port->work);
-		if (device_is_registered(&port->dev))
-			device_del(&port->dev);
 	}
+
 	if (serial->type->disconnect)
 		serial->type->disconnect(serial);
 
+	for (i = 0; i < serial->num_ports; ++i) {
+		port = serial->port[i];
+		if (device_is_registered(&port->dev))
+			device_del(&port->dev);
+	}
+
 	release_sibling(serial, interface);
 
 	/* let the last holder of this object cause it to be cleaned up */

2.54.0

USB serial driver modules register one driver for the USB bus and one or more drivers for the ports on the USB serial bus.

When unloading a driver module, the USB driver must be deregistered before the USB serial bus drivers so that I/O is stopped before unbinding the ports to avoid use-after-free in completion handlers accessing port data.

Note that the order does not matter currently in the registration error path as the USB driver is not bound until after the USB serial drivers have been registered.

---
 drivers/usb/serial/usb-serial.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

index a4fbc849c0fa..08a9b6f8cf7d 100644
@@ -1522,9 +1522,9 @@ int __usb_serial_register_drivers(struct usb_serial_driver *const serial_drivers
 	return 0;
 
 err_deregister_drivers:
+	usb_deregister(udriver);
 	while (sd-- > serial_drivers)
 		usb_serial_deregister(*sd);
-	usb_deregister(udriver);
 err_free_driver:
 	kfree(udriver);
 	return rc;

@@ -1543,9 +1543,15 @@ void usb_serial_deregister_drivers(struct usb_serial_driver *const serial_driver
 {
 	struct usb_driver *udriver = (*serial_drivers)->usb_driver;
 
+	/*
+	 * The USB driver must be deregistered before the USB serial drivers
+	 * so that I/O is stopped before unbinding the ports.
+	 */
+	usb_deregister(udriver);
+
 	for (; *serial_drivers; ++serial_drivers)
 		usb_serial_deregister(*serial_drivers);
-	usb_deregister(udriver);
+
 	kfree(udriver);
 }
 EXPORT_SYMBOL_GPL(usb_serial_deregister_drivers);

2.54.0

But __usb_serial_register_drivers() does usb_register(udriver) _before_ calling usb_serial_register() for the serial_drivers. Not _after_, as claimed here.

And if the order in the registration error path does not matter, why does the patch change it?

Or did you mean something else?

Also, what about the failure mode described in 765e0ba62613? Not to mention that it seems odd to register a parent device driver after a child device driver, rather than before. Or to unregister them in the reverse order.

If the point is to avoid I/O operations completing after the device structure has been deallocated, why not rely on the serial-bus drivers to stop all their I/O when they are unbound?

If these questions don't make sense, remember that I haven't done serious work on this subsystem for 15 years or so and blame it on that. :-)

Alan Stern

It registers the driver, but it doesn't set up the id table and therefore will not bind to any interface until after the serial drivers have also been registered (as an optimisation).

For symmetry with deregistration and for the unlikely event that there is ever a later error path added (or if the optimisation mentioned above is removed).

There is generally no issue registering a driver for a child device after a driver for a parent so that everything is in place when the parent driver registers the child device.

In fact, that is sort of what is done today by not setting up the id_table until after the serial drivers have been registered even if things are complicated by how dynamic id support is implemented for USB serial.

As I'm not changing the registration order I sort of skimmed over the dynamic id issue addressed by commit 765e0ba62613 ("usb-serial: new API for driver registration"). But I guess need to revisit that to make sure I don't reintroduce a race during deregistration.

The problem is that there are two drivers involved for the USB interface and USB serial port, respectively, both of which can do I/O and where the parent USB driver may access the driver data of the child ports.

Tearing things down properly is a bit involved as you know (and as can be seen from the first patch in the series). Everything is therefore managed from the parent driver disconnect callback and individual ports are, for example, no longer allowed to be unbound independently (cf. commit fdb838efa31e ("USB: serial: suppress driver bind attributes")).

The only remaining corner case should be driver deregistration.

Heh. They make perfect sense. Thanks for taking a look.

Johan

Nice catch: