Use of usb_find_interface in open is racy

Use of usb_find_interface in open is racy

[Russ Dill]

Many usb drivers that create character devices use "struct
usb_class_driver", a set of fops, and a usb_find_interface in their
open call. A prime example is drivers/usb/usb-skeleton.c. A race
occurs when userspace receives a hotplug event for the addition for
the interface and then opens the associated device file before the
device is added to the driver's klist_devices.

The usb core senses a new usb device (usb_new_device) and calls
device_add. This eventually gets down to really_probe and the
usb-skeleton probe function, skel_probe. skel_probe calls
usb_register_dev() which registers the associated character device for
skel_class. The hotplug events for the class device get emitted.

User space receives the hotplug event for the class device, makes the
device node and notifies another program that opens the device node.
The program opens the device node which calls into usb_open and then
skel_open. skel_open calls usb_find_interface. usb_find_interfaces
searches the klist_devices of skel_driver, finds no device associated
with the minor number and returns NULL. skel_open returns -ENODEV.

Control returns to really_probe and really_probe calls driver_bound
which adds the device to the list of devices associated with
skel_driver (klist_devices).

I'm not sure what the right way to solve this is. A call to
wait_for_device_probe() in the skel_open call before calling
usb_find_interface fixes the problem, but it is a rather large hammer.

Re: Use of usb_find_interface in open is racy

[Jiri Kosina]

[ linux-usb should be CCed here at least ]

On Tue, 17 Nov 2009, Russ Dill wrote:

> Many usb drivers that create character devices use "struct
> usb_class_driver", a set of fops, and a usb_find_interface in their
> open call. A prime example is drivers/usb/usb-skeleton.c. A race
> occurs when userspace receives a hotplug event for the addition for
> the interface and then opens the associated device file before the
> device is added to the driver's klist_devices.
> 
> The usb core senses a new usb device (usb_new_device) and calls
> device_add. This eventually gets down to really_probe and the
> usb-skeleton probe function, skel_probe. skel_probe calls
> usb_register_dev() which registers the associated character device for
> skel_class. The hotplug events for the class device get emitted.
> 
> User space receives the hotplug event for the class device, makes the
> device node and notifies another program that opens the device node.
> The program opens the device node which calls into usb_open and then
> skel_open. skel_open calls usb_find_interface. usb_find_interfaces
> searches the klist_devices of skel_driver, finds no device associated
> with the minor number and returns NULL. skel_open returns -ENODEV.
> 
> Control returns to really_probe and really_probe calls driver_bound
> which adds the device to the list of devices associated with
> skel_driver (klist_devices).
> 
> I'm not sure what the right way to solve this is. A call to
> wait_for_device_probe() in the skel_open call before calling
> usb_find_interface fixes the problem, but it is a rather large hammer.

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: Use of usb_find_interface in open is racy

[Oliver Neukum]

Am Mittwoch, 18. November 2009 11:41:25 schrieb Jiri Kosina:
> > User space receives the hotplug event for the class device, makes the
> > device node and notifies another program that opens the device node.
> > The program opens the device node which calls into usb_open and then
> > skel_open. skel_open calls usb_find_interface. usb_find_interfaces
> > searches the klist_devices of skel_driver, finds no device associated
> > with the minor number and returns NULL. skel_open returns -ENODEV.
> > 
> > Control returns to really_probe and really_probe calls driver_bound
> > which adds the device to the list of devices associated with
> > skel_driver (klist_devices).
> > 
> > I'm not sure what the right way to solve this is. A call to
> > wait_for_device_probe() in the skel_open call before calling
> > usb_find_interface fixes the problem, but it is a rather large hammer.
> 

Device core code is hard to follow, but I tried.
How about simply covering all of usb_register_dev() with minor_rwsem?

	Regards
		Oliver

Re: Use of usb_find_interface in open is racy

[Alan Stern]

On Wed, 18 Nov 2009, Oliver Neukum wrote:

