[PATCH v5] usb: usbfs: Require claiming a hub interface for hub class requests

[YuanZhe Liu]

check_ctrlrecip() requires the matching interface to be claimed for
requests addressed to an interface or an endpoint, but requests
addressed to the device or to a port are passed through without any
check.  For a hub that means userspace can send hub class requests
through usbfs without claiming the hub interface, while the hub driver
owns the ports.  Port requests use USB_RT_PORT, whose recipient is
USB_RECIP_OTHER, and SetPortFeature(PORT_SUSPEND) is one of them.

That is more than a policy question.  The request goes to the HCD, and
it can run concurrently with userspace releasing the hub interface
through USBDEVFS_DISCONNECT_CLAIM, which unbinds the hub driver and
disconnects the devices behind it.  xhci_hcd tracks the device behind
each port and issues stop endpoint commands for it in its port suspend
path, and it ended up reading a freed xhci_virt_device.

Require interface 0 to be claimed for every non-GET request to a hub.
The hub driver matches devices by device class or by interface class,
but a hub has a single interface either way, and that is the one the
driver holds.

Userspace can no longer change port settings while the hub driver is
bound.  USBDEVFS_CLAIM_PORT hands over the device behind a port, not the
port itself, so a request that pokes port state is not safe for a
claimed port either.  Programs that used to do this have to unbind the
hub driver first.

Signed-off-by: YuanZhe Liu <25031212351@xxxxxxxxxxxxxxxxx>
---
Hi Alan,

v5 takes both points.  is_hub() is tested first, and the comment now says
that IN requests are safe because they do not affect the hub, while OUT
requests need the interface claimed, instead of talking about recipients.

I will hold the URB synchronization patch for usb_hub_release_port() until
this one is settled.

YuanZhe

 drivers/usb/core/devio.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index e191934623c7..6d69a993c0f1 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -878,6 +878,23 @@ static int findintfep(struct usb_device *dev, unsigned int ep)
 	return -ENOENT;
 }
 
+/*
+ * Return true if @dev is a hub.  The hub driver matches devices by device
+ * class or by interface class, and a hub has a single interface either way.
+ */
+static bool is_hub(struct usb_device *dev)
+{
+	struct usb_host_config *config = dev->actconfig;
+
+	if (dev->descriptor.bDeviceClass == USB_CLASS_HUB)
+		return true;
+	if (!config || config->desc.bNumInterfaces < 1)
+		return false;
+	return config->interface[0] && config->interface[0]->cur_altsetting &&
+	       config->interface[0]->cur_altsetting->desc.bInterfaceClass ==
+			USB_CLASS_HUB;
+}
+
 static int check_ctrlrecip(struct usb_dev_state *ps, unsigned int requesttype,
 			   unsigned int request, unsigned int index)
 {
@@ -891,6 +908,18 @@ static int check_ctrlrecip(struct usb_dev_state *ps, unsigned int requesttype,
 	if (USB_TYPE_VENDOR == (USB_TYPE_MASK & requesttype))
 		return 0;
 
+	/*
+	 * A hub driver owns the ports and the devices behind them.  Letting
+	 * userspace change hub or port state behind its back, for example
+	 * with a SET_FEATURE(PORT_SUSPEND) request while the hub interface is
+	 * being released, is not safe: the request is passed down to the HCD,
+	 * which may walk per-device state that is being torn down.  IN
+	 * requests are safe, they do not affect the hub, but OUT requests are
+	 * only allowed when the caller has claimed the hub interface.
+	 */
+	if (is_hub(ps->dev) && (requesttype & USB_DIR_IN) == 0)
+		return checkintf(ps, 0);
+
 	/*
 	 * check for the special corner case 'get_device_id' in the printer
 	 * class specification, which we always want to allow as it is used
-- 
2.34.1