From: Nicolas Bouchinet <nicolas.bouchinet@ssi.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@ssi.gouv.fr>
Signed-off-by: Luc Bonnafoux <luc.bonnafoux@ssi.gouv.fr>
Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
---
 drivers/usb/core/config.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/usb/core/hub.c    |  6 ++++++
 drivers/usb/core/usb.c    |  5 +++++
 include/linux/usb.h       |  2 ++
 4 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
index 13bd4ec4ea5f7a6fef615b6f50b1acb3bbe44ba4..45ee8e93e263c41f1bf4271be4e69ccfcac3f650 100644
--- a/drivers/usb/core/config.c
+++ b/drivers/usb/core/config.c
@@ -14,6 +14,7 @@
 #include <asm/byteorder.h>
 #include "usb.h"
 
+#include "authent.h"
 
 #define USB_MAXALTSETTING		128	/* Hard limit */
 
@@ -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
+	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;
+				} else {
+					pr_notice("device has been authorized\n");
+				}
+			} else {
+				// USB authentication unsupported
+				// Apply security policy on failed devices
+				pr_notice("no authentication capability\n");
+			}
+#endif
+		} 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;
+
 	kfree(bos);
 	if (total_len < length)
 		return -EINVAL;
@@ -1122,6 +1167,10 @@ int usb_get_bos_descriptor(struct usb_device *dev)
 			dev->bos->ptm_cap =
 				(struct usb_ptm_cap_descriptor *)buffer;
 			break;
+		case USB_AUTHENT_CAP_TYPE:
+			dev->bos->authent_cap =
+				(struct usb_authent_cap_descriptor *)buffer;
+			break;
 		default:
 			break;
 		}
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;
+	}
+
 	/* Tell the world! */
 	announce_device(udev);
 
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 0b4685aad2d50337f3cacb2198c95a68ae8eff86..76847c01d3493e2527992a3bb927422519d9a974 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -46,6 +46,7 @@
 #include <linux/dma-mapping.h>
 
 #include "hub.h"
+#include "authent_netlink.h"
 
 const char *usbcore_name = "usbcore";
 
@@ -1080,6 +1081,10 @@ static int __init usb_init(void)
 	usb_debugfs_init();
 
 	usb_acpi_register();