> Am Mittwoch, 18. November 2009 11:41:25 schrieb Jiri Kosina:
> > > User space receives the hotplug event for the class device, makes the
> > > device node and notifies another program that opens the device node.
> > > The program opens the device node which calls into usb_open and then
> > > skel_open. skel_open calls usb_find_interface. usb_find_interfaces
> > > searches the klist_devices of skel_driver, finds no device associated
> > > with the minor number and returns NULL. skel_open returns -ENODEV.
> > > 
> > > Control returns to really_probe and really_probe calls driver_bound
> > > which adds the device to the list of devices associated with
> > > skel_driver (klist_devices).
> > > 
> > > I'm not sure what the right way to solve this is. A call to
> > > wait_for_device_probe() in the skel_open call before calling
> > > usb_find_interface fixes the problem, but it is a rather large hammer.
> > 
> 
> Device core code is hard to follow, but I tried.
> How about simply covering all of usb_register_dev() with minor_rwsem?

That won't help.  The window is between the end of usb_register_dev()  
and the end of skel_probe(), during which time the mutex isn't held.

Alan Stern

Re: Use of usb_find_interface in open is racy

[Russ Dill]

On Wed, Nov 18, 2009 at 8:35 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Wed, 18 Nov 2009, Oliver Neukum wrote:
>
>> Am Mittwoch, 18. November 2009 11:41:25 schrieb Jiri Kosina:
>> > > User space receives the hotplug event for the class device, makes the
>> > > device node and notifies another program that opens the device node.
>> > > The program opens the device node which calls into usb_open and then
>> > > skel_open. skel_open calls usb_find_interface. usb_find_interfaces
>> > > searches the klist_devices of skel_driver, finds no device associated
>> > > with the minor number and returns NULL. skel_open returns -ENODEV.
>> > >
>> > > Control returns to really_probe and really_probe calls driver_bound
>> > > which adds the device to the list of devices associated with
>> > > skel_driver (klist_devices).
>> > >
>> > > I'm not sure what the right way to solve this is. A call to
>> > > wait_for_device_probe() in the skel_open call before calling
>> > > usb_find_interface fixes the problem, but it is a rather large hammer.
>> >
>>
>> Device core code is hard to follow, but I tried.
>> How about simply covering all of usb_register_dev() with minor_rwsem?
>
> That won't help.  The window is between the end of usb_register_dev()
> and the end of skel_probe(), during which time the mutex isn't held.
>

Like I said above, the window goes all the way till the device gets
added to the klist_devices on the driver.

Re: Use of usb_find_interface in open is racy

[Russ Dill]

On Wed, Nov 18, 2009 at 7:27 AM, Oliver Neukum <oliver@neukum.org> wrote:
> Am Mittwoch, 18. November 2009 11:41:25 schrieb Jiri Kosina:
>> > User space receives the hotplug event for the class device, makes the
>> > device node and notifies another program that opens the device node.
>> > The program opens the device node which calls into usb_open and then
>> > skel_open. skel_open calls usb_find_interface. usb_find_interfaces
>> > searches the klist_devices of skel_driver, finds no device associated
>> > with the minor number and returns NULL. skel_open returns -ENODEV.
>> >
>> > Control returns to really_probe and really_probe calls driver_bound
>> > which adds the device to the list of devices associated with
>> > skel_driver (klist_devices).
>> >
>> > I'm not sure what the right way to solve this is. A call to
>> > wait_for_device_probe() in the skel_open call before calling
>> > usb_find_interface fixes the problem, but it is a rather large hammer.
>>
>
> Device core code is hard to follow, but I tried.
> How about simply covering all of usb_register_dev() with minor_rwsem?
>

The race is open from the point that usb_register_dev calls
device_create (at the end of usb_register_dev) till the point that
skel_probe returns up to the point of really_probe, really_probe calls
driver_bound, and driver_bound adds the device to the driver's
klist_devices.

call stack:

device_create
usb_register_dev
skel_probe
usb_probe_device (via drv->probe())
really_probe

really_probe:

dev->driver = drv;
if (driver_sysfs_add(dev)) {
[...]
}

