On Sun, 2 May 2010, Michael Buesch wrote:
> On Sunday 02 May 2010 21:24:49 Alan Stern wrote:
> > On Sun, 2 May 2010, Michael Buesch wrote:
> >
> > > This fixes a NULL pointer dereference triggered by an off-by-one
> > > error, if the USB_REQ_SET_CONFIGURATION request to the device in
> > > usb_reset_configuration() fails.
> > >
> > > Signed-off-by: Michael Buesch <mb@xxxxxxxxx>
> > > Cc: stable@xxxxxxxxxx
> > >
> > > ---
> > >
> > > Alan, this fixes the crash.
> >
> > Can you explain why? I don't see any off-by-one errors.
> >
> > > Index: linux-2.6.33/drivers/usb/core/message.c
> > > ===================================================================
> > > --- linux-2.6.33.orig/drivers/usb/core/message.c 2010-05-02 19:41:58.000000000 +0200
> > > +++ linux-2.6.33/drivers/usb/core/message.c 2010-05-02 19:42:46.000000000 +0200
> > > @@ -1489,8 +1489,10 @@ reset_old_alts:
> > > USB_REQ_SET_CONFIGURATION, 0,
> > > config->desc.bConfigurationValue, 0,
> > > NULL, 0, USB_CTRL_SET_TIMEOUT);
> > > - if (retval < 0)
> > > + if (retval < 0) {
> > > + i--;
> > > goto reset_old_alts;
> > > + }
> >
> > This extra decrement should not be needed, because the "for" loop at
> > reset_old_alts starts out by decrementing i.
>
> No it does not. The initialization area of the for loop is empty.
> Which is correct for the case where the loop is entered through the
> first if (retval < 0) without a goto. Because in that case the previous
> for-loop was terminated with a break.
No, it's wrong in that case as well. It will attempt to deallocate
bandwidth that didn't get allocated previously.
> However for the second case, the first
> for loop will have looped through all of its elements. Leaving i at bNumInterfaces.
> So just before goto reset_old_alts i is bNumInterfaces. Which is an invalid index
> as it is one beyond the array.
Ah, I see the problem. The bug has already been fixed in 2.6.34-rc by:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=e4a3d94658b5760fc947d7f7185c57db47ca362a
Evidently this patch needs to be applied to the stable trees. Sarah,
this would explain why other people using 2.6.33 observe the same bug.
Greg, can you take care of this?
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
