On Thu, 28 Feb 2019 at 17:57, Greg Kurz <gr...@kaod.org> 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 <gr...@kaod.org> > --- > 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 ? 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 ? 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. 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. thanks -- PMM