Patchwork Out off array access in usb-net

login
register
mail settings
Submitter Gleb Natapov
Date Nov. 9, 2010, 7:36 a.m.
Message ID <20101109073653.GF9036@redhat.com>
Download mbox | patch
Permalink /patch/70498/
State New
Headers show

Comments

Gleb Natapov - Nov. 9, 2010, 7:36 a.m.
Properly check array bounds before accessing array element.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
--
			Gleb.
Paolo Bonzini - Nov. 9, 2010, 9:29 a.m.
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
Markus Armbruster - Nov. 9, 2010, 9:30 a.m.
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.
Gleb Natapov - Nov. 9, 2010, 9:39 a.m.
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.
Markus Armbruster - Nov. 9, 2010, 10:16 a.m.
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!
Gleb Natapov - Nov. 9, 2010, 10:34 a.m.
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.
Markus Armbruster - Nov. 9, 2010, 10:51 a.m.
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?

[...]
Anthony Liguori - Nov. 16, 2010, 8:08 p.m.
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

> [...]
>
>
>
Gleb Natapov - Nov. 16, 2010, 8:14 p.m.
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.
Anthony Liguori - Nov. 16, 2010, 10:24 p.m.
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.
>
>
>

Patch

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;