linux-kernel - Re: [PATCH v4] usb: gadget: f_fs: add capability for dfu run-time descriptor

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: <2024080516-flatness-humorous-03ca@gregkh>
Date: Mon, 5 Aug 2024 09:01:52 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: crwulff@...il.com
Cc: linux-usb@...r.kernel.org, Jonathan Corbet <corbet@....net>,
	Paul Cercueil <paul@...pouillou.net>,
	Christian Brauner <brauner@...nel.org>,
	Eric Farman <farman@...ux.ibm.com>,
	Wesley Cheng <quic_wcheng@...cinc.com>,
	Dmitry Antipov <dmantipov@...dex.ru>,
	Jeff Layton <jlayton@...nel.org>, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org, David Sands <david.sands@...mp.com>
Subject: Re: [PATCH v4] usb: gadget: f_fs: add capability for dfu run-time
 descriptor

On Sun, Aug 04, 2024 at 08:06:40PM -0400, crwulff@...il.com wrote:
> From: David Sands <david.sands@...mp.com>
> 
> From: David Sands <david.sands@...mp.com>

Twice?

> Add the ability for FunctionFS driver to be able to create DFU Run-Time
> descriptors.

As others said, please spell out "DFU" and I do not think that
"Run-Time" needs Capital letters, or a '-', right?

Also include here a lot more description of how this is to be used.

> 
> Signed-off-by: David Sands <david.sands@...mp.com>
> Co-developed-by: Chris Wulff <crwulff@...il.com>
> Signed-off-by: Chris Wulff <crwulff@...il.com>
> ---
> v4: Clean up unneeded change, switch to BIT macros, more documentation
> v3: Documentation, additional constants and constant order fixed
> https://lore.kernel.org/all/CO1PR17MB54197F118CBC8783D289B97DE1102@CO1PR17MB5419.namprd17.prod.outlook.com/
> v2: https://lore.kernel.org/linux-usb/CO1PR17MB54198D086B61F7392FA9075FE10E2@CO1PR17MB5419.namprd17.prod.outlook.com/
> v1: https://lore.kernel.org/linux-usb/CO1PR17MB5419AC3907C74E28D80C5021E1082@CO1PR17MB5419.namprd17.prod.outlook.com/
> ---
>  Documentation/usb/functionfs-desc.rst | 22 ++++++++++++++++++++++
>  Documentation/usb/functionfs.rst      |  2 ++
>  Documentation/usb/index.rst           |  1 +
>  drivers/usb/gadget/function/f_fs.c    | 12 ++++++++++--
>  include/uapi/linux/usb/ch9.h          |  8 ++++++--
>  include/uapi/linux/usb/functionfs.h   | 25 +++++++++++++++++++++++++
>  6 files changed, 66 insertions(+), 4 deletions(-)
>  create mode 100644 Documentation/usb/functionfs-desc.rst
> 
> diff --git a/Documentation/usb/functionfs-desc.rst b/Documentation/usb/functionfs-desc.rst
> new file mode 100644
> index 000000000000..73d2b8a3f02c
> --- /dev/null
> +++ b/Documentation/usb/functionfs-desc.rst
> @@ -0,0 +1,22 @@
> +======================
> +FunctionFS Descriptors
> +======================
> +
> +Interface Descriptors
> +---------------------
> +
> +Standard USB interface descriptors may be added. The class/subclass of the
> +most recent interface descriptor determines what type of class-specific
> +descriptors are accepted.
> +
> +Class-Specific Descriptors
> +--------------------------
> +

Why an empty section?

> +DFU Functional Descriptor
> +~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +When the interface class is USB_CLASS_APP_SPEC and  the interface subclass

Extra space?


> +is USB_SUBCLASS_DFU, a DFU functional descriptor can be provided.

Provided how?

> +
> +.. kernel-doc:: include/uapi/linux/usb/functionfs.h
> +   :doc: usb_dfu_functional_descriptor
> diff --git a/Documentation/usb/functionfs.rst b/Documentation/usb/functionfs.rst
> index d05a775bc45b..4f96e4b93d7b 100644
> --- a/Documentation/usb/functionfs.rst
> +++ b/Documentation/usb/functionfs.rst
> @@ -70,6 +70,8 @@ have been written to their ep0's.
>  Conversely, the gadget is unregistered after the first USB function
>  closes its endpoints.
>  
> +For more information about FunctionFS descriptors see :doc:`functionfs-desc`
> +
>  DMABUF interface
>  ================
>  
> diff --git a/Documentation/usb/index.rst b/Documentation/usb/index.rst
> index 27955dad95e1..826492c813ac 100644
> --- a/Documentation/usb/index.rst
> +++ b/Documentation/usb/index.rst
> @@ -11,6 +11,7 @@ USB support
>      dwc3
>      ehci
>      functionfs
> +    functionfs-desc

That's an odd name for a DFU-specific file, right?

Where are the Documentation/ABI/ entries?

