Hi Yuta Hayama,
W dniu 22.02.2023 o 10:02, Yuta Hayama pisze:
Currently, the USB gadget framework supports only version 1.0 of the MS OS
descriptor. OS desc has a field bcdVersion indicating its version, with
v1.0 represented by the value 0x100. However, __ffs_do_os_desc_header()
was expecting the incorrect value 0x1, so allow the correct value 0x100.
The bcdVersion field of the descriptor that is actually sent to the host
is set by composite_setup() (in composite.c) to the fixed value 0x100.
Therefore, it can be understood that __ffs_do_os_desc_header() is only
performing a format check of the OS desc passed to functionfs. If a value
other than 0x100 is accepted, there is no effect on communication over
the USB bus. Indeed, until now __ffs_do_os_desc_header() has only accepted
the incorrect value 0x1, but there was no problem with the communication
over the USB bus.
However, this can be confusing for functionfs userspace drivers. Since
bcdVersion=0x100 is used in actual communication, functionfs should accept
the value 0x100.
Note that the correct value for bcdVersion in OS desc v1.0 is 0x100, but
to avoid breaking old userspace drivers, the value 0x1 is also accepted as
an exception. At this time, a message is output to notify the user to fix
the userspace driver.
Now that I think of it, aren't the numbers supposed to be in little endian
order?
In include/uapi/linux/usb/functionfs.h, descriptors and legacy descriptors
specify numbers as LE32, and below there is this sentence:
"All numbers must be in little endian order.".
And that is regardless of what endianness the machine running the gadget
uses. In other words, in the buffer passed through ep0 any numbers shall be
stored in such a way, that the least significant byte goes first.
The tables specifying the OS descriptors use general U16/U32 type, though,
but struct usb_os_desc_header says:
/* MS OS Descriptor header */
struct usb_os_desc_header {
__u8 interface;
__le32 dwLength;
__le16 bcdVersion;
...
which is a strong hint that, in particular, bcdVersion _is_ little endian.
And a two-byte quantity of 0x0100 stored in little endian format looks like
this: 0x0001.
So it seems to me the correct thing to do is to ensure that the verifying code
works correctly both on big and little endian machines running USB gadgets,
rather than adding 0x0100 as a correct value and deprecating 0x0001.
Regards,
Andrzej
Signed-off-by: Yuta Hayama <hayama@xxxxxxxxxxx>
---
I am currently considering the patch approach itself. Please see below.
https://lore.kernel.org/linux-usb/80754db0-a3bb-ee5e-db39-b75b8ebbd30e@xxxxxxxxxxx/
Changelog
v2:
- remove comma inserted incorrectly in pr_vdebug()
- if bcd_version == 0x1, use pr_warn() instead of pr_vdebug()
drivers/usb/gadget/function/f_fs.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 8ad354741380..05f83469956a 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -2292,8 +2292,12 @@ static int __ffs_do_os_desc_header(enum ffs_os_desc_type *next_type,
u16 bcd_version = le16_to_cpu(desc->bcdVersion);
u16 w_index = le16_to_cpu(desc->wIndex);
- if (bcd_version != 1) {
- pr_vdebug("unsupported os descriptors version: %d",
+ if (bcd_version == 0x1) {
+ pr_warn("bcdVersion is expected to be 0x100, but it was 0x%x. "
+ "Pass for compatibility, but fix your user space driver.\n",
+ bcd_version);
+ } else if (bcd_version != 0x100) {
+ pr_vdebug("unsupported os descriptors version: 0x%x\n",
bcd_version);
return -EINVAL;
}
