[PATCH] usb: misc: legousbtower: Fix NULL pointer deference

[PATCH] usb: misc: legousbtower: Fix NULL pointer deference

[James]

[-- Attachment #1: Type: text/plain, Size: 3896 bytes --]

This patch fixes a NULL pointer dereference caused by a race codition 
in the probe function of the legousbtower driver. It re-structures the 
probe function to only register the interface after successfully 
reading the board's firmware ID.

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. 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.

This bug has existed since 2003. Patch tested by emulated device.

Signed-off-by: James Patrick-Evans <james@jmp-e.com>
---
 drivers/usb/misc/legousbtower.c | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 7771be3..4dd531a 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;
--


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] usb: misc: legousbtower: Fix NULL pointer deference

[Greg KH]

On Mon, Sep 19, 2016 at 07:09:51PM +0100, James wrote:
> This patch fixes a NULL pointer dereference caused by a race codition in the
> probe function of the legousbtower driver. It re-structures the probe
> function to only register the interface after successfully reading the
> board's firmware ID.
> 
> 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. 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.
> 
> This bug has existed since 2003. Patch tested by emulated device.
> 
> Signed-off-by: James Patrick-Evans <james@jmp-e.com>
> ---
> drivers/usb/misc/legousbtower.c | 35 +++++++++++++++++------------------
> 1 file changed, 17 insertions(+), 18 deletions(-)

When you sent this the last time (back on Sept 2), I sent you a
"simpler" patch, the same one below, and asked you if that worked
instead.  I never heard back, could you test this patch and see if it
resolves the issue for you?

thanks,

greg k-h

---------------

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;

Re: [PATCH] usb: misc: legousbtower: Fix NULL pointer deference

[Oliver Neukum]

On Tue, 2016-09-20 at 08:22 +0200, Greg KH wrote:
> When you sent this the last time (back on Sept 2), I sent you a
> "simpler" patch, the same one below, and asked you if that worked
> instead.  I never heard back, could you test this patch and see if it
> resolves the issue for you?

As far as I can tell, your patches are identical.

	Regards
		Oliver

Re: [PATCH] usb: misc: legousbtower: Fix NULL pointer deference

[Greg KH]

On Tue, Sep 20, 2016 at 10:18:21AM +0200, Oliver Neukum wrote:
> On Tue, 2016-09-20 at 08:22 +0200, Greg KH wrote:
> > When you sent this the last time (back on Sept 2), I sent you a
> > "simpler" patch, the same one below, and asked you if that worked
> > instead.  I never heard back, could you test this patch and see if it
> > resolves the issue for you?
> 
> As far as I can tell, your patches are identical.

Hah, so they are.  James's original patch was not this one, sorry for
not looking at it closer.

James, any reason why you are passing off my patch as yours here?  Did
you test mine and see that this did solve the problem?  You did change
your changelog text a bit, to reflect the change between mine and yours,
so I'm guessing that you did?

thanks,

greg k-h

Re: [PATCH] usb: misc: legousbtower: Fix NULL pointer deference

[James]

[-- Attachment #1: Type: text/plain, Size: 1243 bytes --]

Hi greg, 

	Many appologies, I did not see your patch attached/forgot it 
existed when I fixed this yesterday. I created this patch based on your
feedback from my original email on September 2nd. I have tested 
this patch with an emulated device and it fixes the vulnerability.
Again, applogies for the long delay and patch confusion.

Best regards,

James

On Tue, Sep 20, 2016 at 10:32:56AM +0200, Greg KH wrote:
>On Tue, Sep 20, 2016 at 10:18:21AM +0200, Oliver Neukum wrote:
>> On Tue, 2016-09-20 at 08:22 +0200, Greg KH wrote:
>> > When you sent this the last time (back on Sept 2), I sent you a
>> > "simpler" patch, the same one below, and asked you if that worked
>> > instead.  I never heard back, could you test this patch and see if it
>> > resolves the issue for you?
>>
>> As far as I can tell, your patches are identical.
>
>Hah, so they are.  James's original patch was not this one, sorry for
>not looking at it closer.
>
>James, any reason why you are passing off my patch as yours here?  Did
>you test mine and see that this did solve the problem?  You did change
>your changelog text a bit, to reflect the change between mine and yours,
>so I'm guessing that you did?
>
>thanks,
>
>greg k-h

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] usb: misc: legousbtower: Fix NULL pointer deference

[Greg KH]

On Tue, Sep 20, 2016 at 10:09:56AM +0100, James wrote:
> Hi greg,
> 
> 	Many appologies, I did not see your patch attached/forgot it existed when I
> fixed this yesterday. I created this patch based on your
> feedback from my original email on September 2nd. I have tested this patch
> with an emulated device and it fixes the vulnerability.
> Again, applogies for the long delay and patch confusion.

Heh, my feedback contained the patch itself, so it might have been hard
to miss it :)

Anyway, I'll queue this up later today, thanks for the report, and
testing, much appreciated.

greg k-h

Re: [PATCH] usb: misc: legousbtower: Fix NULL pointer deference

[Greg KH]

On Mon, Sep 19, 2016 at 07:09:51PM +0100, James wrote:
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> This patch fixes a NULL pointer dereference caused by a race codition in the
> probe function of the legousbtower driver. It re-structures the probe
> function to only register the interface after successfully reading the
> board's firmware ID.
> 
> 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. 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.
> 
> This bug has existed since 2003. Patch tested by emulated device.
> 
> Reported-by: James Patrick-Evans <james@jmp-e.com>
> Tested-by: James Patrick-Evans <james@jmp-e.com>
> Signed-off-by: James Patrick-Evans <james@jmp-e.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> drivers/usb/misc/legousbtower.c | 35 +++++++++++++++++------------------
> 1 file changed, 17 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
> index 7771be3..4dd531a 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;
> --

Ugh, all of the tabs got turned into spaces, next time be more careful
with your email client :(

thanks,

greg k-h