| Message ID | 20090720232310.GA29764@psychosis.jim.sh (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 21, 2009 at 1:23 AM, Jim Paris<jim@jtan.com> wrote: > G wrote: >> And thanks for your help and suggestions so far, btw. > > 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! 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. 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. However, I cannot find a suitable place to do this, so in the meantime I'll use your patch, although I do hope the kvm developers will implement a more stable/reliable malloc()/realloc() solution in the future. 1993 bytes isn't far from the 2048 bytes limit, and it seems to me that there are more places in the usb code where statically sized buffer are used which could lead to more problems of this kind. 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... The differences between the usb code in kvm-72 (which works without a patch) and kvm-87 are too big for me to try to find out why it works in kvm-72. Anyways, I'm happy. Once again, thanks. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
G wrote: > On Tue, Jul 21, 2009 at 1:23 AM, Jim Paris<jim@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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 {