| Message ID | 20200917095959.174378-1-m.v.b@runbox.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] usbcore/driver: Fix specific driver selection | expand |
On Thu, 2020-09-17 at 12:59 +0300, M. Vefa Bicakci wrote: > This commit resolves two minor bugs in the selection/discovery of > more > specific USB device drivers for devices that are currently bound to > generic USB device drivers. > > The first bug is related to the way a candidate USB device driver is > compared against the generic USB device driver. The code in > is_dev_usb_generic_driver() used to unconditionally use > to_usb_device_driver() on each device driver, without verifying that > the device driver in question is a USB device driver (as opposed to a > USB interface driver). You could also return early if is_usb_device() fails in __usb_bus_reprobe_drivers(). Each of the interface and the device itself is a separate "struct device", and "non-interface" devices won't have interface devices assigned to them. > The second bug is related to the logic that determines whether a > device > currently bound to a generic USB device driver should be re-probed by > a > more specific USB device driver or not. The code in > __usb_bus_reprobe_drivers() used to have the following lines: > > if (usb_device_match_id(udev, new_udriver->id_table) == NULL && > (!new_udriver->match || new_udriver->match(udev) != 0)) > return 0; > > ret = device_reprobe(dev); > > As the reader will notice, the code checks whether the USB device in > consideration matches the identifier table (id_table) of a specific > USB device_driver (new_udriver), followed by a similar check, but > this > time with the USB device driver's match function. However, the match > function's return value is not checked correctly. When match() > returns > zero, it means that the specific USB device driver is *not* > applicable > to the USB device in question, but the code then goes on to reprobe > the > device with the new USB device driver under consideration. All this > to > say, the logic is inverted. Could you split that change as the first commit in your patchset? I'll test your patches locally once you've respun them. Thanks for working on this. Cheers
On 17/09/2020 13.23, Bastien Nocera wrote: > On Thu, 2020-09-17 at 12:59 +0300, M. Vefa Bicakci wrote: >> This commit resolves two minor bugs in the selection/discovery of >> more >> specific USB device drivers for devices that are currently bound to >> generic USB device drivers. >> >> The first bug is related to the way a candidate USB device driver is >> compared against the generic USB device driver. The code in >> is_dev_usb_generic_driver() used to unconditionally use >> to_usb_device_driver() on each device driver, without verifying that >> the device driver in question is a USB device driver (as opposed to a >> USB interface driver). > > You could also return early if is_usb_device() fails in > __usb_bus_reprobe_drivers(). Each of the interface and the device > itself is a separate "struct device", and "non-interface" devices won't > have interface devices assigned to them. Will do! If I understand you correctly, you mean something like the following: static int __usb_bus_reprobe_drivers(struct device *dev, void *data) { struct usb_device_driver *new_udriver = data; struct usb_device *udev; int ret; /* Proposed addition begins */ if (!is_usb_device(dev)) return 0; /* Proposed addition ends */ if (!is_dev_usb_generic_driver(dev)) return 0; >> The second bug is related to the logic that determines whether a >> device >> currently bound to a generic USB device driver should be re-probed by >> a >> more specific USB device driver or not. The code in >> __usb_bus_reprobe_drivers() used to have the following lines: >> >> if (usb_device_match_id(udev, new_udriver->id_table) == NULL && >> (!new_udriver->match || new_udriver->match(udev) != 0)) >> return 0; >> >> ret = device_reprobe(dev); >> >> As the reader will notice, the code checks whether the USB device in >> consideration matches the identifier table (id_table) of a specific >> USB device_driver (new_udriver), followed by a similar check, but >> this >> time with the USB device driver's match function. However, the match >> function's return value is not checked correctly. When match() >> returns >> zero, it means that the specific USB device driver is *not* >> applicable >> to the USB device in question, but the code then goes on to reprobe >> the >> device with the new USB device driver under consideration. All this >> to >> say, the logic is inverted. > > Could you split that change as the first commit in your patchset? Of course! > I'll test your patches locally once you've respun them. Thanks for > working on this. > > Cheers Thank you for reviewing the patches! I intend to address your comments, test the patches and publish them soon (i.e., likely within a few hours). Thank you, Vefa
On Thu, 2020-09-17 at 13:39 +0300, M. Vefa Bicakci wrote: > On 17/09/2020 13.23, Bastien Nocera wrote: > > On Thu, 2020-09-17 at 12:59 +0300, M. Vefa Bicakci wrote: > > > This commit resolves two minor bugs in the selection/discovery of > > > more > > > specific USB device drivers for devices that are currently bound > > > to > > > generic USB device drivers. > > > > > > The first bug is related to the way a candidate USB device driver > > > is > > > compared against the generic USB device driver. The code in > > > is_dev_usb_generic_driver() used to unconditionally use > > > to_usb_device_driver() on each device driver, without verifying > > > that > > > the device driver in question is a USB device driver (as opposed > > > to a > > > USB interface driver). > > > > You could also return early if is_usb_device() fails in > > __usb_bus_reprobe_drivers(). Each of the interface and the device > > itself is a separate "struct device", and "non-interface" devices > > won't > > have interface devices assigned to them. > > Will do! If I understand you correctly, you mean something like the > following: > > static int __usb_bus_reprobe_drivers(struct device *dev, void *data) > { > struct usb_device_driver *new_udriver = data; > struct usb_device *udev; > int ret; > > /* Proposed addition begins */ > > if (!is_usb_device(dev)) > return 0; > > /* Proposed addition ends */ > > if (!is_dev_usb_generic_driver(dev)) > return 0; Or: if (!is_usb_device(dev) || !is_dev_usb_generic_driver(dev)) return 0;
diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c index c976ea9f9582..509bb0d5fa4f 100644 --- a/drivers/usb/core/driver.c +++ b/drivers/usb/core/driver.c @@ -907,10 +907,18 @@ static int usb_uevent(struct device *dev, struct kobj_uevent_env *env) static bool is_dev_usb_generic_driver(struct device *dev) { - struct usb_device_driver *udd = dev->driver ? - to_usb_device_driver(dev->driver) : NULL; + /* A non-existing driver can never be equal to &usb_generic_driver. */ + if (!dev->driver) + return 0; + + /* Check whether the driver is a USB interface driver, which is not + * a USB device driver, and hence cannot be &usb_generic_driver. + * (Plus, to_usb_device_driver is only valid for USB device drivers.) + */ + if (!is_usb_device_driver(dev->driver)) + return 0; - return udd == &usb_generic_driver; + return to_usb_device_driver(dev->driver) == &usb_generic_driver; } static int __usb_bus_reprobe_drivers(struct device *dev, void *data) @@ -924,7 +932,7 @@ static int __usb_bus_reprobe_drivers(struct device *dev, void *data) udev = to_usb_device(dev); if (usb_device_match_id(udev, new_udriver->id_table) == NULL && - (!new_udriver->match || new_udriver->match(udev) != 0)) + (!new_udriver->match || new_udriver->match(udev) == 0)) return 0; ret = device_reprobe(dev);
This commit resolves two minor bugs in the selection/discovery of more specific USB device drivers for devices that are currently bound to generic USB device drivers. The first bug is related to the way a candidate USB device driver is compared against the generic USB device driver. The code in is_dev_usb_generic_driver() used to unconditionally use to_usb_device_driver() on each device driver, without verifying that the device driver in question is a USB device driver (as opposed to a USB interface driver). The second bug is related to the logic that determines whether a device currently bound to a generic USB device driver should be re-probed by a more specific USB device driver or not. The code in __usb_bus_reprobe_drivers() used to have the following lines: if (usb_device_match_id(udev, new_udriver->id_table) == NULL && (!new_udriver->match || new_udriver->match(udev) != 0)) return 0; ret = device_reprobe(dev); As the reader will notice, the code checks whether the USB device in consideration matches the identifier table (id_table) of a specific USB device_driver (new_udriver), followed by a similar check, but this time with the USB device driver's match function. However, the match function's return value is not checked correctly. When match() returns zero, it means that the specific USB device driver is *not* applicable to the USB device in question, but the code then goes on to reprobe the device with the new USB device driver under consideration. All this to say, the logic is inverted. This commit resolves both of the bugs, which were found by code inspection and instrumentation after Andrey Konovalov's report indicating USB/IP subsystem's misbehaviour with the generic USB device driver matching code. Reported-by: Andrey Konovalov <andreyknvl@google.com> Fixes: d5643d2249 ("USB: Fix device driver race") Link: https://lore.kernel.org/linux-usb/CAAeHK+zOrHnxjRFs=OE8T=O9208B9HP_oo8RZpyVOZ9AJ54pAA@mail.gmail.com/ Cc: <stable@vger.kernel.org> # 5.8 Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Alan Stern <stern@rowland.harvard.edu> Cc: Bastien Nocera <hadess@hadess.net> Cc: <syzkaller@googlegroups.com> Signed-off-by: M. Vefa Bicakci <m.v.b@runbox.com> --- drivers/usb/core/driver.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) base-commit: 871e6496207c6aa94134448779c77631a11bfa2e