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

[Peter Maydell]

On Fri, 1 Mar 2019 at 14:59, Greg Kurz <address@hidden> wrote:
>
> 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:
> > > +    /*
> > > +     * 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);

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

If the data packet length is such that the packet stops
one byte into a two-byte character in the filename,
do we try to read the two bytes that straddle
the end of the buffer ?

In fact this expression looks bogus:
 filename_len = MIN(dataset->length, dlen - offsetof(ObjectInfo, filename));

because dataset->length (as the comment says) is a
count of 2-byte characters, but the calculation involving
offsetof() is a byte count. So we don't correctly clip
the character count and utf16_to_str() will happily
read off the end of the buffer it is passed.

thanks
-- PMM