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?
-Kees
--
Kees Cook
Nexus Security
--
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
