Message-ID: <8cc10112-23a7-41af-b81f-7fc0c097d34d@oss.cyber.gouv.fr>
Date: Mon, 30 Jun 2025 13:20:27 +0200
From: Nicolas Bouchinet <nicolas.bouchinet@....cyber.gouv.fr>
To: Alan Stern <stern@...land.harvard.edu>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Kannappan R <r.kannappan@...el.com>,
 Sabyrzhan Tasbolatov <snovitoll@...il.com>,
 Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
 Stefan Eichenberger <stefan.eichenberger@...adex.com>,
 Thomas Gleixner <tglx@...utronix.de>, Pawel Laszczak <pawell@...ence.com>,
 Ma Ke <make_ruc2021@....com>, Jeff Johnson <jeff.johnson@....qualcomm.com>,
 Luc Bonnafoux <luc.bonnafoux@....gouv.fr>,
 Luc Bonnafoux <luc.bonnafoux@....cyber.gouv.fr>,
 Nicolas Bouchinet <nicolas.bouchinet@....gouv.fr>,
 linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [RFC PATCH 3/4] usb: core: Plug the usb authentication capability

Hi Alan,

Thank you very much for your review. We will take every form remarks into
account in the next patch version.

On 6/20/25 21:11, Alan Stern wrote:
> On Fri, Jun 20, 2025 at 04:27:18PM +0200, nicolas.bouchinet@....cyber.gouv.fr wrote:
>> From: Nicolas Bouchinet <nicolas.bouchinet@....gouv.fr>
>>
>> Plugs the usb authentication implementation in the usb stack and more
>> particularly in the usb_parse_configuration function after the BOS has
>> been parsed and the usb authentication capacity has been controlled.
>>
>> The authentication bulk is implemented by the usb_authenticate_device
>> function.
>>
>> The authorization decision enforcement is done via the authorized field of
>> the usb_device and the associated authorization and deauthorization functions.
>> The usb_device also contains an authenticated field that could be used to track
>> the result of the authentication process and allow for more complex security
>> policy: the user could manually authorize a device that failed the
>> authentication or manually deauthorize a device that was previously
>> authenticated.
>>
>> The usb_authenticate_device returns 0 or an error code. If 0 is
>> returned, the authorized and authenticated fields of the usb_device are
>> updated with the result of the authentication.
>>
>> Co-developed-by: Luc Bonnafoux <luc.bonnafoux@....gouv.fr>
>> Signed-off-by: Luc Bonnafoux <luc.bonnafoux@....gouv.fr>
>> Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@....gouv.fr>
>> ---
> Here are some more stylistic suggestions, similar to what Greg said.
>
>> @@ -824,7 +825,50 @@ static int usb_parse_configuration(struct usb_device *dev, int cfgidx,
>>   		kref_init(&intfc->ref);
>>   	}
>>   
>> -	/* FIXME: parse the BOS descriptor */
>> +	/* If device USB version is above 2.0, get BOS descriptor */
>> +	/*
>> +	 * Requirement for bcdUSB >= 2.10 is defined in USB 3.2 §9.2.6.6
>> +	 * "Devices with a value of at least 0210H in the bcdUSB field of their
>> +	 * device descriptor shall support GetDescriptor (BOS Descriptor) requests."
>> +	 *
>> +	 * To discuss, BOS request could be also sent for bcdUSB >= 0x0201
>> +	 */
>> +	// Set a default value for authenticated at true in order not to block devices
>> +	// that do not support the authentication
> It looks really weird to see three comment blocks, in three different
> styles, right next to each other.  In the kernel, we avoid C++-style //
> comments.  And two adjacent /**/-style comments would normally be
> separated by a blank line or just merged into one bigger comment.
>
>> +	dev->authenticated = 1;
>> +
>> +	if (le16_to_cpu(dev->descriptor.bcdUSB) >= 0x0201) {
>> +		pr_notice("bcdUSB >= 0x0201\n");
>> +		retval = usb_get_bos_descriptor(dev);
>> +		if (!retval) {
>> +			pr_notice("found BOS\n");
>> +#ifdef CONFIG_USB_AUTHENTICATION
>> +			if (dev->bos->authent_cap) {
>> +				/* If authentication cap is present, start device authent */
>> +				pr_notice("found Authent BOS\n");
>> +				retval = usb_authenticate_device(dev);
>> +				if (retval != 0) {
>> +					pr_err("failed to authenticate the device: %d\n",
>> +					       retval);
>> +				} else if (!dev->authenticated) {
>> +					pr_notice("device has been rejected\n");
>> +					// return early from the configuration process
>> +					return 0;
> Do these two cases need to be handled separately?  Regardless of whether
> the function call fails, or succeeds but gives a negative result,
> shouldn't the end result be the same?
>
>> +				} else {
>> +					pr_notice("device has been authorized\n");
>> +				}
> Be careful not to mix up the two separate notions of authentication and
> authorization.  It's already difficult to keep them straight, because
> the words are so similar.


You are right indeed.

We moved the `usb_authenticate_dev()` call in `usb_new_device()` in 
order to
perform the authentication only once the device configuration is 
complete. Also
we think we need to split the problem of handling the authentication vs
authorization in two parts.

- which component has authority to set the two fields ?
- where/how is it enforced ?

To answer the first question :

- We think that the authenticated field can only be set by the
`usb_authenticate_dev()` function.

- it is less clear for the authorized status which is already 
manipulated by
the sysfs (usbguard) and the default hcd policy.

The reconciliation between the two fields could be done at the enforcement
point. In `usb_probe_interface()` instead of simply checking the 
authorized flag
it could check a more complex policy. For example:

+-------------------+----------------------------------------+----------------+ 

|                   | authorized                             | not 
authorized |
+-------------------+----------------------------------------+----------------+ 

| authenticated     | OK                                     | NOK       
      |
+-------------------+----------------------------------------+----------------+ 

| not authenticated | Depends on tolerance in local security 
|                |
|                   | policy (set by cmdline or sysctl)      | NOK      
       |
+-------------------+----------------------------------------+----------------+ 


This way it would also help to handle internal devices. When 
`hcd->dev_policy` is
set to USB_DEVICE_AUTHORIZE_INTERNAL, only internal devices are 
authorized by
default on connection. So external devices will have to be authenticated 
and
then authorized via the sysfs. Internal devices will be authorized and not
authenticated.

>
>> +			} else {
>> +				// USB authentication unsupported
>> +				// Apply security policy on failed devices
>> +				pr_notice("no authentication capability\n");
>> +			}
>> +#endif
> We generally prefer to avoid #if or #ifdef blocks inside function
> definitions, if at all possible.  In this case, you could define a
> separate function usb_handle_bos_authent() (or whatever you want to call
> it) that does this work, all inside a #ifdef block, along with a #else
> section that defines usb_handle_bos_authent() to be an inline empty
> function.
>
>> +		} else {
>> +			// Older USB version, authentication not supported
>> +			// Apply security policy on failed devices
>> +			pr_notice("device does not have a BOS descriptor\n");
>> +		}
>> +	}
>>   
>>   	/* Skip over any Class Specific or Vendor Specific descriptors;
>>   	 * find the first interface descriptor */
>> @@ -1051,6 +1095,7 @@ int usb_get_bos_descriptor(struct usb_device *dev)
>>   	length = bos->bLength;
>>   	total_len = le16_to_cpu(bos->wTotalLength);
>>   	num = bos->bNumDeviceCaps;
>> +
> Patches shouldn't make extraneous whitespace changes to existing code.
>
>>   	kfree(bos);
>>   	if (total_len < length)
>>   		return -EINVAL;
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index 0e1dd6ef60a719f59a22d86caeb20f86991b5b29..753e55155ea34a7739b5f530dad429534e60842d 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -2640,6 +2640,12 @@ int usb_new_device(struct usb_device *udev)
>>   	udev->dev.devt = MKDEV(USB_DEVICE_MAJOR,
>>   			(((udev->bus->busnum-1) * 128) + (udev->devnum-1)));
>>   
>> +	// TODO: Check the device state, we want to avoid semi-initialized device to userspace.
>> +	if (!udev->authenticated) {
>> +		// If the device is not authenticated, abort the procedure
>> +		goto fail;
> A comment that simply repeats what the code already says is not very
> useful.  Such comments do exist here and there (I'm guilty of adding
> some of them myself), but in general they should be avoided.
>
>> diff --git a/include/linux/usb.h b/include/linux/usb.h
>> index b46738701f8dc46085f251379873b6a8a008d99d..e9037c8120b43556f8610f9acb3ad4129e847b98 100644
>> --- a/include/linux/usb.h
>> +++ b/include/linux/usb.h
>> @@ -431,6 +431,8 @@ struct usb_host_bos {
>>   	struct usb_ssp_cap_descriptor	*ssp_cap;
>>   	struct usb_ss_container_id_descriptor	*ss_id;
>>   	struct usb_ptm_cap_descriptor	*ptm_cap;
>> +	/* Authentication capability */
>> +	struct usb_authent_cap_descriptor *authent_cap;
> None of the other entries here have a comment like this.  Why does the
> new entry need one?
>
> Alan Stern

Powered by blists - more mailing lists