| 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
| ||
|
Message-ID: <CAB0kiBKGb=vuYbs1C3w2wzmbSZuVp3t9iqjkfL+dYkKQKgA7ow@mail.gmail.com>
Date: Mon, 5 Aug 2024 20:21:54 -0400
From: Chris Wulff <crwulff@...il.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
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 Mon, Aug 5, 2024 at 3:01 AM Greg Kroah-Hartman
<gregkh@...uxfoundation.org> wrote:
>
> 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?
Oops. Not sure what happened, but I'll try to make it not happen next time.
>
> > 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.
Ok, I will expand on this (and the associated documentation.)
>
> >
> > 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?
It was just a heading-2 for the class-specific descriptor section (with each
of the class-specific descriptors being heading-3). I can add a bit of
text though.
>
> > +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?
I will expand on this a bit more. Most of the functionfs descriptor
behavior wasn't documented. The functionfs page talks about how
these are written to the ep0 file, but doesn't mention anything about
what descriptors can be written other than mentioning that ep# files
are created when endpoint descriptors are written.
>
> > +
> > +.. 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?
functionfs-desc was intended to be for more than DFU. I was thinking
it would be nice to also talk about other descriptors that can be
written to functionfs since I couldn't find any documentation on
that, but I didn't want to add documentation for a bunch of existing
stuff to this same patch. It seems like that would be better submitted
separately (which I can work on if you think it's useful.) I only included
the descriptors that were relevant to DFU.
I will see what I can add for the ABI documentation as well.
>
> > 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,
> > - ¤t_class);
> > + ¤t_class, ¤t_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 :(
Oh. I'm surprised there is more than one macro for bits. I will
change this to _BITUL().
>
> thanks,
>
> greg k-h
Powered by blists - more mailing lists