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

[Greg Kurz]

On Thu, 28 Feb 2019 18:28:23 +0000
Peter Maydell <address@hidden> wrote:

> On Thu, 28 Feb 2019 at 17:57, Greg Kurz <address@hidden> wrote:
> >
> > Build fails with gcc 9:
> >
> >   CC      hw/usb/dev-mtp.o
> > hw/usb/dev-mtp.c: In function ‘usb_mtp_write_metadata’:
> > hw/usb/dev-mtp.c:1754:36: error: taking address of packed member of ‘struct 
> > <anonymous>’ may result in an unaligned pointer value 
> > [-Werror=address-of-packed-member]
> >  1754 |                             dataset->filename);
> >       |                             ~~~~~~~^~~~~~~~~~
> > cc1: all warnings being treated as errors
> >
> > The way the MTP protocol encodes strings with a leading 8-bit unsigned
> > integer containing the string length causes the @filename field of the
> > packed ObjectInfo structure to be unaligned.
> >
> > Use a temporary buffer for the utf16 filename instead of passing the
> > dataset->filename pointer around.
> >
> > Signed-off-by: Greg Kurz <address@hidden>
> > ---
> >  hw/usb/dev-mtp.c |   18 +++++++++++++++---
> >  1 file changed, 15 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
> > index 4ee4fc5a893a..8a0ef7f9f4a8 100644
> > --- a/hw/usb/dev-mtp.c
> > +++ b/hw/usb/dev-mtp.c
> > @@ -1692,13 +1692,25 @@ static void usb_mtp_write_metadata(MTPState *s, 
> > uint64_t dlen)
> >      MTPObject *o;
> >      MTPObject *p = usb_mtp_object_lookup(s, s->dataset.parent_handle);
> >      uint32_t next_handle = s->next_handle;
> > +    uint16_t *utf16_filename;
> > +    uint8_t filename_len;
> >
> >      assert(!s->write_pending);
> >      assert(p != NULL);
> >
> > -    filename = utf16_to_str(MIN(dataset->length,
> > -                                dlen - offsetof(ObjectInfo, filename)),
> > -                            dataset->filename);
> > +    /*
> > +     * MTP Specification 3.2.3 Strings
> > +     * Strings in PTP (and thus MTP) consist of standard 2-byte Unicode
> > +     * characters as defined by ISO 10646. Strings begin with a single, 
> > 8-bit
> > +     * unsigned integer that identifies the number of characters to follow 
> > (not
> > +     * bytes).
> > +     *
> > +     * This causes dataset->filename to be unaligned.
> > +     */
> > +    filename_len = MIN(dataset->length, dlen - offsetof(ObjectInfo, 
> > filename));
> > +    utf16_filename = g_memdup(dataset->filename, filename_len * 2);
> > +    filename = utf16_to_str(filename_len, utf16_filename);
> > +    g_free(utf16_filename);  
> 
> 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... :-\

> 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