+
+	// TODO : check error case
+	usb_auth_init_netlink();
+
 	retval = bus_register(&usb_bus_type);
 	if (retval)
 		goto bus_register_failed;
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;
 };
 
 int __usb_get_extra_descriptor(char *buffer, unsigned size,

-- 
2.50.0

Hi,

I am afraid someone has to address this.

On 20.06.25 16:27, nicolas.bouchinet@oss.cyber.gouv.fr wrote:

> +	// Set a default value for authenticated at true in order not to block devices
> +	// that do not support the authentication
> +	dev->authenticated = 1;

So the default is authenticated. OK.

> +	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 the device claims not to support authentication ...

> +				/* 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;
> +				} else {
> +					pr_notice("device has been authorized\n");
> +				}
> +			} else {
> +				// USB authentication unsupported
> +				// Apply security policy on failed devices
> +				pr_notice("no authentication capability\n");

... we do nothing about it. We enumerate.

The purpose of authentication is guarding against unknown or malicious devices,
isn't it? This behavior seems to be kind of incompatible with the goal.

	Regards
		Oliver

Hi Olivier,

Thank you for your review.

Indeed our current implementation of the usb authentication is still a bit
crude.
Currently, most, if not all of usb devices can't handle authentication. 
If we
want to have an integration that doesn't break on current hosts, we need to
have a fail safe. We are still working on the best way to handle the
combination of authentication and authorization.
See the reply to Alan [1].

[1]: 
https://lore.kernel.org/linux-usb/8cc10112-23a7-41af-b81f-7fc0c097d34d@oss.cyber.gouv.fr/

On 6/23/25 20:15, Oliver Neukum wrote:
> Hi,
>
> I am afraid someone has to address this.
>
> On 20.06.25 16:27, nicolas.bouchinet@oss.cyber.gouv.fr wrote:
>
>> +    // Set a default value for authenticated at true in order not to 
>> block devices
>> +    // that do not support the authentication
>> +    dev->authenticated = 1;
>
> So the default is authenticated. OK.
>
>> +    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 the device claims not to support authentication ...
>
>> +                /* 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;
>> +                } else {
>> +                    pr_notice("device has been authorized\n");
>> +                }
>> +            } else {
>> +                // USB authentication unsupported
>> +                // Apply security policy on failed devices
>> +                pr_notice("no authentication capability\n");
>
> ... we do nothing about it. We enumerate.
>
> The purpose of authentication is guarding against unknown or malicious 
> devices,
> isn't it? This behavior seems to be kind of incompatible with the goal.
>
>     Regards
>         Oliver
>
>

Hi,

On 30.06.25 14:34, Nicolas Bouchinet wrote:
> Hi Olivier,
> 
> Thank you for your review.
> 
> Indeed our current implementation of the usb authentication is still a bit
> crude.

that is understood, but the question here is not just whether they
are crude, but whether they are sensible. You are putting the use of
the authentication code in usb_new_device(). This means that the admin
cannot change the settings of currently connected devices. It would seem
to me that the check should go into usb_claim_interface().

	Regards
		Oliver

On Fri, Jun 20, 2025 at 7:27 PM <nicolas.bouchinet@oss.cyber.gouv.fr> wrote:
>
> From: Nicolas Bouchinet <nicolas.bouchinet@ssi.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@ssi.gouv.fr>
> Signed-off-by: Luc Bonnafoux <luc.bonnafoux@ssi.gouv.fr>
> Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> ---
>  drivers/usb/core/config.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++-
>  drivers/usb/core/hub.c    |  6 ++++++
>  drivers/usb/core/usb.c    |  5 +++++
>  include/linux/usb.h       |  2 ++
>  4 files changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
> index 13bd4ec4ea5f7a6fef615b6f50b1acb3bbe44ba4..45ee8e93e263c41f1bf4271be4e69ccfcac3f650 100644
> --- a/drivers/usb/core/config.c
> +++ b/drivers/usb/core/config.c
> @@ -14,6 +14,7 @@
>  #include <asm/byteorder.h>
>  #include "usb.h"
>
> +#include "authent.h"
>
>  #define USB_MAXALTSETTING              128     /* Hard limit */
>
> @@ -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
> +       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;

Returning 0 after rejecting a device leaves udev half-initialised and still
linked in usb_bus_type (the hub driver only aborts after this call).
The next hot-plug may oops on dangling pointers.
Please consider returning -EPERM instead of 0.

> +                               } else {
> +                                       pr_notice("device has been authorized\n");
> +                               }
> +                       } else {
> +                               // USB authentication unsupported
> +                               // Apply security policy on failed devices
> +                               pr_notice("no authentication capability\n");
> +                       }
> +#endif
> +               } 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;
> +
>         kfree(bos);
>         if (total_len < length)
>                 return -EINVAL;
> @@ -1122,6 +1167,10 @@ int usb_get_bos_descriptor(struct usb_device *dev)
>                         dev->bos->ptm_cap =
>                                 (struct usb_ptm_cap_descriptor *)buffer;
>                         break;
> +               case USB_AUTHENT_CAP_TYPE:
> +                       dev->bos->authent_cap =
> +                               (struct usb_authent_cap_descriptor *)buffer;
> +                       break;
>                 default:
>                         break;
>                 }
> 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;
> +       }
> +
>         /* Tell the world! */
>         announce_device(udev);
>
> diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> index 0b4685aad2d50337f3cacb2198c95a68ae8eff86..76847c01d3493e2527992a3bb927422519d9a974 100644
> --- a/drivers/usb/core/usb.c
> +++ b/drivers/usb/core/usb.c
> @@ -46,6 +46,7 @@
>  #include <linux/dma-mapping.h>
>
>  #include "hub.h"
> +#include "authent_netlink.h"
>
>  const char *usbcore_name = "usbcore";
>
> @@ -1080,6 +1081,10 @@ static int __init usb_init(void)
>         usb_debugfs_init();
>
>         usb_acpi_register();
> +
> +       // TODO : check error case
> +       usb_auth_init_netlink();
> +
>         retval = bus_register(&usb_bus_type);
>         if (retval)
>                 goto bus_register_failed;
> 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;
>  };
>
>  int __usb_get_extra_descriptor(char *buffer, unsigned size,
>
> --
> 2.50.0
>

On 6/21/25 13:09, Sabyrzhan Tasbolatov wrote:
> On Fri, Jun 20, 2025 at 7:27 PM <nicolas.bouchinet@oss.cyber.gouv.fr> wrote:
>> From: Nicolas Bouchinet <nicolas.bouchinet@ssi.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@ssi.gouv.fr>
>> Signed-off-by: Luc Bonnafoux <luc.bonnafoux@ssi.gouv.fr>
>> Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
>> ---
>>   drivers/usb/core/config.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++-
>>   drivers/usb/core/hub.c    |  6 ++++++
>>   drivers/usb/core/usb.c    |  5 +++++
>>   include/linux/usb.h       |  2 ++
>>   4 files changed, 63 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
>> index 13bd4ec4ea5f7a6fef615b6f50b1acb3bbe44ba4..45ee8e93e263c41f1bf4271be4e69ccfcac3f650 100644
>> --- a/drivers/usb/core/config.c
>> +++ b/drivers/usb/core/config.c
>> @@ -14,6 +14,7 @@
>>   #include <asm/byteorder.h>
>>   #include "usb.h"
>>
>> +#include "authent.h"
>>
>>   #define USB_MAXALTSETTING              128     /* Hard limit */
>>
>> @@ -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
>> +       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;
> Returning 0 after rejecting a device leaves udev half-initialised and still
> linked in usb_bus_type (the hub driver only aborts after this call).
> The next hot-plug may oops on dangling pointers.
> Please consider returning -EPERM instead of 0.
We have changed the way we handle errors and 
authentication/authorization. It
will appear in the next version of the patch.
We have commented and explained how we want to proceed in reply to Alan [1].

[1]: 
https://lore.kernel.org/linux-usb/8cc10112-23a7-41af-b81f-7fc0c097d34d@oss.cyber.gouv.fr/

>
>> +                               } else {
>> +                                       pr_notice("device has been authorized\n");
>> +                               }
>> +                       } else {
>> +                               // USB authentication unsupported
>> +                               // Apply security policy on failed devices
>> +                               pr_notice("no authentication capability\n");
>> +                       }
>> +#endif
>> +               } 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;
>> +
>>          kfree(bos);
>>          if (total_len < length)
>>                  return -EINVAL;
>> @@ -1122,6 +1167,10 @@ int usb_get_bos_descriptor(struct usb_device *dev)
>>                          dev->bos->ptm_cap =
>>                                  (struct usb_ptm_cap_descriptor *)buffer;
>>                          break;
>> +               case USB_AUTHENT_CAP_TYPE:
>> +                       dev->bos->authent_cap =
>> +                               (struct usb_authent_cap_descriptor *)buffer;
>> +                       break;
>>                  default:
>>                          break;
>>                  }
>> 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;
>> +       }
>> +
>>          /* Tell the world! */
>>          announce_device(udev);
>>
>> diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
>> index 0b4685aad2d50337f3cacb2198c95a68ae8eff86..76847c01d3493e2527992a3bb927422519d9a974 100644
>> --- a/drivers/usb/core/usb.c
>> +++ b/drivers/usb/core/usb.c
>> @@ -46,6 +46,7 @@
>>   #include <linux/dma-mapping.h>
>>
>>   #include "hub.h"
>> +#include "authent_netlink.h"
>>
>>   const char *usbcore_name = "usbcore";
>>
>> @@ -1080,6 +1081,10 @@ static int __init usb_init(void)
>>          usb_debugfs_init();
>>
>>          usb_acpi_register();
>> +
>> +       // TODO : check error case
>> +       usb_auth_init_netlink();
>> +
>>          retval = bus_register(&usb_bus_type);
>>          if (retval)
>>                  goto bus_register_failed;
>> 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;
>>   };
>>
>>   int __usb_get_extra_descriptor(char *buffer, unsigned size,
>>
>> --
>> 2.50.0
>>

On Fri, Jun 20, 2025 at 04:27:18PM +0200, nicolas.bouchinet@oss.cyber.gouv.fr wrote:
> From: Nicolas Bouchinet <nicolas.bouchinet@ssi.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@ssi.gouv.fr>
> Signed-off-by: Luc Bonnafoux <luc.bonnafoux@ssi.gouv.fr>
> Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.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

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@oss.cyber.gouv.fr wrote:
>> From: Nicolas Bouchinet <nicolas.bouchinet@ssi.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@ssi.gouv.fr>
>> Signed-off-by: Luc Bonnafoux <luc.bonnafoux@ssi.gouv.fr>
>> Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.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

On Mon, Jun 30, 2025 at 01:20:27PM +0200, Nicolas Bouchinet wrote:
> We moved the `usb_authenticate_dev()` call in `usb_new_device()` in order to
> perform the authentication only once the device configuration is complete.

usb_new_device() does device initialization, not device configuration.  
The default configuration is selected by usb_choose_configuration(), but 
the config can be changed at any time by the user (via sysfs or usbfs).

> 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.

Okay, that seems like a reasonable approach.

Alan Stern