On Wed, Jul 02 2014, Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxx> wrote: > Add support for OS descriptors. The new format of descriptors is used, > because the "flags" field is required for extensions. os_count gives > the number of OSDesc[] elements. > The format of descriptors is given in include/uapi/linux/usb/functionfs.h. > > For extended properties descriptor the usb_ext_prop_desc structure covers > only a part of a descriptor, because the wPropertyNameLength is unknown > up front. > > Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxx> > --- > Rebased onto Felipe's testing/next with gadget directory cleanup patches > applied: > > http://www.spinics.net/lists/linux-usb/msg109928.html > > This is meant for 3.17. > > @Michal: I kindly ask for your review. Your comments will be very > valuable. For now I've only skimmed over binding code. > > drivers/usb/gadget/function/f_fs.c | 345 +++++++++++++++++++++++++++++++++++- > drivers/usb/gadget/function/u_fs.h | 7 + > include/uapi/linux/usb/functionfs.h | 87 ++++++++- > 3 files changed, 432 insertions(+), 7 deletions(-) > > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c > index 88d6fa2..55c0db7 100644 > --- a/drivers/usb/gadget/function/f_fs.c > +++ b/drivers/usb/gadget/function/f_fs.c > @@ -34,6 +34,7 @@ > > #include "u_fs.h" > #include "u_f.h" > +#include "u_os_desc.h" > #include "configfs.h" > > #define FUNCTIONFS_MAGIC 0xa647361 /* Chosen by a honest dice roll ;) */ > @@ -1644,11 +1645,18 @@ enum ffs_entity_type { > FFS_DESCRIPTOR, FFS_INTERFACE, FFS_STRING, FFS_ENDPOINT > }; > > +enum ffs_os_desc_type { > + FFS_OS_DESC, FFS_OS_DESC_EXT_COMPAT, FFS_OS_DESC_EXT_PROP > +}; > + > typedef int (*ffs_entity_callback)(enum ffs_entity_type entity, > u8 *valuep, > struct usb_descriptor_header *desc, > void *priv); > > +typedef int (*ffs_os_desc_callback)(enum ffs_os_desc_type entity, > + u8 *valuep, void *data, void *priv); > + > static int __must_check ffs_do_desc(char *data, unsigned len, > ffs_entity_callback entity, void *priv) > { > @@ -1855,11 +1863,190 @@ static int __ffs_data_do_entity(enum ffs_entity_type type, > return 0; > } > > +/* > + * Process all extended compatibility/extended property descriptors > + * of a feature descriptor > + */ > +static int __must_check ffs_do_os_desc(char *data, unsigned len, u8 *valuep, > + u16 feature_count, > + ffs_os_desc_callback entity, void *priv, > + struct usb_os_desc_header *desc) The name of this function has to be changed. Perhaps ffs_do_os_single_desc? It took me embarrassing amount of time to figure out that ffs_do_os_descs and ffs_do_os_desc are different functions. > +{ > + int ret; > + enum ffs_os_desc_type *type = (enum ffs_os_desc_type *)valuep; Read the value already: enum ffs_os_desc_type type = *(enum ffs_os_desc_type *)valuep; > + const unsigned _len = len; > + > + ENTER(); > + > + /* loop over all ext compat/ext prop descriptors */ > + while (feature_count--) { > + ret = entity(*type, (u8 *)desc, (void *)data, priv); You don't have to cast to void *. It's implicit. > + if (unlikely(ret < 0)) { > + pr_debug("bad OS descriptor, type: %d\n", *valuep); > + return ret; > + } > + data += ret; > + len -= ret; > + } > + return _len - len; > +} > + > +/* Process a number of complete Feature Descriptors (Ext Compat or Ext Prop) */ > +static int __must_check ffs_do_os_descs(unsigned count, > + char *data, unsigned len, > + ffs_os_desc_callback entity, void *priv) > +{ > + const unsigned _len = len; > + unsigned long num = 0; > + > + ENTER(); > + > + for (;;) { > + int ret; > + enum ffs_os_desc_type type; > + u16 feature_count; > + struct usb_os_desc_header *desc = (void *)data; if (len < sizeof(*desc)) return -EINVAL; With the union addition that I've mentioned near the end of the email, this would also handle checking if bCount/wCount is there. > + > + if (num == count) > + return _len - len; for (num = 0; num < count; ++num) { …; } return _len - len; > + > + /* Record "descriptor" entity */ > + /* > + * Process dwLength, bcdVersion, wIndex, > + * get b/wCount. > + * Move the data pointer to the beginning of > + * extended compatibilities proper or > + * extended properties proper portions of the > + * data > + */ > + ret = entity(FFS_OS_DESC, (u8 *)&type, desc, priv); This casting looks ugly. Just change type of the second argument of entity callback to void*. You are casting it back and forth anyway. > + if (unlikely(ret < 0)) { > + pr_debug("entity OS_DESCRIPTOR(%02lx); ret = %d\n", > + num, ret); > + return ret; > + } > + if (type == FFS_OS_DESC_EXT_COMPAT) { > + struct usb_ext_compat_desc_header *h = > + (struct usb_ext_compat_desc_header *)data; Add checking if h->Reserved is zero, reject if not. > + feature_count = h->bCount; > + } else if (type == FFS_OS_DESC_EXT_PROP) { > + struct usb_ext_prop_desc_header *h = (void *)data; > + > + feature_count = le16_to_cpu(h->wCount); > + } else { > + return -EINVAL; > + } Perhaps you could take advantage of the fact that in little endian a 16-bit “?? 00” is the same as 8-bit “??”. Something like: feature_count = le16_to_cpu(d->wCount); if (type == FFS_OS_DESC_EXT_COMPAT && feature_count > 255) return -EINVAL; > + if (type == FFS_OS_DESC_EXT_COMPAT) { > + struct usb_ext_compat_desc_header *h = > + (struct usb_ext_compat_desc_header *)data; Add checking if h->Reserved is zero, reject if not. > + feature_count = h->bCount; > + } else if (type == FFS_OS_DESC_EXT_PROP) { > + struct usb_ext_prop_desc_header *h = (void *)data; > + > + feature_count = le16_to_cpu(h->wCount); > + } else { > + return -EINVAL; > + } > + len -= (ret + 2); > + data += (ret + 2); > + > + /* > + * Process all function/property descriptors > + * of this Feature Descriptor > + */ > + ret = ffs_do_os_desc(data, len, (u8 *)&type, feature_count, > + entity, priv, desc); > + if (unlikely(ret < 0)) { > + pr_debug("%s returns %d\n", __func__, ret); > + return ret; > + } > + > + len -= ret; > + data += ret; > + ++num; > + } > +} > + > +/** > + * Validate contents of the buffer from userspace related to OS descriptors. > + */ > +static int __ffs_data_do_os_desc(enum ffs_os_desc_type type, > + u8 *valuep, void *data, void *priv) As commented above, make it “void *valuep”. > +{ > + struct ffs_data *ffs = priv; > + __u8 length; > + > + ENTER(); > + > + switch (type) { > + case FFS_OS_DESC: { > + struct usb_os_desc_header *desc = data; > + enum ffs_os_desc_type *next_type = > + (enum ffs_os_desc_type *)valuep; > + __u16 bcd_version = le16_to_cpu(desc->bcdVersion); > + __u16 w_index = le16_to_cpu(desc->wIndex); > + > + if (bcd_version != 0x1) { Is it just me, or does 0x1 look kinda weird? > + pr_vdebug("unsupported os descriptors version: %d", > + bcd_version); > + return -EINVAL; > + } > + if (w_index != 0x4 && w_index != 0x5) { > + pr_vdebug("unsupported os descriptor type: %d", > + w_index); > + return -EINVAL; Stash this as a “default” in the switch below. > + } > + switch (w_index) { > + case 0x4: > + *next_type = FFS_OS_DESC_EXT_COMPAT; > + break; > + case 0x5: > + *next_type = FFS_OS_DESC_EXT_PROP; > + break; > + } > + length = sizeof(*desc); > + } > + break; > + case FFS_OS_DESC_EXT_COMPAT: { > + struct usb_ext_compat_desc *d = data; if (len < sizeof *d) return -EINVAL; > + > + if (d->bFirstInterfaceNumber >= ffs->interfaces_count) > + return -EINVAL; > + Also add checking whether d->Reserved1 and d->Reserved2[] is zero, reject if not. > + length = sizeof(struct usb_ext_compat_desc); > + } > + break; > + case FFS_OS_DESC_EXT_PROP: { > + struct usb_os_desc_header *desc = > + (struct usb_os_desc_header *)valuep; > + struct usb_ext_prop_desc *d = data; > + __le32 type, pdl; > + __le16 pnl; Why __le32 and __le16? You want u32 and u16 here. if (len < sizeof *d) return -EINVAL; > + > + if (desc->interface >= ffs->interfaces_count) > + return -EINVAL; > + length = le32_to_cpu(d->dwSize); > + type = le32_to_cpu(d->dwPropertyDataType); > + if (type < USB_EXT_PROP_UNICODE || > + type > USB_EXT_PROP_UNICODE_MULTI) { > + pr_vdebug("unsupported os descriptor property type: %d", > + type); > + return -EINVAL; > + } > + pnl = le16_to_cpu(d->wPropertyNameLength); > + pdl = le32_to_cpu(*(u32 *)(data + 10 + pnl)); > + if (length != 14 + pnl + pdl) { > +#define FMT "invalid os descriptor length: %d pnl:%d pdl:%d (descriptor %d)\n" > + pr_vdebug(FMT, length, pnl, pdl, type); > +#undef FMT Uh? Why the #define? > + return -EINVAL; > + } > + ++ffs->ms_os_descs_ext_prop_count; > + ffs->ms_os_descs_ext_prop_name_len += (pnl << 1); Why is pnl multiplied by two? > + ffs->ms_os_descs_ext_prop_data_len += pdl; > + } > + break; > + default: > + pr_vdebug("unknown descriptor: %d\n", type); > + return -EINVAL; > + } > + return length; > +} > + > static int __ffs_data_got_descs(struct ffs_data *ffs, > char *const _data, size_t len) > { > char *data = _data, *raw_descs; > - unsigned counts[3], flags; > + unsigned os_descs_count = 0, counts[3], flags; > int ret = -EINVAL, i; > > ENTER(); > @@ -1877,7 +2064,8 @@ static int __ffs_data_got_descs(struct ffs_data *ffs, > flags = get_unaligned_le32(data + 8); > if (flags & ~(FUNCTIONFS_HAS_FS_DESC | > FUNCTIONFS_HAS_HS_DESC | > - FUNCTIONFS_HAS_SS_DESC)) { > + FUNCTIONFS_HAS_SS_DESC | > + FUNCTIONFS_HAS_MS_OS_DESC)) { > ret = -ENOSYS; > goto error; > } > @@ -1900,6 +2088,11 @@ static int __ffs_data_got_descs(struct ffs_data *ffs, > len -= 4; > } > } > + if (flags & (1 << i)) { > + os_descs_count = get_unaligned_le32(data); > + data += 4; > + len -= 4; > + }; > > /* Read descriptors */ > raw_descs = data; > @@ -1913,6 +2106,14 @@ static int __ffs_data_got_descs(struct ffs_data *ffs, > data += ret; > len -= ret; > } > + if (os_descs_count) { > + ret = ffs_do_os_descs(os_descs_count, data, len, > + __ffs_data_do_os_desc, ffs); > + if (ret < 0) > + goto error; > + data += ret; > + len -= ret; > + } > > if (raw_descs == data || len) { > ret = -EINVAL; > @@ -1925,6 +2126,7 @@ static int __ffs_data_got_descs(struct ffs_data *ffs, > ffs->fs_descs_count = counts[0]; > ffs->hs_descs_count = counts[1]; > ffs->ss_descs_count = counts[2]; > + ffs->ms_os_descs_count = os_descs_count; > > return 0; > > @@ -2091,6 +2293,7 @@ static void __ffs_event_add(struct ffs_data *ffs, > rem_type2 = FUNCTIONFS_SUSPEND; > /* FALL THROUGH */ > case FUNCTIONFS_SUSPEND: > + Unrelated and unnecessary. > case FUNCTIONFS_SETUP: > rem_type1 = type; > /* Discard all similar events */ > diff --git a/include/uapi/linux/usb/functionfs.h b/include/uapi/linux/usb/functionfs.h > index 2a4b4a7..4ec3798 100644 > --- a/include/uapi/linux/usb/functionfs.h > +++ b/include/uapi/linux/usb/functionfs.h > @@ -33,6 +32,42 @@ struct usb_endpoint_descriptor_no_audio { > __u8 bInterval; > } __attribute__((packed)); > > +/* MS OS Descriptor header */ > +struct usb_os_desc_header { > + __u8 interface; > + __u32 dwLength; > + __u16 bcdVersion; > + __u16 wIndex; Is that __u32 or __le32? And same with 16? You are accessing the data with le*_to_cpu so I assume this should read __le32 and __le16. Instead of having separate structures, how about adding this: union { struct { __u8 bCount; __u8 Reserved; } __attribute__((packed)); __le16 wCount; }; Feel free to name the union and struct if you are so inclined. > +} __attribute__((packed)); > + > +/* MS OS Extended Compatibility Descriptor header */ > +struct usb_ext_compat_desc_header { > + struct usb_os_desc_header header; > + __u8 bCount; > + __u8 Reserved; > +} __attribute__((packed)); > + > +struct usb_ext_compat_desc { > + __u8 bFirstInterfaceNumber; > + __u8 Reserved1; > + __u8 CompatibleID[8]; > + __u8 SubCompatibleID[8]; > + __u8 Reserved2[6]; > +}; > + > +/* MS OS Extended Properties Descriptor header */ > +struct usb_ext_prop_desc_header { > + struct usb_os_desc_header header; > + __u16 wCount; > +} __attribute__((packed)); > + > +struct usb_ext_prop_desc { > + __u32 dwSize; > + __u32 dwPropertyDataType; > + __u16 wPropertyNameLength; > +} __attribute__((packed)); > + > +#ifndef __KERNEL__ > > /* > * Descriptors format: -- Best regards, _ _ .o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o ..o | Computer Science, Michał “mina86” Nazarewicz (o o) ooo +--<mpn@xxxxxxxxxx>--<xmpp:mina86@xxxxxxxxxx>--ooO--(_)--Ooo-- -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
