Re: [Qemu-devel] [PATCH] usb-mtp: Fix build with gcc 9

[Bandan Das <bsd@redhat.com>]

Greg Kurz <groug@kaod.org> writes:
...
>> 
>> I think there's an underlying problem with this code which we
>> should deal with differently. The 'dataset' local in this
>> file is (I think) pointing at on-the-wire information from
>> the USB device, but we're treating it as an array of
>> host-order uint16_t values. Is this really correct on a
>> big-endian host ?
>
> I don't know much about usb-mtp and the MTP spec says:
>
> https://theta360blog.files.wordpress.com/2016/04/mtpforusb-ifv1-1.pdf
>
> 3.1.1 Multi-byte Data
>
> The standard format for multi-byte data in this specification is
> big-endian. That is, the bits within a byte will be read such that
> the most significant byte is read first. The actual multi-byte data
> sent over the transport may not necessarily adhere to this same
> format, and the actual multi-byte data used on the devices may also
> use a different multi-byte format. The big-endian convention only
> applies within this document, except where otherwise stated.
>
> So I'm not sure about what the code should really do here... :-\
>

If I remember correctly, with USB transport, multibyte values are
little endian and it supersedes the MTP spec? (which is why the code works
as expected on a little endian host). As Peter said, some byte swapping
is probably needed for this to work on big endian hosts.

>> Do we do the right thing if we are
>> passed a malicious USB packet that ends halfway through a
>> utf16_t character, or do we index off the end of the packet
>> data ?
>> 
>
> Can you elaborate ?
>
>> I think that we should define the "filename" field in
>> ObjectInfo to be a uint8_t array, make utf16_to_str()
>> take a uint8_t* for its data array, and have it do the
>> reading of data from the array with lduw_he_p(), which
>> can handle accessing unaligned data.
>> 
>> We should also check what the endianness of other fields in
>> the ObjectInfo struct is (eg "format" and "size" and see
>> whether we should be doing byte swapping here.
>> 
>
> I don't have any idea on that... the code just seems to assume
> everything is host endian.
>
>> PS: it is a bit confusing that in this function the local
>> variable "dataset" is a pointer to a struct of entirely
>> different type to the one that s->dataset is.
>> 
>
> Maybe Gerd or Bandan can comment on that.
>
>> thanks
>> -- PMM