[PATCH v2] 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
device.  A hub has a single interface, so hub-wide and port requests
are covered by the same check.  Tools that manipulate ports of a hub
under control of the hub driver now fail with -EBUSY unless they first
disconnect the hub driver.

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

Hi Alan,

v2.  v1 was wrong: I required USB_RECIP_DEVICE, but port requests are
sent as USB_RT_PORT, whose recipient is USB_RECIP_OTHER, and
check_ctrlrecip() has no case for that recipient at all.  v1 would not
have stopped the SET_FEATURE(PORT_SUSPEND) request this started from.
The test is now on the direction only, which is what you asked for, and
since a hub has a single interface, hub-wide and port requests both end
up checking interface 0.

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.  Once a claim
is required, the request either fails with -EBUSY while the hub driver
is bound, or the claim itself is what unbinds the driver and disconnects
the children, after which the port has no slot left to suspend.

One thing to decide before this goes anywhere: tools that power-cycle
ports by sending SET_FEATURE(PORT_POWER) to a hub they have not claimed
(uhubctl is the usual one) will now get -EBUSY from checkintf(), because
the hub driver owns interface 0.  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 | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 486ced74405e..943f0f7f6b64 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -891,6 +891,19 @@ 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 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 and hub-wide ones USB_RECIP_DEVICE,
+	 * and a hub has a single interface, so check interface 0 for both.
+	 */
+	if (ps->dev->descriptor.bDeviceClass == USB_CLASS_HUB &&
+	    (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