On Wed, Sep 07, 2016 at 04:09:36PM -0700, Kees Cook wrote:
> On Thu, Sep 1, 2016 at 11:07 PM, Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > On Fri, Sep 02, 2016 at 01:20:21AM +0100, James wrote:
> >> This patch fixes a NULL pointer dereference caused by a race codition in the probe
> >> function of the legousbtower driver. The probe function does not deregister the
> >> usb interface after an error receiving the devices firmware ID. The device file
> >> registered (/dev/usb/legousbtower%d) may be read/written globally before the probe
> >> function returns; then when tower_delete is called in the probe function (after an
> >> r/w has been initiated), core dev structures are deleted while the file operation
> >> functions are still running. If the 0 address is mappable on the machine, this
> >> vulnerability can be used to create a Local Priviege Escalation exploit via a
> >> write-what-where condition by remapping dev->interrupt_out_buffer in tower_write.
> >> A forged USB device and local program execution would be required for LPE.
> >> The USB device would have to delay the control message in tower_probe and
> >> accept the control urb in tower_open whilst guest code initiated a write to
> >> the device file as tower_delete is called from the error in tower_probe.
> >
> > How about this simpler patch, that moves the registering for the minor
> > number as the last thing that happens in the probe function (which is
> > what it should be).
> >
> > Can you test this to verify it prevents the above problem?
> >
> > And this driver really needs an overhaul one of these days...
> >
> > thanks,
> >
> > greg k-h
> >
> > diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
> > index 7771be3ac178..4dd531ac5a7f 100644
> > --- a/drivers/usb/misc/legousbtower.c
> > +++ b/drivers/usb/misc/legousbtower.c
> > @@ -898,24 +898,6 @@ static int tower_probe (struct usb_interface *interface, const struct usb_device
> > dev->interrupt_in_interval = interrupt_in_interval ? interrupt_in_interval : dev->interrupt_in_endpoint->bInterval;
> > dev->interrupt_out_interval = interrupt_out_interval ? interrupt_out_interval : dev->interrupt_out_endpoint->bInterval;
> >
> > - /* we can register the device now, as it is ready */
> > - usb_set_intfdata (interface, dev);
> > -
> > - retval = usb_register_dev (interface, &tower_class);
> > -
> > - if (retval) {
> > - /* something prevented us from registering this driver */
> > - dev_err(idev, "Not able to get a minor for this device.\n");
> > - usb_set_intfdata (interface, NULL);
> > - goto error;
> > - }
> > - dev->minor = interface->minor;
> > -
> > - /* let the user know what node this device is now attached to */
> > - dev_info(&interface->dev, "LEGO USB Tower #%d now attached to major "
> > - "%d minor %d\n", (dev->minor - LEGO_USB_TOWER_MINOR_BASE),
> > - USB_MAJOR, dev->minor);
> > -
> > /* get the firmware version and log it */
> > result = usb_control_msg (udev,
> > usb_rcvctrlpipe(udev, 0),
> > @@ -936,6 +918,23 @@ static int tower_probe (struct usb_interface *interface, const struct usb_device
> > get_version_reply.minor,
> > le16_to_cpu(get_version_reply.build_no));
> >
> > + /* we can register the device now, as it is ready */
> > + usb_set_intfdata (interface, dev);
> > +
> > + retval = usb_register_dev (interface, &tower_class);
> > +
> > + if (retval) {
> > + /* something prevented us from registering this driver */
> > + dev_err(idev, "Not able to get a minor for this device.\n");
> > + usb_set_intfdata (interface, NULL);
> > + goto error;
> > + }
> > + dev->minor = interface->minor;
> > +
> > + /* let the user know what node this device is now attached to */
> > + dev_info(&interface->dev, "LEGO USB Tower #%d now attached to major "
> > + "%d minor %d\n", (dev->minor - LEGO_USB_TOWER_MINOR_BASE),
> > + USB_MAJOR, dev->minor);
> >
> > exit:
> > return retval;
> >
>
> Did this get any testing?
I have not heard anything back from the original poster :(
greg k-h
--
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
