linux-kernel - Re: [RFC PATCH 3/4] usb: core: Plug the usb authentication capability

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1a85b3c3-66e1-4a31-ad39-391b03393bf9@rowland.harvard.edu>
Date: Fri, 20 Jun 2025 15:11:36 -0400
From: Alan Stern <stern@...land.harvard.edu>
To: nicolas.bouchinet@....cyber.gouv.fr
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

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.

> +			} 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