| Message ID | 20200917144151.355848-2-m.v.b@runbox.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] usbcore/driver: Fix specific driver selection | expand |
On Thu, Sep 17, 2020 at 05:41:50PM +0300, M. Vefa Bicakci wrote: > This commit resolves a minor bug in the selection/discovery of more > specific USB device drivers for devices that are currently bound to > generic USB device drivers. > > The 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() assumes that the device driver in question > is a USB device driver by calling to_usb_device_driver(dev->driver) > to downcast; however I have observed that this assumption is not always > true, through code instrumentation. > > Given that USB device drivers are bound to struct device instances with > of the type &usb_device_type, this commit checks the return value of > is_usb_device() before the call to is_dev_usb_generic_driver(), and > therefore ensures that incorrect type downcasts do not occur. The use > of is_usb_device() was suggested by Bastien Nocera. > > This bug was found while investigating Andrey Konovalov's report > indicating USB/IP subsystem's misbehaviour with the generic USB device > driver matching code. > > 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: Andrey Konovalov <andreyknvl@google.com> > Cc: <syzkaller@googlegroups.com> > Signed-off-by: M. Vefa Bicakci <m.v.b@runbox.com> > --- > drivers/usb/core/driver.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c > index 950044a6e77f..ba7acd6e7cc4 100644 > --- a/drivers/usb/core/driver.c > +++ b/drivers/usb/core/driver.c > @@ -919,7 +919,7 @@ static int __usb_bus_reprobe_drivers(struct device *dev, void *data) > struct usb_device *udev; > int ret; > > - if (!is_dev_usb_generic_driver(dev)) > + if (!is_usb_device(dev) || !is_dev_usb_generic_driver(dev)) > return 0; > > udev = to_usb_device(dev); > -- > 2.26.2 Why not simplify the whole thing by testing the underlying driver pointer? /* Don't reprobe if current driver isn't usb_generic_driver */ if (dev->driver != &usb_generic_driver.drvwrap.driver) return 0; Then is_dev_usb_generic_driver can be removed entirely. Alan Stern
On 17/09/2020 18.01, Alan Stern wrote: > On Thu, Sep 17, 2020 at 05:41:50PM +0300, M. Vefa Bicakci wrote: >> This commit resolves a minor bug in the selection/discovery of more >> specific USB device drivers for devices that are currently bound to >> generic USB device drivers. >> >> The 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() assumes that the device driver in question >> is a USB device driver by calling to_usb_device_driver(dev->driver) >> to downcast; however I have observed that this assumption is not always >> true, through code instrumentation. >> >> Given that USB device drivers are bound to struct device instances with >> of the type &usb_device_type, this commit checks the return value of >> is_usb_device() before the call to is_dev_usb_generic_driver(), and >> therefore ensures that incorrect type downcasts do not occur. The use >> of is_usb_device() was suggested by Bastien Nocera. >> >> This bug was found while investigating Andrey Konovalov's report >> indicating USB/IP subsystem's misbehaviour with the generic USB device >> driver matching code. >> >> 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: Andrey Konovalov <andreyknvl@google.com> >> Cc: <syzkaller@googlegroups.com> >> Signed-off-by: M. Vefa Bicakci <m.v.b@runbox.com> >> --- >> drivers/usb/core/driver.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c >> index 950044a6e77f..ba7acd6e7cc4 100644 >> --- a/drivers/usb/core/driver.c >> +++ b/drivers/usb/core/driver.c >> @@ -919,7 +919,7 @@ static int __usb_bus_reprobe_drivers(struct device *dev, void *data) >> struct usb_device *udev; >> int ret; >> >> - if (!is_dev_usb_generic_driver(dev)) >> + if (!is_usb_device(dev) || !is_dev_usb_generic_driver(dev)) >> return 0; >> >> udev = to_usb_device(dev); >> -- >> 2.26.2 > > Why not simplify the whole thing by testing the underlying driver > pointer? > > /* Don't reprobe if current driver isn't usb_generic_driver */ > if (dev->driver != &usb_generic_driver.drvwrap.driver) > return 0; > > Then is_dev_usb_generic_driver can be removed entirely. Alan, sorry for the delay, and thanks for the review! I had not thought of this. I will apply the changes you have suggested with the next version of the patch series. Thanks again, Vefa
diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c index 950044a6e77f..ba7acd6e7cc4 100644 --- a/drivers/usb/core/driver.c +++ b/drivers/usb/core/driver.c @@ -919,7 +919,7 @@ static int __usb_bus_reprobe_drivers(struct device *dev, void *data) struct usb_device *udev; int ret; - if (!is_dev_usb_generic_driver(dev)) + if (!is_usb_device(dev) || !is_dev_usb_generic_driver(dev)) return 0; udev = to_usb_device(dev);
This commit resolves a minor bug in the selection/discovery of more specific USB device drivers for devices that are currently bound to generic USB device drivers. The 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() assumes that the device driver in question is a USB device driver by calling to_usb_device_driver(dev->driver) to downcast; however I have observed that this assumption is not always true, through code instrumentation. Given that USB device drivers are bound to struct device instances with of the type &usb_device_type, this commit checks the return value of is_usb_device() before the call to is_dev_usb_generic_driver(), and therefore ensures that incorrect type downcasts do not occur. The use of is_usb_device() was suggested by Bastien Nocera. This bug was found while investigating Andrey Konovalov's report indicating USB/IP subsystem's misbehaviour with the generic USB device driver matching code. 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: Andrey Konovalov <andreyknvl@google.com> Cc: <syzkaller@googlegroups.com> Signed-off-by: M. Vefa Bicakci <m.v.b@runbox.com> --- drivers/usb/core/driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)