[PATCH v3] 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 the interface the hub driver uses to be claimed for every
non-GET request to a hub.  Hubs match the driver by device class or by
interface class, so look up the interface instead of testing
bDeviceClass.  A port claimed with USBDEVFS_CLAIM_PORT belongs to the
caller and the hub driver does not connect devices to it, so requests
for such a port stay allowed; that is what the ioctl is for.

Tools that manipulate ports of a hub under control of the hub driver,
without claiming the port or the interface, now fail with -EBUSY.

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

v3.  v2 had two problems, both pointed out after it went out:

 - a hub in a composite device matches the hub driver by interface
   class, not by device class, so testing bDeviceClass alone missed it.
   The check now looks up the interface the hub driver uses, whichever
   way the device matched.
 - USBDEVFS_CLAIM_PORT hands userspace a single port while the hub
   driver stays bound; that is what the ioctl is for, and v2 made it
   unusable by requiring the whole interface.  Requests for a port owned
   by the caller are allowed now.  That needs a small helper in hub.c to
   test ownership, which is why this touches three files.

On whether this closes the window completely: it does not serialize the
transfer, and it is not meant to.  check_ctrlrecip() runs under the
device lock and do_proc_control() drops that lock before
usbfs_start_wait_urb(), so an in-flight request is not ordered against a
claim being released.  What the check gives us is that whenever a
request is allowed, the hub driver is bound, so the port has no device
for xhci_free_dev() to free.  The memory safety itself comes from the
xhci hunk in the other thread.

To your question: yes, that is exactly the sequence - one thread sends
SET_FEATURE(PORT_SUSPEND) to the root hub without claiming interface 0,
and another one releases the hub interface through usbfs.

With the port owner exception above, tools like uhubctl that send
SET_FEATURE(PORT_POWER) to a hub without claiming either the port or the
interface will still get -EBUSY.  Do you want an exception while
privileges have not been dropped, or is losing that use case the point?

Whether this should be marked for stable is your call.  The missing
check goes back to the initial git import (1da177e4c3f4) and the ioctl
that can race with it arrived with 0837e7e5270b, so I did not add a
Fixes: tag; I also left out Cc: stable because this changes behaviour
for userspace.  The stable candidate is the xhci_hcd fix in the other
thread.  If you would rather see this backported, tell me which commit
you want named and I will resend with the tags.

YuanZhe

 drivers/usb/core/devio.c | 43 ++++++++++++++++++++++++++++++++++++++++
 drivers/usb/core/hub.c   | 14 +++++++++++++
 drivers/usb/core/usb.h   |  2 ++
 3 files changed, 59 insertions(+)

diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 486ced74405e..7dc179108a31 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -878,10 +878,36 @@ static int findintfep(struct usb_device *dev, unsigned int ep)
 	return -ENOENT;
 }
 
+/*
+ * Return the interface number through which the hub driver drives this
+ * device, or -1 if it is not a hub.  Hubs match the driver by device class
+ * or by interface class, and a hub embedded in a composite device uses the
+ * latter.
+ */
+static int hub_interface_number(struct usb_device *dev)
+{
+	struct usb_host_config *config = dev->actconfig;
+	int i;
+
+	if (dev->descriptor.bDeviceClass == USB_CLASS_HUB)
+		return 0;
+	if (!config)
+		return -1;
+	for (i = 0; i < config->desc.bNumInterfaces; i++) {
+		struct usb_interface *intf = config->interface[i];
+
+		if (intf && intf->cur_altsetting &&
+		    intf->cur_altsetting->desc.bInterfaceClass == USB_CLASS_HUB)
+			return i;
+	}
+	return -1;
+}
+
 static int check_ctrlrecip(struct usb_dev_state *ps, unsigned int requesttype,
 			   unsigned int request, unsigned int index)
 {
 	int ret = 0;
+	int hub_ifnum;
 	struct usb_host_interface *alt_setting;
 
 	if (ps->dev->state != USB_STATE_UNAUTHENTICATED
@@ -891,6 +917,23 @@ 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 at the same
+	 * time.  Port requests use USB_RECIP_OTHER, hub-wide ones use
+	 * USB_RECIP_DEVICE.  A port claimed with USBDEVFS_CLAIM_PORT belongs to
+	 * the caller though, and the hub driver leaves devices on such a port
+	 * alone, so requests for it are allowed.
+	 */
+	hub_ifnum = hub_interface_number(ps->dev);
+	if (hub_ifnum >= 0 && (requesttype & USB_DIR_IN) == 0 &&
+	    !((requesttype & USB_RECIP_MASK) == USB_RECIP_OTHER &&
+	      usb_hub_port_is_owned(ps->dev, index & 0xff, ps)))
+		return checkintf(ps, hub_ifnum);
+
 	/*
 	 * check for the special corner case 'get_device_id' in the printer
 	 * class specification, which we always want to allow as it is used
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index b119f5b6065c..16dbcb59dae5 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2107,6 +2107,20 @@ int usb_hub_release_port(struct usb_device *hdev, unsigned port1,
 }
 EXPORT_SYMBOL_GPL(usb_hub_release_port);
 
+/*
+ * Returns true if @owner has claimed port @port1 through usbfs.  The caller
+ * must hold the device lock.
+ */
+bool usb_hub_port_is_owned(struct usb_device *hdev, unsigned int port1,
+			       struct usb_dev_state *owner)
+{
+	struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
+
+	if (!hub || port1 == 0 || port1 > hdev->maxchild)
+		return false;
+	return hub->ports[port1 - 1]->port_owner == owner;
+}
+
 void usb_hub_release_all_ports(struct usb_device *hdev, struct usb_dev_state *owner)
 {
 	struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index a9b37aeb515b..012c110ac4e9 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -82,6 +82,8 @@ extern void usb_unbind_and_rebind_marked_interfaces(struct usb_device *udev);
 extern void usb_hub_release_all_ports(struct usb_device *hdev,
 		struct usb_dev_state *owner);
 extern bool usb_device_is_owned(struct usb_device *udev);
+extern bool usb_hub_port_is_owned(struct usb_device *hdev, unsigned int port1,
+				     struct usb_dev_state *owner);
 
 extern int  usb_hub_init(void);
 extern void usb_hub_cleanup(void);
-- 
2.34.1