[PATCH] 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 itself are passed through without any check.
For a hub that means userspace can send hub class requests, including
SET_FEATURE(PORT_SUSPEND), through usbfs without claiming the hub
interface, and while the hub driver owns the ports.

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 to be claimed for non-GET device requests to a
hub, the same way interface requests are already handled.  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,

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.

Here is the patch.  Interface and endpoint recipients already go
through checkintf() further down, so I added the same requirement for
device recipients.

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 | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 486ced74405e..cc6669743017 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -891,6 +891,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 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.
+	 */
+	if (ps->dev->descriptor.bDeviceClass == USB_CLASS_HUB &&
+	    (requesttype & USB_DIR_IN) == 0 &&
+	    (requesttype & USB_RECIP_MASK) == USB_RECIP_DEVICE)
+		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