G wrote:
> On Tue, Jul 21, 2009 at 1:23 AM, Jim Paris<j...@jtan.com> wrote:
> > Here's a patch to try. I'm not familiar with the code, but it looks
> > like this buffer might be too small versus the packet lengths that
> > you're seeing, and similar definitions in hw/usb-uhci.c.
> >
> > -jim
> >
> > diff -urN kvm-87-orig/usb-linux.c kvm-87/usb-linux.c
> > --- kvm-87-orig/usb-linux.c 2009-06-23 09:32:38.000000000 -0400
> > +++ kvm-87/usb-linux.c 2009-07-20 19:15:35.000000000 -0400
> > @@ -115,7 +115,7 @@
> > uint16_t offset;
> > uint8_t state;
> > struct usb_ctrlrequest req;
> > - uint8_t buffer[1024];
> > + uint8_t buffer[2048];
> > };
> >
> > typedef struct USBHostDevice {
>
> Yes! Applying this patch makes the crash go away! Thank you!
Great!
> In addition to enabling DEBUG and applying your debug printout
> patches, I added a debug printout right above the memcpy()s in
> usb-linux.c, and found that the memcpy() in do_token_in() is called
> multiple time (since do_token_in() is called multiple times for the
> 1993 bytes usb packet I have in my usb sniff dumps), which I guess is
> what's causing a buffer overflow as the offset is pushed beyond 1024
> bytes. But I'm not sure.
Yeah, I think that's it.
> I've looked at the code trying to figure out a better way to solve
> this, now that the problem spot has been found. To me it seems that
> malloc()ing and, when the need arises (the large 1993 bytes packets
> I'm seeing), realloc()ing the buffer, instead of using a statically
> sized buffer, would be the best solution.
Dynamically sizing the buffer might get tricky. It looks like
hw/usb-uhci.c will go up to 2048, while hw/usb-ohci.c and
hw/usb-musb.c could potentially go up to 8192. I think bumping it to
8192 and adding an error instead of overflowing would be good enough.
I'll try to understand the code a bit more and then spin a patch.
> One could of course redefine all buffers to be 8192 bytes instead,
> but that would just be a false sense of security, and perhaps some
> buffers need to be of a particular size to conform to the USB
> specification...
USB packets don't get that large, but the host controllers can combine
them, from what I understand. So it's more a question of what the
host controllers can do.
-jim
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
