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

[Peter Maydell]

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 ? 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