| Message ID | 20101109073653.GF9036@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 11/09/2010 08:36 AM, Gleb Natapov wrote: > Properly check array bounds before accessing array element. > > Signed-off-by: Gleb Natapov<gleb@redhat.com> > diff --git a/hw/usb-net.c b/hw/usb-net.c > index 70f9263..84e2d79 100644 > --- a/hw/usb-net.c > +++ b/hw/usb-net.c > @@ -1142,7 +1142,7 @@ static int usb_net_handle_control(USBDevice *dev, int request, int value, > break; > > default: > - if (usb_net_stringtable[value& 0xff]) { > + if (ARRAY_SIZE(usb_net_stringtable)> (value& 0xff)) { > ret = set_usb_string(data, > usb_net_stringtable[value& 0xff]); > break; > -- > Gleb. > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Paolo
Gleb Natapov <gleb@redhat.com> writes: > Properly check array bounds before accessing array element. Impact? Apply to stable as well? > Signed-off-by: Gleb Natapov <gleb@redhat.com> > diff --git a/hw/usb-net.c b/hw/usb-net.c > index 70f9263..84e2d79 100644 > --- a/hw/usb-net.c > +++ b/hw/usb-net.c > @@ -1142,7 +1142,7 @@ static int usb_net_handle_control(USBDevice *dev, int request, int value, > break; > > default: > - if (usb_net_stringtable[value & 0xff]) { > + if (ARRAY_SIZE(usb_net_stringtable) > (value & 0xff)) { > ret = set_usb_string(data, > usb_net_stringtable[value & 0xff]); > break; Makes sense. Nitpick: LIMIT > INDEX looks unusual to me; INDEX < LIMIT is more common.
On Tue, Nov 09, 2010 at 10:30:54AM +0100, Markus Armbruster wrote: > Gleb Natapov <gleb@redhat.com> writes: > > > Properly check array bounds before accessing array element. > > Impact? > Gapping security hole for those unfortunate enough to use usb-net? > Apply to stable as well? > Definitely. Actually for me Windows7 crashed when usb-net is present. > > Signed-off-by: Gleb Natapov <gleb@redhat.com> > > diff --git a/hw/usb-net.c b/hw/usb-net.c > > index 70f9263..84e2d79 100644 > > --- a/hw/usb-net.c > > +++ b/hw/usb-net.c > > @@ -1142,7 +1142,7 @@ static int usb_net_handle_control(USBDevice *dev, int request, int value, > > break; > > > > default: > > - if (usb_net_stringtable[value & 0xff]) { > > + if (ARRAY_SIZE(usb_net_stringtable) > (value & 0xff)) { > > ret = set_usb_string(data, > > usb_net_stringtable[value & 0xff]); > > break; > > Makes sense. > > Nitpick: LIMIT > INDEX looks unusual to me; INDEX < LIMIT is more > common. -- Gleb.
Gleb Natapov <gleb@redhat.com> writes: > On Tue, Nov 09, 2010 at 10:30:54AM +0100, Markus Armbruster wrote: >> Gleb Natapov <gleb@redhat.com> writes: >> >> > Properly check array bounds before accessing array element. >> >> Impact? >> > Gapping security hole for those unfortunate enough to use usb-net? Doesn't that bit of information belong in the commit message. >> Apply to stable as well? >> > Definitely. Actually for me Windows7 crashed when usb-net is present. And that as well. Good catch!
On Tue, Nov 09, 2010 at 11:16:43AM +0100, Markus Armbruster wrote: > Gleb Natapov <gleb@redhat.com> writes: > > > On Tue, Nov 09, 2010 at 10:30:54AM +0100, Markus Armbruster wrote: > >> Gleb Natapov <gleb@redhat.com> writes: > >> > >> > Properly check array bounds before accessing array element. > >> > >> Impact? > >> > > Gapping security hole for those unfortunate enough to use usb-net? > > Doesn't that bit of information belong in the commit message. > Some people prefer not to put such information into commit message. > >> Apply to stable as well? > >> > > Definitely. Actually for me Windows7 crashed when usb-net is present. > > And that as well. > > Good catch! -- Gleb.
Gleb Natapov <gleb@redhat.com> writes: > On Tue, Nov 09, 2010 at 11:16:43AM +0100, Markus Armbruster wrote: >> Gleb Natapov <gleb@redhat.com> writes: >> >> > On Tue, Nov 09, 2010 at 10:30:54AM +0100, Markus Armbruster wrote: >> >> Gleb Natapov <gleb@redhat.com> writes: >> >> >> >> > Properly check array bounds before accessing array element. >> >> >> >> Impact? >> >> >> > Gapping security hole for those unfortunate enough to use usb-net? >> >> Doesn't that bit of information belong in the commit message. >> > Some people prefer not to put such information into commit message. Correct, but does "some people" include the QEMU maintainers? Anthony? [...]
On 11/09/2010 04:51 AM, Markus Armbruster wrote: > Gleb Natapov<gleb@redhat.com> writes: > > >> On Tue, Nov 09, 2010 at 11:16:43AM +0100, Markus Armbruster wrote: >> >>> Gleb Natapov<gleb@redhat.com> writes: >>> >>> >>>> On Tue, Nov 09, 2010 at 10:30:54AM +0100, Markus Armbruster wrote: >>>> >>>>> Gleb Natapov<gleb@redhat.com> writes: >>>>> >>>>> >>>>>> Properly check array bounds before accessing array element. >>>>>> >>>>> Impact? >>>>> >>>>> >>>> Gapping security hole for those unfortunate enough to use usb-net? >>>> >>> Doesn't that bit of information belong in the commit message. >>> >>> >> Some people prefer not to put such information into commit message. >> > Correct, but does "some people" include the QEMU maintainers? Anthony? > I don't have a strong opinion either way. If there's a CVE, I'd prefer the CVE number was prominent in the commit log but other than that, I'd leave it to the author's discretion. Regards, Anthony Liguori > [...] > > >
On Tue, Nov 16, 2010 at 02:08:13PM -0600, Anthony Liguori wrote: > On 11/09/2010 04:51 AM, Markus Armbruster wrote: > >Gleb Natapov<gleb@redhat.com> writes: > > > >>On Tue, Nov 09, 2010 at 11:16:43AM +0100, Markus Armbruster wrote: > >>>Gleb Natapov<gleb@redhat.com> writes: > >>> > >>>>On Tue, Nov 09, 2010 at 10:30:54AM +0100, Markus Armbruster wrote: > >>>>>Gleb Natapov<gleb@redhat.com> writes: > >>>>> > >>>>>>Properly check array bounds before accessing array element. > >>>>>Impact? > >>>>> > >>>>Gapping security hole for those unfortunate enough to use usb-net? > >>>Doesn't that bit of information belong in the commit message. > >>> > >>Some people prefer not to put such information into commit message. > >Correct, but does "some people" include the QEMU maintainers? Anthony? > > I don't have a strong opinion either way. If there's a CVE, I'd > prefer the CVE number was prominent in the commit log but other than > that, I'd leave it to the author's discretion. > No CVE. Please apply as is. -- Gleb.
On 11/09/2010 01:36 AM, Gleb Natapov wrote: > Properly check array bounds before accessing array element. > > Signed-off-by: Gleb Natapov<gleb@redhat.com> > Applied. Thanks. Regards, Anthony Liguori > diff --git a/hw/usb-net.c b/hw/usb-net.c > index 70f9263..84e2d79 100644 > --- a/hw/usb-net.c > +++ b/hw/usb-net.c > @@ -1142,7 +1142,7 @@ static int usb_net_handle_control(USBDevice *dev, int request, int value, > break; > > default: > - if (usb_net_stringtable[value& 0xff]) { > + if (ARRAY_SIZE(usb_net_stringtable)> (value& 0xff)) { > ret = set_usb_string(data, > usb_net_stringtable[value& 0xff]); > break; > -- > Gleb. > > >
diff --git a/hw/usb-net.c b/hw/usb-net.c index 70f9263..84e2d79 100644 --- a/hw/usb-net.c +++ b/hw/usb-net.c @@ -1142,7 +1142,7 @@ static int usb_net_handle_control(USBDevice *dev, int request, int value, break; default: - if (usb_net_stringtable[value & 0xff]) { + if (ARRAY_SIZE(usb_net_stringtable) > (value & 0xff)) { ret = set_usb_string(data, usb_net_stringtable[value & 0xff]); break;
Properly check array bounds before accessing array element. Signed-off-by: Gleb Natapov <gleb@redhat.com> -- Gleb.