>      gadget_configfs
>      gadget_hid
>      gadget_multi
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index d8b096859337..ba5c6e4827ba 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -2478,7 +2478,7 @@ typedef int (*ffs_os_desc_callback)(enum ffs_os_desc_type entity,
>  
>  static int __must_check ffs_do_single_desc(char *data, unsigned len,
>  					   ffs_entity_callback entity,
> -					   void *priv, int *current_class)
> +					   void *priv, int *current_class, int *current_subclass)
>  {
>  	struct usb_descriptor_header *_ds = (void *)data;
>  	u8 length;
> @@ -2535,6 +2535,7 @@ static int __must_check ffs_do_single_desc(char *data, unsigned len,
>  		if (ds->iInterface)
>  			__entity(STRING, ds->iInterface);
>  		*current_class = ds->bInterfaceClass;
> +		*current_subclass = ds->bInterfaceSubClass;
>  	}
>  		break;
>  
> @@ -2559,6 +2560,12 @@ static int __must_check ffs_do_single_desc(char *data, unsigned len,
>  			if (length != sizeof(struct ccid_descriptor))
>  				goto inv_length;
>  			break;
> +		} else if (*current_class == USB_CLASS_APP_SPEC &&
> +			   *current_subclass == USB_SUBCLASS_DFU) {
> +			pr_vdebug("dfu functional descriptor\n");
> +			if (length != sizeof(struct usb_dfu_functional_descriptor))
> +				goto inv_length;
> +			break;
>  		} else {
>  			pr_vdebug("unknown descriptor: %d for class %d\n",
>  			      _ds->bDescriptorType, *current_class);
> @@ -2621,6 +2628,7 @@ static int __must_check ffs_do_descs(unsigned count, char *data, unsigned len,
>  	const unsigned _len = len;
>  	unsigned long num = 0;
>  	int current_class = -1;
> +	int current_subclass = -1;
>  
>  	for (;;) {
>  		int ret;
> @@ -2640,7 +2648,7 @@ static int __must_check ffs_do_descs(unsigned count, char *data, unsigned len,
>  			return _len - len;
>  
>  		ret = ffs_do_single_desc(data, len, entity, priv,
> -			&current_class);
> +			&current_class, &current_subclass);
>  		if (ret < 0) {
>  			pr_debug("%s returns %d\n", __func__, ret);
>  			return ret;
> diff --git a/include/uapi/linux/usb/ch9.h b/include/uapi/linux/usb/ch9.h
> index 44d73ba8788d..91f0f7e214a5 100644
> --- a/include/uapi/linux/usb/ch9.h
> +++ b/include/uapi/linux/usb/ch9.h
> @@ -254,6 +254,9 @@ struct usb_ctrlrequest {
>  #define USB_DT_DEVICE_CAPABILITY	0x10
>  #define USB_DT_WIRELESS_ENDPOINT_COMP	0x11
>  #define USB_DT_WIRE_ADAPTER		0x21
> +/* From USB Device Firmware Upgrade Specification, Revision 1.1 */
> +#define USB_DT_DFU_FUNCTIONAL		0x21
> +/* these are from the Wireless USB spec */
>  #define USB_DT_RPIPE			0x22
>  #define USB_DT_CS_RADIO_CONTROL		0x23
>  /* From the T10 UAS specification */
> @@ -329,9 +332,10 @@ struct usb_device_descriptor {
>  #define USB_CLASS_USB_TYPE_C_BRIDGE	0x12
>  #define USB_CLASS_MISC			0xef
>  #define USB_CLASS_APP_SPEC		0xfe
> -#define USB_CLASS_VENDOR_SPEC		0xff
> +#define USB_SUBCLASS_DFU			0x01
>  
> -#define USB_SUBCLASS_VENDOR_SPEC	0xff
> +#define USB_CLASS_VENDOR_SPEC		0xff
> +#define USB_SUBCLASS_VENDOR_SPEC		0xff
>  
>  /*-------------------------------------------------------------------------*/
>  
> diff --git a/include/uapi/linux/usb/functionfs.h b/include/uapi/linux/usb/functionfs.h
> index 9f88de9c3d66..40f87cbabf7a 100644
> --- a/include/uapi/linux/usb/functionfs.h
> +++ b/include/uapi/linux/usb/functionfs.h
> @@ -37,6 +37,31 @@ struct usb_endpoint_descriptor_no_audio {
>  	__u8  bInterval;
>  } __attribute__((packed));
>  
> +/**
> + * struct usb_dfu_functional_descriptor - DFU Functional descriptor
> + * @bLength:		Size of the descriptor (bytes)
> + * @bDescriptorType:	USB_DT_DFU_FUNCTIONAL
> + * @bmAttributes:	DFU attributes
> + * @wDetachTimeOut:	Maximum time to wait after DFU_DETACH (ms, le16)
> + * @wTransferSize:	Maximum number of bytes per control-write (le16)
> + * @bcdDFUVersion:	DFU Spec version (BCD, le16)
> + */
> +struct usb_dfu_functional_descriptor {
> +	__u8  bLength;
> +	__u8  bDescriptorType;
> +	__u8  bmAttributes;
> +	__le16 wDetachTimeOut;
> +	__le16 wTransferSize;
> +	__le16 bcdDFUVersion;
> +} __attribute__ ((packed));
> +
> +/* from DFU functional descriptor bmAttributes */
> +#define DFU_FUNC_ATT_CAN_DOWNLOAD	BIT(0)
> +#define DFU_FUNC_ATT_CAN_UPLOAD		BIT(1)
> +#define DFU_FUNC_ATT_MANIFEST_TOLERANT	BIT(2)
> +#define DFU_FUNC_ATT_WILL_DETACH	BIT(3)

Wrong macro for bit fields for uapi .h files :(

thanks,

greg k-h