diff mbox series

[1/3] usbcore/driver: Fix specific driver selection

Message ID 20200917144151.355848-1-m.v.b@runbox.com (mailing list archive)
State New, archived
Headers show
Series [1/3] usbcore/driver: Fix specific driver selection | expand

Commit Message

M. Vefa Bicakci Sept. 17, 2020, 2:41 p.m. UTC 
This commit resolves a 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 in 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 bug was 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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


base-commit: 871e6496207c6aa94134448779c77631a11bfa2e

Comments

M. Vefa Bicakci Sept. 18, 2020, 2:31 p.m. UTC | #1 
On 17/09/2020 17.41, M. Vefa Bicakci wrote:
> This commit resolves a 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 in 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 bug was 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 | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index c976ea9f9582..950044a6e77f 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -924,7 +924,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);
> 
> base-commit: 871e6496207c6aa94134448779c77631a11bfa2e

Hello all,

I noticed that applying this patch on its own to the kernel causes the following
unexpected behaviour: As soon as the usbip_host module is loaded, all of the
USB devices are re-probed() by their drivers, and this causes the USB devices
connected to my system to be momentarily unavailable. This happens because
*without* the third patch in this patch set, the match function for the usbip_host
device driver unconditionally returns true.

The third patch in this patch set [1] makes this unexpected behaviour go
away, as it makes the usbip device driver's match function only match devices
that were requested by user-space to be used with USB-IP.

Is this something to be concerned about? I was thinking of people who might be
using git-bisect, who might encounter this issue in an unexpected manner.

As a potential solution, I can prepare another patch to revert commit
7a2f2974f2 ("usbip: Implement a match function to fix usbip") so that this
unexpected behaviour will not be observed. This revert would be placed as
the first patch in the patch series.

Thank you,

Vefa

[1] https://lore.kernel.org/linux-usb/20200917144151.355848-3-m.v.b@runbox.com/
Alan Stern Sept. 18, 2020, 2:52 p.m. UTC | #2 
On Fri, Sep 18, 2020 at 05:31:26PM +0300, M. Vefa Bicakci wrote:
> Hello all,
> 
> I noticed that applying this patch on its own to the kernel causes the following
> unexpected behaviour: As soon as the usbip_host module is loaded, all of the
> USB devices are re-probed() by their drivers, and this causes the USB devices
> connected to my system to be momentarily unavailable. This happens because
> *without* the third patch in this patch set, the match function for the usbip_host
> device driver unconditionally returns true.
> 
> The third patch in this patch set [1] makes this unexpected behaviour go
> away, as it makes the usbip device driver's match function only match devices
> that were requested by user-space to be used with USB-IP.
> 
> Is this something to be concerned about? I was thinking of people who might be
> using git-bisect, who might encounter this issue in an unexpected manner.
> 
> As a potential solution, I can prepare another patch to revert commit
> 7a2f2974f2 ("usbip: Implement a match function to fix usbip") so that this
> unexpected behaviour will not be observed. This revert would be placed as
> the first patch in the patch series.

Yes, that sounds like a good solution.

Alan Stern
M. Vefa Bicakci Sept. 19, 2020, 1:52 p.m. UTC | #3 
On 18/09/2020 17.52, Alan Stern wrote:
> On Fri, Sep 18, 2020 at 05:31:26PM +0300, M. Vefa Bicakci wrote:
>> Hello all,
>>
>> I noticed that applying this patch on its own to the kernel causes the following
>> unexpected behaviour: As soon as the usbip_host module is loaded, all of the
>> USB devices are re-probed() by their drivers, and this causes the USB devices
>> connected to my system to be momentarily unavailable. This happens because
>> *without* the third patch in this patch set, the match function for the usbip_host
>> device driver unconditionally returns true.
>>
>> The third patch in this patch set [1] makes this unexpected behaviour go
>> away, as it makes the usbip device driver's match function only match devices
>> that were requested by user-space to be used with USB-IP.
>>
>> Is this something to be concerned about? I was thinking of people who might be
>> using git-bisect, who might encounter this issue in an unexpected manner.
>>
>> As a potential solution, I can prepare another patch to revert commit
>> 7a2f2974f2 ("usbip: Implement a match function to fix usbip") so that this
>> unexpected behaviour will not be observed. This revert would be placed as
>> the first patch in the patch series.
> 
> Yes, that sounds like a good solution.
> 
> Alan Stern

Thanks for the feedback, Alan!

Given Shuah's answer to my other question, it looks like there is a need for
further work.

Vefa
diff mbox series

Patch

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index c976ea9f9582..950044a6e77f 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -924,7 +924,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);