On Mon, Jul 07 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>
> ---
> drivers/usb/gadget/function/f_fs.c | 346 +++++++++++++++++++++++++++++++++++-
> drivers/usb/gadget/function/u_fs.h | 7 +
> include/uapi/linux/usb/functionfs.h | 81 ++++++++-
> 3 files changed, 427 insertions(+), 7 deletions(-)
> @@ -1856,11 +1864,192 @@ 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_single_os_desc(char *data, unsigned len,
> + u8 *valuep, u16 feature_count,
Why is valuep pointer to u8? Why is it a pointer? The function is used
in one place only to simply pass value of enum ffs_os_desc_type so this
argument can be easily changed to “enum ffs_os_desc_type type”.
> + ffs_os_desc_callback entity,
> + void *priv,
> + struct usb_os_desc_header *desc)
> +{
> + int ret;
> + 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, desc, data, priv);
You need to pass length to entity callback as well, and check it there.
Otherwise it could go over the available data. Unless you check the
length here outside of the callback.
> + if (unlikely(ret < 0)) {
> + pr_debug("bad OS descriptor, type: %d\n", type);
> + 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 (num = 0; num < count; ++num) {
> + 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;
> +
> + /* 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
> + */
This wrapping around 60th column looks a bit weird here.
> + ret = entity(FFS_OS_DESC, &type, desc, priv);
> + if (unlikely(ret < 0)) {
> + pr_debug("entity OS_DESCRIPTOR(%02lx); ret = %d\n",
> + num, ret);
> + return ret;
> + }
> + /*
> + * 16-bit hex "?? 00" Little Endian looks like 8-bit hex "??"
> + */
> + feature_count = le16_to_cpu(desc->wCount);
> + if (type == FFS_OS_DESC_EXT_COMPAT &&
> + (feature_count > 255 || desc->Reserved))
> + return -EINVAL;
> + len -= ret;
> + data += ret;
> +
> + /*
> + * Process all function/property descriptors
> + * of this Feature Descriptor
> + */
> + ret = ffs_do_single_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;
> + }
> + return _len - len;
> +
Useless empty line.
> +}
> +
> +/**
> + * Validate contents of the buffer from userspace related to OS descriptors.
> + */
> +static int __ffs_data_do_os_desc(enum ffs_os_desc_type type,
> + void *valuep, void *data, void *priv)
> +{
> + 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;
You don't need the cast.
> + __u16 bcd_version = le16_to_cpu(desc->bcdVersion);
> + __u16 w_index = le16_to_cpu(desc->wIndex);
You want u16. __u16 is for use in header files that are read by user
space. And since each of those variables is used only once, you could
get away with only a single temporary variable, but that's up to you.
> +
> + if (bcd_version != 1) {
> + pr_vdebug("unsupported os descriptors version: %d",
> + bcd_version);
> + return -EINVAL;
> + }
> + switch (w_index) {
> + case 0x4:
> + *next_type = FFS_OS_DESC_EXT_COMPAT;
> + break;
> + case 0x5:
> + *next_type = FFS_OS_DESC_EXT_PROP;
> + break;
> + default:
> + pr_vdebug("unsupported os descriptor type: %d",
> + w_index);
> + return -EINVAL;
> + }
> + length = sizeof(*desc);
> + }
> + break;
> + case FFS_OS_DESC_EXT_COMPAT: {
> + struct usb_ext_compat_desc *d = data;
> + int i;
> +
> + if (d->bFirstInterfaceNumber >= ffs->interfaces_count ||
> + d->Reserved1)
> + return -EINVAL;
> + for (i = 0; i < 6; ++i)
for (i = 0; i < ARRAY_SIZE(d->Reserved2); ++i)
> + if (d->Reserved2[i])
> + return -EINVAL;
> +
> + 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;
No need to cast.
> + struct usb_ext_prop_desc *d = data;
> + u32 type, pdl;
> + u16 pnl;
> +
> + 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) {
> + pr_vdebug("invalid os descriptor length: %d pnl:%d pdl:%d (descriptor %d)\n",
> + length, pnl, pdl, type);
> + return -EINVAL;
> + }
> + ++ffs->ms_os_descs_ext_prop_count;
> + /* property name reported to the host as "WCHAR"s */
> + ffs->ms_os_descs_ext_prop_name_len += pnl * 2;
> + 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();
> @@ -2267,6 +2471,88 @@ static int __ffs_func_bind_do_nums(enum ffs_entity_type type, u8 *valuep,
> return 0;
> }
>
> +static int __ffs_func_bind_do_os_desc(enum ffs_os_desc_type type,
> + void *valuep, void *data, void *priv)
> +{
> + struct ffs_function *func = priv;
> + __u8 length = 0;
> +
> + 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;
Cast is not needed.
> + __u16 w_index = le16_to_cpu(desc->wIndex);
> +
> + 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);
> + }
So in both callbacks, the FFS_OS_DESC case does exactly the same thing.
Perhaps it's better to move it out of the callback and into a separate
function? I think it would also make the prototype of the callback
saner as it would not have to cover both types of call.
> + break;
> + case FFS_OS_DESC_EXT_COMPAT: {
> + struct usb_ext_compat_desc *desc = data;
> + struct usb_os_desc_table *t;
> +
> + t = &func->function.os_desc_table[desc->bFirstInterfaceNumber];
> + t->if_id = func->interfaces_nums[desc->bFirstInterfaceNumber];
> + memcpy(t->os_desc->ext_compat_id, (void *)desc + 2, 16);
Uhm, no. Arithmetic on void pointer is undefined. And since desc is
a structure, you actually just want to take address of the field in it:
/* Copy CompatibleID and SubCompatibleID in one go, hence 16. */
memcpy(t->os_desc->ext_compat_id, &desc->CompatibleID, 16);
or even, if you feel particularly verbose:
memcpy(t->os_desc->ext_compat_id, &desc->CompatibleID,
ARRAY_SIZE(desc->CompatibleID) +
ARRAY_SIZE(desc->SubCompatibleID));
> + length = sizeof(*desc);
> + }
> + break;
> + case FFS_OS_DESC_EXT_PROP: {
> + struct usb_os_desc_header *h =
> + (struct usb_os_desc_header *)valuep;
Unnecessary cast.
> + struct usb_ext_prop_desc *desc = data;
> + struct usb_os_desc_table *t;
> + struct usb_os_desc_ext_prop *ext_prop;
> + char *ext_prop_name;
> + char *ext_prop_data;
> +
> + t = &func->function.os_desc_table[h->interface];
> + t->if_id = func->interfaces_nums[h->interface];
> +
> + ext_prop = func->ffs->ms_os_descs_ext_prop_avail;
> + func->ffs->ms_os_descs_ext_prop_avail += sizeof(*ext_prop);
> +
> + ext_prop->type = le32_to_cpu(desc->dwPropertyDataType);
> + ext_prop->name_len = le16_to_cpu(desc->wPropertyNameLength);
> + ext_prop->data_len =
> + le32_to_cpu(*(u32 *)(data + 10 + ext_prop->name_len));
Again, arithmetic on void * is undefined (or unspecified or something).
You need to cast to u8* or char* first. Applies throughout the code.
> + length = ext_prop->name_len + ext_prop->data_len + 14;
> +
> + ext_prop_name = func->ffs->ms_os_descs_ext_prop_name_avail;
> + func->ffs->ms_os_descs_ext_prop_name_avail +=
> + ext_prop->name_len;
> +
> + ext_prop_data = func->ffs->ms_os_descs_ext_prop_data_avail;
> + func->ffs->ms_os_descs_ext_prop_data_avail +=
> + ext_prop->data_len;
> + memcpy(ext_prop_data, data + 14 + ext_prop->name_len,
> + ext_prop->data_len);
> + ext_prop->data_len <<= 1;
> + ext_prop->data = ext_prop_data;
> +
> + memcpy(ext_prop_name, data + 10, ext_prop->name_len);
> + ext_prop->name_len <<= 1;
> + ext_prop->name = ext_prop_name;
> +
> + t->os_desc->ext_prop_len +=
> + (ext_prop->name_len + ext_prop->data_len + 14);
> + ++t->os_desc->ext_prop_count;
> + list_add_tail(&ext_prop->entry, &t->os_desc->ext_prop);
> + }
> + break;
> + }
> +
> + return length;
> +}
> +
> static inline struct f_fs_opts *ffs_do_functionfs_bind(struct usb_function *f,
> struct usb_configuration *c)
> {
> @@ -2354,8 +2652,20 @@ static int _ffs_func_bind(struct usb_configuration *c,
> if (unlikely(!vlabuf))
> return -ENOMEM;
>
> + ffs->ms_os_descs_ext_prop_avail = vla_ptr(vlabuf, d, ext_prop);
> + ffs->ms_os_descs_ext_prop_name_avail =
> + vla_ptr(vlabuf, d, ext_prop_name);
> + ffs->ms_os_descs_ext_prop_data_avail =
> + vla_ptr(vlabuf, d, ext_prop_data);
> +
> /* Zero */
> memset(vla_ptr(vlabuf, d, eps), 0, d_eps__sz);
> + memset(vla_ptr(vlabuf, d, os_desc_table), 0, d_os_desc_table__sz);
> + memset(vla_ptr(vlabuf, d, ext_compat), 0, d_ext_compat__sz);
> + memset(vla_ptr(vlabuf, d, os_desc), 0, d_os_desc__sz);
> + memset(vla_ptr(vlabuf, d, ext_prop), 0, d_ext_prop__sz);
> + memset(vla_ptr(vlabuf, d, ext_prop_name), 0, d_ext_prop_name__sz);
> + memset(vla_ptr(vlabuf, d, ext_prop_data), 0, d_ext_prop_data__sz);
Just change allocation of vlabuf to use kzalloc because this is getting
ridiculous.
> /* Copy descriptors */
> memcpy(vla_ptr(vlabuf, d, raw_descs), ffs->raw_descs,
> ffs->raw_descs_length);
> @@ -2430,6 +2744,28 @@ static int _ffs_func_bind(struct usb_configuration *c,
> if (unlikely(ret < 0))
> goto error;
>
> + func->function.os_desc_table = vla_ptr(vlabuf, d, os_desc_table);
> + if (c->cdev->use_os_string)
> + for (i = 0; i < ffs->interfaces_count; ++i) {
> + struct usb_os_desc *desc;
> +
> + desc = func->function.os_desc_table[i].os_desc =
> + vla_ptr(vlabuf, d, os_desc) +
> + i * sizeof(struct usb_os_desc);
> + desc->ext_compat_id =
> + vla_ptr(vlabuf, d, ext_compat) + i * 16;
> + INIT_LIST_HEAD(&desc->ext_prop);
> + }
> + ret = ffs_do_os_descs(ffs->ms_os_descs_count,
> + vla_ptr(vlabuf, d, raw_descs) +
> + fs_len + hs_len + ss_len,
> + d_raw_descs__sz - fs_len - hs_len - ss_len,
> + __ffs_func_bind_do_os_desc, func);
> + func->function.os_desc_n =
> + c->cdev->use_os_string ? ffs->interfaces_count : 0;
> + if (unlikely(ret < 0))
> + goto error;
We might just as well check for this error condition one statement
earlier, no?
> +
> /* And we're done */
> ffs_event_add(ffs, FUNCTIONFS_BIND);
> return 0;
--
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
