Hi Andrzej,
On 2023/02/22 20:08, Andrzej Pietrasiewicz wrote:
> 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.
Yes, I think you are correct that each field in the (OS) descriptor must be in
little-endian order.
However, the current code does not seem to be consistent with the part that
verifies bcd_version and the part that verifies w_index. Below are the results
of my experiments on my ARM v7 machine (AM335X).
On the userspace driver, set the values to the descriptor as follows:
#include <endian.h>
...
struct usb_os_desc_header ext_compat_hdr = {};
...
ext_compat_hdr.bcdVersion = htole16(0x0001);
ext_compat_hdr.wIndex = htole16(0x0004);
...
In the Extended Compat ID Descriptor descriptor, the correct value for
bcdVersion is probably 0x100 in little-endian format, and the correct value for
wIndex is 0x4 in little-endian format. Note that the current kernel (without my
patch) only accepts a value of 0x1 for bcdVersion, so the above is the case.
On the other hand, the code to verify bcdVersion and wIndex in the kernel is as
follows:
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",
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;
}
When setting a value to a structure in the userspace driver, cpu_to_leXX() is
used, and when checking the value in the kernel, leXX_to_cpu() is used after
taking the value out of the structure. Therefore, I do not think we need to
worry about endianness at the time of verification. Actually, the w_index
verification, which expects a value of 0x4 or 0x5, seems to be working
correctly.
As I stated in my email below, I am confident that 0x100 is the correct value
for bcd_version. If this is correct, shouldn't the kernel accept the value
0x100 obtained by leXX_to_cpu()?
https://lore.kernel.org/linux-usb/5c049a94-f848-ff9a-ffbe-c1cf335ca644@xxxxxxxxxxx/
> 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.
Sorry, I don't have a big-endian machine, so it will take some time to prepare
an environment for checking.
The procedure is to do cpu_to_leXX() on the user space driver and then
leXX_to_cpu() in the kernel, so I think it will work no matter what the
endianness of the machine is....
Regards,
Yuta Hayama
