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

[Greg Kurz <groug@kaod.org>]

On Fri, 1 Mar 2019 15:15:43 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> On Fri, 1 Mar 2019 at 14:59, Greg Kurz <groug@kaod.org> wrote:
> >
> > On Thu, 28 Feb 2019 18:28:23 +0000
> > Peter Maydell <peter.maydell@linaro.org> wrote:
> >  
> > > On Thu, 28 Feb 2019 at 17:57, Greg Kurz <groug@kaod.org> 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.
> 

Oh you're right, this should rather be:

    filename_len = MIN(dataset->length * 2, dlen - offsetof(ObjectInfo, filename)) / 2;

So that if length is say 20 (ie, 40 bytes) but the buffer only has say 39 bytes
for the filename, we clip at 19 characters.

Since the current code already has the issue, I guess this should be fixed
in its own patch.

> thanks
> -- PMM