Re: [PATCH v2 19/19] USB: Authorize devices and interfaces through device trust

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential
issues to consider:
- [High] usb: core: usb_resume_device() fails with -ENOTCONN on
  trust-disabled devices
- [High] usb: core: missing usb_notify_remove_device() on unbound
  disconnect
- [Medium] usb: core: probing deauthorized device or interface
  prematurely commits trust
- [Medium] usb: core: usb_driver_claim_interface() rejects disabled
  sibling before driver rule
- [Medium] usb: core: usb_authorize_device() drops device lock and races
  with deauthorize
- [Medium] usb: core: usb_device_authorized() true before driver
  admission
- [Medium] usb: core: usb_set_configuration() drops parent adversarial
  policy
- [Medium] usb: core: race in usb_authorize_interface() with
  deauthorization
- [Low] usb: core: missing device_trust_init() in usb_alloc_dev()
--

commit b35772e1cb354ec9df9404c1711878235a7ba8c8
Author: Leon Romanovsky <leon@kernel.org>

USB: Authorize devices and interfaces through device trust

This commit integrates USB device and interface authorization with device
trust by mapping the default authorization policies to
DEVICE_TRUST_POLICY_DISABLED and requesting DEVICE_TRUST_POLICY_FULL when
authorizing via sysfs.

> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index 04b77691fbd7d..2ebd05d3855f4 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -334,10 +334,10 @@ static int usb_probe_interface(struct device *dev)
>  	if (usb_device_is_owned(udev))
>  		return error;
>  
> -	if (udev->authorized == 0) {
> +	if (!usb_device_authorized(udev)) {
>  		dev_info(&intf->dev, "Device is not authorized for usage\n");
>  		return error;
> -	} else if (intf->authorized == 0) {
> +	} else if (!usb_interface_authorized(intf)) {
>  		dev_info(&intf->dev, "Interface %d is not authorized for usage\n",
>  				intf->altsetting->desc.bInterfaceNumber);
>  		return error;

[Severity: Medium]
Does rejecting a deauthorized device or interface inside
usb_probe_interface() (or usb_generic_driver_probe()) happen too late to
avoid committing its trust level?

When an interface starts with interface_authorized_default enabled
(trust_state == DEVICE_TRUST_SELECTABLE) and is deauthorized before any
driver binds, intf->authorized is cleared to 0 while intf->dev.trust_policy
stays DEVICE_TRUST_POLICY_DEFAULT.  Because usb_device_match() does not check
authorization, a subsequent driver load or reprobe enters really_probe() and
runs device_trust_prepare() before usb_probe_interface() is called:

drivers/base/dd.c:really_probe() {
    ...
re_probe:
	ret = device_trust_prepare(dev, drv);
	if (ret) {
    ...
}

device_trust_prepare() permanently sets dev->p->trust_state to
DEVICE_TRUST_COMMITTED before usb_probe_interface() returns -ENODEV, and
device_trust_clear() in the probe failure path does not revert trust_state.
User space can then no longer change trust_policy on the deauthorized
interface and gets -EBUSY.

> @@ -560,13 +560,9 @@ int usb_driver_claim_interface(struct usb_driver *driver,
>  		return -EBUSY;
>  
>  	/* reject claim if interface is not authorized */
> -	if (!iface->authorized)
> +	if (!usb_interface_authorized(iface))
>  		return -ENODEV;
>  
> -	if (!device_is_registered(dev) &&
> -	    dev->trust_policy == DEVICE_TRUST_POLICY_DISABLED)
> -		return -EPERM;
> -
>  	dev->driver = &driver->driver;
>  	usb_set_intfdata(iface, data);
>  	iface->needs_binding = 0;

[Severity: Medium]
Does checking usb_interface_authorized(iface) here break claiming sibling
interfaces when interfaces start disabled by default and a driver is
authorized via a device_trust.driver= boot rule?

When interface_authorized_default is 0 or device_trust.default=disabled is
set, each interface starts with iface->dev.trust_policy set to
DEVICE_TRUST_POLICY_DISABLED.  Driver rules from device_trust.driver= are
only applied when device_trust_prepare() runs inside device_bind_driver() or
device_add() later in usb_driver_claim_interface().  Testing
usb_interface_authorized(iface) up front sees DEVICE_TRUST_POLICY_DISABLED on
the sibling interface and returns -ENODEV before device_trust_prepare() can
evaluate the driver rule.

> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 199d3f535a891..086b8db36f51a 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c

[ ... ]

> @@ -2786,7 +2786,22 @@ int usb_authorize_device(struct usb_device *usb_dev)
>  {
>  	int result = 0, c;
>  
> +	/* Authorizing a device that its trust policy holds back admits it */
> +	if (device_policy_is_disabled(&usb_dev->dev)) {
> +		result = device_trust_set_policy(&usb_dev->dev,
> +						 DEVICE_TRUST_POLICY_FULL);
> +		if (result)
> +			return result;
> +	}
> +
>  	usb_lock_device(usb_dev);
> +	if (!usb_dev->dev.driver) {
> +		/* Binding the generic driver configures the device */
> +		usb_dev->authorized = 1;
> +		usb_unlock_device(usb_dev);
> +		result = device_attach(&usb_dev->dev);
> +		return result < 0 ? result : 0;
> +	}
>  	if (usb_dev->authorized == 1)
>  		goto out_authorized;

[Severity: Medium]
Can a concurrent usb_deauthorize_device() race with usb_authorize_device()
across the unlocked windows between device_trust_set_policy(),
usb_lock_device(), and device_attach()?

Because usb_alloc_dev() initializes usb_dev->authorized to 1,
usb_device_authorized(usb_dev) becomes true as soon as
device_trust_set_policy() sets DEVICE_TRUST_POLICY_FULL and drops the device
lock:

CPU0                                  CPU1
usb_authorize_device(usb_dev)
  device_trust_set_policy(..., FULL)
    device_lock(&usb_dev->dev)
    usb_dev->dev.trust_policy = FULL
    device_unlock(&usb_dev->dev)
                                      usb_deauthorize_device(usb_dev)
                                        usb_lock_device(usb_dev)
                                        usb_dev->authorized = 0
                                        usb_set_configuration(usb_dev, -1)
                                        usb_unlock_device(usb_dev)
  usb_lock_device(usb_dev)
  if (!usb_dev->dev.driver)
    usb_dev->authorized = 1
    usb_unlock_device(usb_dev)
    device_attach(&usb_dev->dev)  <-- configures deauthorized device

A deauthorization that completed on CPU1 is silently overwritten, leaving the
device authorized and configured.  If usb_deauthorize_device() instead runs
between usb_unlock_device() and device_attach(), device_attach() still binds
usb_generic_driver and commits DEVICE_TRUST_FULL on the deauthorized device.

> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> index 2d75b85b2d139..ddede900dace8 100644
> --- a/drivers/usb/core/message.c
> +++ b/drivers/usb/core/message.c

[ ... ]

> @@ -1872,16 +1872,27 @@ void usb_deauthorize_interface(struct usb_interface *intf)
>   * usb_authorize_interface - authorize an USB interface
>   *
>   * @intf: USB interface structure
> + *
> + * Return: 0 on success, or the error of the trust policy request.
>   */
> -void usb_authorize_interface(struct usb_interface *intf)
> +int usb_authorize_interface(struct usb_interface *intf)
>  {
>  	struct device *dev = &intf->dev;
> +	int ret;
> +
> +	/* Authorizing an interface that its trust policy holds back admits it */
> +	if (device_policy_is_disabled(dev)) {
> +		ret = device_trust_set_policy(dev, DEVICE_TRUST_POLICY_FULL);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	if (!intf->authorized) {
>  		device_lock(dev);
>  		intf->authorized = 1; /* authorize interface */
>  		device_unlock(dev);
>  	}
> +	return 0;
>  }

[Severity: Medium]
Can usb_authorize_interface() race with a concurrent
usb_deauthorize_interface() when device_trust_set_policy() and
intf->authorized = 1 run in separate critical sections without
device_lock(dev->parent)?

CPU0                                  CPU1
usb_authorize_interface(intf)
  device_trust_set_policy(..., FULL)
    device_lock(dev)
    dev->trust_policy = FULL
    device_unlock(dev)
                                      usb_deauthorize_interface(intf)
                                        device_lock(dev->parent)
                                        device_lock(dev)
                                        intf->authorized = 0
                                        device_unlock(dev)
                                        usb_forced_unbind_intf(intf)
                                          intf->needs_binding = 1
  if (!intf->authorized)
    device_lock(dev)
    intf->authorized = 1  <-- clobbers deauthorization and races on bitfield
    device_unlock(dev)
                                        device_unlock(dev->parent)

Because intf->authorized starts at 1, usb_interface_authorized(intf) becomes
true as soon as device_trust_set_policy() drops device_lock(dev).  If
usb_deauthorize_interface() runs at that point, usb_authorize_interface()
then sees !intf->authorized and sets intf->authorized back to 1 without
holding device_lock(dev->parent), undoing the deauthorization and racing with
usb_forced_unbind_intf() writing intf->needs_binding in the same bitfield
word.

[ ... ]

> @@ -2173,7 +2184,10 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
>  		intfc = cp->intf_cache[i];
>  		intf->altsetting = intfc->altsetting;
>  		intf->num_altsetting = intfc->num_altsetting;
> -		intf->authorized = !!HCD_INTF_AUTHORIZED(hcd);
> +		/* The authorization default is the interface's initial trust policy */
> +		if (!HCD_INTF_AUTHORIZED(hcd))
> +			intf->dev.trust_policy = DEVICE_TRUST_POLICY_DISABLED;
> +		intf->authorized = 1;
>  		kref_get(&intfc->ref);

[Severity: Medium]
When a USB device is authorized with the "adversary" trust policy, does
usb_set_configuration() fail to propagate that policy to its child
interfaces?

Documentation/usb/authorization.rst explains that writing "adversary" to a
device's trust_policy before writing 1 to authorized operates the device as
an adversary, and boot rules such as device_trust=usb:1-2=adversary set the
policy on the struct usb_device as well.  However, usb_set_configuration()
leaves intf->dev.trust_policy at DEVICE_TRUST_POLICY_DEFAULT whenever
HCD_INTF_AUTHORIZED(hcd) is set, and usb_bus_type does not propagate parent
adversarial trust to child interfaces.  As a result, USB interface drivers
bind to intf->dev with DEVICE_TRUST_FULL and device_is_adversarial(&intf->dev)
returns false.

> diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> index 2559a180fdeec..8b427209bcc02 100644
> --- a/drivers/usb/core/usb.c
> +++ b/drivers/usb/core/usb.c
> @@ -747,7 +747,10 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent,
>  	dev->active_duration = -jiffies;
>  #endif
>  
> -	dev->authorized = usb_dev_authorized(dev, usb_hcd);
> +	/* The authorization default is the device's initial trust policy */
> +	if (!usb_dev_authorized(dev, usb_hcd))
> +		dev->dev.trust_policy = DEVICE_TRUST_POLICY_DISABLED;
> +	dev->authorized = 1;

[Severity: High]
When a USB device is held with DEVICE_TRUST_POLICY_DISABLED so
usb_generic_driver does not bind, does usb_resume_device() fail with
-ENOTCONN after the device is suspended?

Previously, an unauthorized USB device still bound usb_generic_driver during
device_add(), so udev->dev.driver was non-NULL.  With udev->dev.driver now
NULL on a trust-disabled device, usb_suspend_device() falls back to
usb_generic_driver and suspends the port, setting udev->state to
USB_STATE_SUSPENDED, but usb_resume_device() rejects unbound devices:

drivers/usb/core/driver.c:usb_resume_device() {
    ...
	/* Can't resume it if it doesn't have a driver. */
	if (udev->dev.driver == NULL) {
		status = -ENOTCONN;
		goto done;
	}
    ...
}

System sleep resume and runtime resume therefore fail with -ENOTCONN, leaving
the upstream port suspended and udev->state stuck in USB_STATE_SUSPENDED, and
subsequent authorization via usb_authorize_device() fails when trying to
configure the device over the suspended port.

[Severity: High]
Does leaving udev->dev.driver NULL on a trust-disabled USB device also skip
usb_notify_remove_device() when the device is disconnected before being
authorized?

usb_disconnect() relies on the bound device driver's disconnect callback to
invoke the remove-device notifier chain during device_del(&udev->dev), and
usb_generic_driver_disconnect() is the only caller of
usb_notify_remove_device(udev).  While a device is held back by device trust,
its /dev/bus/usb/BBB/DDD node still exists and can be opened via
usbdev_open().  If the device is unplugged without ever binding
usb_generic_driver, usb_notify_remove_device() and usbdev_remove() are never
called, so processes blocked in poll() or usbfs ioctls on that file
descriptor are never woken up and hang indefinitely.

>  	trace_usb_alloc_dev(dev);
>  	return dev;
>  }

[Severity: Low]
Should usb_alloc_dev() call device_trust_init(&dev->dev) before
trace_usb_alloc_dev(dev)?

trace_usb_alloc_dev() and the early trace_usb_set_device_state() calls prior
to device_add() record usb_device_authorized(udev).  Because
device_trust_init(&dev->dev) has not been called yet, those tracepoints log
the HCD default policy rather than any boot policy from device_trust.default=
or device_trust=.

> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index 825f03b290670..2ead3234b1301 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h

[ ... ]

> @@ -747,6 +753,15 @@ struct usb_device {
>  
>  #define to_usb_device(__dev)	container_of_const(__dev, struct usb_device, dev)
>  
> +/*
> + * A device is authorized when its trust policy lets it bind and user space has
> + * not deauthorized it since.
> + */
> +static inline bool usb_device_authorized(const struct usb_device *udev)
> +{
> +	return udev->authorized && !device_policy_is_disabled(&udev->dev);
> +}

[Severity: Medium]
Does usb_device_authorized() report true as soon as trust_policy is changed
from disabled, even before the device has been authorized or bound to a
driver?

Because usb_alloc_dev() initializes udev->authorized to 1, writing
"adversary" to trust_policy (as Documentation/usb/authorization.rst instructs
before writing 1 to authorized) makes device_policy_is_disabled(&udev->dev)
false and usb_device_authorized(udev) true while udev->dev is still unbound
and unadmitted.  At that point, reading sysfs authorized already returns 1,
and writing to bConfigurationValue (or issuing USBDEVFS_SETCONFIGURATION)
passes the !usb_device_authorized(dev) check in usb_set_configuration() and
configures the device and binds interface drivers while udev->dev.driver is
still NULL and udev->dev's trust level is still DEVICE_TRUST_DISABLED.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-device-trust-core-v2-0-3b12d9a9f861@nvidia.com?part=19