if (dev->bus->probe) {
[...]
} else if (drv->probe) {

Re: Use of usb_find_interface in open is racy

[Alan Stern]

On Wed, 18 Nov 2009, Jiri Kosina wrote:

> On Tue, 17 Nov 2009, Russ Dill wrote:
> 
> > Many usb drivers that create character devices use "struct
> > usb_class_driver", a set of fops, and a usb_find_interface in their
> > open call. A prime example is drivers/usb/usb-skeleton.c. A race
> > occurs when userspace receives a hotplug event for the addition for
> > the interface and then opens the associated device file before the
> > device is added to the driver's klist_devices.
> > 
> > The usb core senses a new usb device (usb_new_device) and calls
> > device_add. This eventually gets down to really_probe and the
> > usb-skeleton probe function, skel_probe. skel_probe calls
> > usb_register_dev() which registers the associated character device for
> > skel_class. The hotplug events for the class device get emitted.
> > 
> > User space receives the hotplug event for the class device, makes the
> > device node and notifies another program that opens the device node.
> > The program opens the device node which calls into usb_open and then
> > skel_open. skel_open calls usb_find_interface. usb_find_interfaces
> > searches the klist_devices of skel_driver, finds no device associated
> > with the minor number and returns NULL. skel_open returns -ENODEV.
> > 
> > Control returns to really_probe and really_probe calls driver_bound
> > which adds the device to the list of devices associated with
> > skel_driver (klist_devices).
> > 
> > I'm not sure what the right way to solve this is. A call to
> > wait_for_device_probe() in the skel_open call before calling
> > usb_find_interface fixes the problem, but it is a rather large hammer.

I think the proper answer is for usb_find_interface() to use 
bus_for_each_dev() instead of driver_for_each_device().

Alan Stern

Re: Use of usb_find_interface in open is racy

[Greg KH]

On Wed, Nov 18, 2009 at 10:31:47AM -0500, Alan Stern wrote:
> On Wed, 18 Nov 2009, Jiri Kosina wrote:
> 
> > On Tue, 17 Nov 2009, Russ Dill wrote:
> > 
> > > Many usb drivers that create character devices use "struct
> > > usb_class_driver", a set of fops, and a usb_find_interface in their
> > > open call. A prime example is drivers/usb/usb-skeleton.c. A race
> > > occurs when userspace receives a hotplug event for the addition for
> > > the interface and then opens the associated device file before the
> > > device is added to the driver's klist_devices.
> > > 
> > > The usb core senses a new usb device (usb_new_device) and calls
> > > device_add. This eventually gets down to really_probe and the
> > > usb-skeleton probe function, skel_probe. skel_probe calls
> > > usb_register_dev() which registers the associated character device for
> > > skel_class. The hotplug events for the class device get emitted.
> > > 
> > > User space receives the hotplug event for the class device, makes the
> > > device node and notifies another program that opens the device node.
> > > The program opens the device node which calls into usb_open and then
> > > skel_open. skel_open calls usb_find_interface. usb_find_interfaces
> > > searches the klist_devices of skel_driver, finds no device associated
> > > with the minor number and returns NULL. skel_open returns -ENODEV.
> > > 
> > > Control returns to really_probe and really_probe calls driver_bound
> > > which adds the device to the list of devices associated with
> > > skel_driver (klist_devices).
> > > 
> > > I'm not sure what the right way to solve this is. A call to
> > > wait_for_device_probe() in the skel_open call before calling
> > > usb_find_interface fixes the problem, but it is a rather large hammer.
> 
> I think the proper answer is for usb_find_interface() to use 
> bus_for_each_dev() instead of driver_for_each_device().

Yeah, I think so, sorry about that I think this is my fault :(

I'm travelling all today, could someone make up a patch for this before
Thursday if it's an issue?

Russ, have you ever hit this problem, or did you just find it by looking
at the code?

thanks,

greg k-h

Re: Use of usb_find_interface in open is racy

[Russ Dill]

On Wed, Nov 18, 2009 at 8:39 AM, Greg KH <greg@kroah.com> wrote:
> On Wed, Nov 18, 2009 at 10:31:47AM -0500, Alan Stern wrote:
>> On Wed, 18 Nov 2009, Jiri Kosina wrote:
>>
>> > On Tue, 17 Nov 2009, Russ Dill wrote:
>> >
>> > > Many usb drivers that create character devices use "struct
>> > > usb_class_driver", a set of fops, and a usb_find_interface in their
>> > > open call. A prime example is drivers/usb/usb-skeleton.c. A race
>> > > occurs when userspace receives a hotplug event for the addition for
>> > > the interface and then opens the associated device file before the
>> > > device is added to the driver's klist_devices.
>> > >
>> > > The usb core senses a new usb device (usb_new_device) and calls
>> > > device_add. This eventually gets down to really_probe and the
>> > > usb-skeleton probe function, skel_probe. skel_probe calls
>> > > usb_register_dev() which registers the associated character device for
>> > > skel_class. The hotplug events for the class device get emitted.
>> > >
>> > > User space receives the hotplug event for the class device, makes the
>> > > device node and notifies another program that opens the device node.
>> > > The program opens the device node which calls into usb_open and then
>> > > skel_open. skel_open calls usb_find_interface. usb_find_interfaces
>> > > searches the klist_devices of skel_driver, finds no device associated
>> > > with the minor number and returns NULL. skel_open returns -ENODEV.
>> > >
>> > > Control returns to really_probe and really_probe calls driver_bound
>> > > which adds the device to the list of devices associated with
>> > > skel_driver (klist_devices).
>> > >
>> > > I'm not sure what the right way to solve this is. A call to
>> > > wait_for_device_probe() in the skel_open call before calling
>> > > usb_find_interface fixes the problem, but it is a rather large hammer.
>>
>> I think the proper answer is for usb_find_interface() to use
>> bus_for_each_dev() instead of driver_for_each_device().
>
> Yeah, I think so, sorry about that I think this is my fault :(
>
> I'm travelling all today, could someone make up a patch for this before
> Thursday if it's an issue?
>
> Russ, have you ever hit this problem, or did you just find it by looking
> at the code?
>

Yes, I'm utilizing devtmpfs to create device nodes and I have an app
that handles hotplug events directly (no udev). Even with that small
amount of overhead, the race does not occur every time, but it still
occurs most of the time.

My initial fix was to add the device to the driver's device list
sooner, but the change to usb_find_interface seems less intrusive and
less likely to cause problems with other code that calls
driver_for_each_device, driver_find_device, or uses
BUS_NOTIFY_BOUND_DRIVER.

I'll work up a patch with the change to usb_find_interface, do some
testing, and email it.

[PATCH] Close usb_find_interface race

[Russ Dill]

USB drivers that create character devices call usb_register_dev in their
probe function. This associates the usb_interface device with that minor
number and creates the character device and announces it to the world.
However, the driver's probe function is called before the new
usb_interface is added to the driver's klist_devices.

This is a problem because userspace will respond to the character device
creation announcement by opening the character device. The driver's open
function will the call usb_find_interface to find the usb_interface
associated with that minor number. usb_find_interface will walk the
driver's list of devices and find the usb_interface with the matching
minor number.

Because the announcement happens before the usb_interface is added to the
driver's klist_devices, a race condition exists. A straightforward fix
is to walk the list of devices on usb_bus_type instead since the device
is added to that list before the announcement occurs.

bus_find_device calls get_device to bump the reference count on the found
device. It is arguable that the reference count should be dropped by the
caller of usb_find_interface instead of usb_find_interface, however,
the current users of usb_find_interface do not expect this.

Signed-off-by: Russ Dill <Russ.Dill@gmail.com>
---
 drivers/usb/core/usb.c |   30 +++++++++++-------------------
 1 files changed, 11 insertions(+), 19 deletions(-)

diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index b1b85ab..d1e9440 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -130,24 +130,17 @@ struct usb_host_interface *usb_altnum_to_altsetting(
 }
 EXPORT_SYMBOL_GPL(usb_altnum_to_altsetting);
 
-struct find_interface_arg {
-	int minor;
-	struct usb_interface *interface;
-};
-
 static int __find_interface(struct device *dev, void *data)
 {
-	struct find_interface_arg *arg = data;
+	int *minor = data;
 	struct usb_interface *intf;
 
 	if (!is_usb_interface(dev))
 		return 0;
 
 	intf = to_usb_interface(dev);
-	if (intf->minor != -1 && intf->minor == arg->minor) {
-		arg->interface = intf;
+	if (intf->minor != -1 && intf->minor == *minor)
 		return 1;
-	}
 	return 0;
 }
 
@@ -156,21 +149,20 @@ static int __find_interface(struct device *dev, void *data)
  * @drv: the driver whose current configuration is considered
  * @minor: the minor number of the desired device
  *
- * This walks the driver device list and returns a pointer to the interface
+ * This walks the bus device list and returns a pointer to the interface
  * with the matching minor.  Note, this only works for devices that share the
  * USB major number.
  */
 struct usb_interface *usb_find_interface(struct usb_driver *drv, int minor)
 {
-	struct find_interface_arg argb;
-	int retval;
+	struct device *dev;
+
+	dev = bus_find_device(&usb_bus_type, NULL, &minor, __find_interface);
+
+	/* Drop reference count from bus_find_device */
+	put_device(dev);
 
-	argb.minor = minor;
-	argb.interface = NULL;
-	/* eat the error, it will be in argb.interface */
-	retval = driver_for_each_device(&drv->drvwrap.driver, NULL, &argb,
-					__find_interface);
-	return argb.interface;
+	return dev ? to_usb_interface(dev) : NULL;
 }
 EXPORT_SYMBOL_GPL(usb_find_interface);
 
-- 
1.6.5

Re: [PATCH] Close usb_find_interface race

[Greg KH]

On Wed, Nov 18, 2009 at 11:02:13AM -0700, Russ Dill wrote:
> USB drivers that create character devices call usb_register_dev in their
> probe function. This associates the usb_interface device with that minor
> number and creates the character device and announces it to the world.
> However, the driver's probe function is called before the new
> usb_interface is added to the driver's klist_devices.
> 
> This is a problem because userspace will respond to the character device
> creation announcement by opening the character device. The driver's open
> function will the call usb_find_interface to find the usb_interface
> associated with that minor number. usb_find_interface will walk the
> driver's list of devices and find the usb_interface with the matching
> minor number.
> 
> Because the announcement happens before the usb_interface is added to the
> driver's klist_devices, a race condition exists. A straightforward fix
> is to walk the list of devices on usb_bus_type instead since the device
> is added to that list before the announcement occurs.
> 
> bus_find_device calls get_device to bump the reference count on the found
> device. It is arguable that the reference count should be dropped by the
> caller of usb_find_interface instead of usb_find_interface, however,
> the current users of usb_find_interface do not expect this.
> 
> Signed-off-by: Russ Dill <Russ.Dill@gmail.com>
> ---
>  drivers/usb/core/usb.c |   30 +++++++++++-------------------
>  1 files changed, 11 insertions(+), 19 deletions(-)

Looks good, thanks for finding and fixing this.  I'll queue it up.

greg k-h

Re: Use of usb_find_interface in open is racy

[Russ Dill]

On Wed, Nov 18, 2009 at 8:31 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Wed, 18 Nov 2009, Jiri Kosina wrote:
>
>> On Tue, 17 Nov 2009, Russ Dill wrote:
>>
>> > Many usb drivers that create character devices use "struct
>> > usb_class_driver", a set of fops, and a usb_find_interface in their
>> > open call. A prime example is drivers/usb/usb-skeleton.c. A race
>> > occurs when userspace receives a hotplug event for the addition for
>> > the interface and then opens the associated device file before the
>> > device is added to the driver's klist_devices.
>> >
>> > The usb core senses a new usb device (usb_new_device) and calls
>> > device_add. This eventually gets down to really_probe and the
>> > usb-skeleton probe function, skel_probe. skel_probe calls
>> > usb_register_dev() which registers the associated character device for
>> > skel_class. The hotplug events for the class device get emitted.
>> >
>> > User space receives the hotplug event for the class device, makes the
>> > device node and notifies another program that opens the device node.
>> > The program opens the device node which calls into usb_open and then
>> > skel_open. skel_open calls usb_find_interface. usb_find_interfaces
>> > searches the klist_devices of skel_driver, finds no device associated
>> > with the minor number and returns NULL. skel_open returns -ENODEV.
>> >
>> > Control returns to really_probe and really_probe calls driver_bound
>> > which adds the device to the list of devices associated with
>> > skel_driver (klist_devices).
>> >
>> > I'm not sure what the right way to solve this is. A call to
>> > wait_for_device_probe() in the skel_open call before calling
>> > usb_find_interface fixes the problem, but it is a rather large hammer.
>
> I think the proper answer is for usb_find_interface() to use
> bus_for_each_dev() instead of driver_for_each_device().
>

This would work, bus_find_device would probably be a better match.