USB driver ID matching broken

USB driver ID matching broken

[Andrey Konovalov]

Hi Bastien, Greg, Alan,

Looks like commit adb6e6ac20ee ("USB: Also match device drivers using
the ->match vfunc") broke the USB driver ID matching process. This, in
turn, led to a complete breakage of the USB fuzzing instance.

This is how an attempt to connect a USB device looks now:

[   39.781642][   T12] usb 1-1: new high-speed USB device number 2
using dummy_hcd
[   40.299955][   T12] usb 1-1: New USB device found, idVendor=0cf3,
idProduct=9271, bcdDevice= 1.08
[   40.303072][   T12] usb 1-1: New USB device strings: Mfr=1,
Product=2, SerialNumber=3
[   40.305678][   T12] usb 1-1: Product: syz
[   40.307041][   T12] usb 1-1: Manufacturer: syz
[   40.308556][   T12] usb 1-1: SerialNumber: syz
[   40.314825][   T12] usbip-host 1-1: 1-1 is not in match_busid table... skip!
[   42.500114][   T51] usb 1-1: USB disconnect, device number 2

It seems that when going through the list of registered IDs the code
tries to match against USB/IP and succeeds as usbip_match() always
returns true.

I'm not sure what's the best fix for this is.

Thanks!

Re: USB driver ID matching broken

[Greg Kroah-Hartman]

On Wed, Sep 16, 2020 at 03:33:25PM +0200, Andrey Konovalov wrote:
> Hi Bastien, Greg, Alan,
> 
> Looks like commit adb6e6ac20ee ("USB: Also match device drivers using
> the ->match vfunc") broke the USB driver ID matching process. This, in
> turn, led to a complete breakage of the USB fuzzing instance.
> 
> This is how an attempt to connect a USB device looks now:
> 
> [   39.781642][   T12] usb 1-1: new high-speed USB device number 2
> using dummy_hcd
> [   40.299955][   T12] usb 1-1: New USB device found, idVendor=0cf3,
> idProduct=9271, bcdDevice= 1.08
> [   40.303072][   T12] usb 1-1: New USB device strings: Mfr=1,
> Product=2, SerialNumber=3
> [   40.305678][   T12] usb 1-1: Product: syz
> [   40.307041][   T12] usb 1-1: Manufacturer: syz
> [   40.308556][   T12] usb 1-1: SerialNumber: syz
> [   40.314825][   T12] usbip-host 1-1: 1-1 is not in match_busid table... skip!
> [   42.500114][   T51] usb 1-1: USB disconnect, device number 2
> 
> It seems that when going through the list of registered IDs the code
> tries to match against USB/IP and succeeds as usbip_match() always
> returns true.
> 
> I'm not sure what's the best fix for this is.

I thought that is what the patch from Bastien was supposed to fix?

If it didn't, we can revert it.

thanks,

greg k-h

Re: USB driver ID matching broken

[Bastien Nocera]

On Wed, 2020-09-16 at 16:15 +0200, Greg Kroah-Hartman wrote:
> On Wed, Sep 16, 2020 at 03:33:25PM +0200, Andrey Konovalov wrote:
> > Hi Bastien, Greg, Alan,
> > 
> > Looks like commit adb6e6ac20ee ("USB: Also match device drivers
> > using
> > the ->match vfunc") broke the USB driver ID matching process. This,
> > in
> > turn, led to a complete breakage of the USB fuzzing instance.
> > 
> > This is how an attempt to connect a USB device looks now:
> > 
> > [   39.781642][   T12] usb 1-1: new high-speed USB device number 2
> > using dummy_hcd
> > [   40.299955][   T12] usb 1-1: New USB device found,
> > idVendor=0cf3,
> > idProduct=9271, bcdDevice= 1.08
> > [   40.303072][   T12] usb 1-1: New USB device strings: Mfr=1,
> > Product=2, SerialNumber=3
> > [   40.305678][   T12] usb 1-1: Product: syz
> > [   40.307041][   T12] usb 1-1: Manufacturer: syz
> > [   40.308556][   T12] usb 1-1: SerialNumber: syz
> > [   40.314825][   T12] usbip-host 1-1: 1-1 is not in match_busid
> > table... skip!
> > [   42.500114][   T51] usb 1-1: USB disconnect, device number 2
> > 
> > It seems that when going through the list of registered IDs the
> > code
> > tries to match against USB/IP and succeeds as usbip_match() always
> > returns true.
> > 
> > I'm not sure what's the best fix for this is.
> 
> I thought that is what the patch from Bastien was supposed to fix?
> 
> If it didn't, we can revert it.

It wasn't. Are you thinking of "usbip: Implement a match function to
fix usbip" by M. Vefa Bicakci (CC:ed)?

Seems to me that usbip wants to match *every* device. Wouldn't that be
a bug in usbip?

Re: USB driver ID matching broken

[M. Vefa Bicakci]

On 16/09/2020 17.39, Bastien Nocera wrote:
> On Wed, 2020-09-16 at 16:15 +0200, Greg Kroah-Hartman wrote:
>> On Wed, Sep 16, 2020 at 03:33:25PM +0200, Andrey Konovalov wrote:
>>> Hi Bastien, Greg, Alan,
>>>
>>> Looks like commit adb6e6ac20ee ("USB: Also match device drivers
>>> using
>>> the ->match vfunc") broke the USB driver ID matching process. This,
>>> in
>>> turn, led to a complete breakage of the USB fuzzing instance.
>>>
>>> This is how an attempt to connect a USB device looks now:
>>>
>>> [   39.781642][   T12] usb 1-1: new high-speed USB device number 2
>>> using dummy_hcd
>>> [   40.299955][   T12] usb 1-1: New USB device found,
>>> idVendor=0cf3,
>>> idProduct=9271, bcdDevice= 1.08
>>> [   40.303072][   T12] usb 1-1: New USB device strings: Mfr=1,
>>> Product=2, SerialNumber=3
>>> [   40.305678][   T12] usb 1-1: Product: syz
>>> [   40.307041][   T12] usb 1-1: Manufacturer: syz
>>> [   40.308556][   T12] usb 1-1: SerialNumber: syz
>>> [   40.314825][   T12] usbip-host 1-1: 1-1 is not in match_busid
>>> table... skip!
>>> [   42.500114][   T51] usb 1-1: USB disconnect, device number 2
>>>
>>> It seems that when going through the list of registered IDs the
>>> code
>>> tries to match against USB/IP and succeeds as usbip_match() always
>>> returns true.
>>>
>>> I'm not sure what's the best fix for this is.
>>
>> I thought that is what the patch from Bastien was supposed to fix?
>>
>> If it didn't, we can revert it.
> 
> It wasn't. Are you thinking of "usbip: Implement a match function to
> fix usbip" by M. Vefa Bicakci (CC:ed)?
> 
> Seems to me that usbip wants to match *every* device. Wouldn't that be
> a bug in usbip?

Hello all,

I agree with Bastien; it looks like the match function that I had prepared
for the "USB-IP no longer works starting with v5.7.y" bug at [1] is not
appropriate due to the fact that the match function always returns true.

My understanding of how USB-IP works is that the user-space provides the
USB bus identifier of the device to be published via USB-IP to the kernel
via /sys/bus/usb/drivers/usbip-host/match_busid. Given that the bus
identifiers written to match_busid are stored in a table, perhaps this
table can be queried in the usbip_match function to avoid the issue
reported by Andrey while preserving USB-IP's functionality?

If needed, I can prepare a patch implementing this proposal, perhaps after
commit 7a2f2974 ("usbip: Implement a match function to fix usbip") is
reverted. The only catch is that my bandwidth is a bit limited, hence it
may take some time for me to publish a patch.

Sorry for this unexpected bug,

Vefa

[1] https://bugzilla.kernel.org/show_bug.cgi?id=208267

[PATCH 1/2] usbcore/driver: Fix specific driver selection

[M. Vefa Bicakci]

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(-)

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);

base-commit: 871e6496207c6aa94134448779c77631a11bfa2e
-- 
2.26.2

[PATCH 2/2] usbip: Make the driver's match function specific

[M. Vefa Bicakci]

Prior to this commit, the USB-IP subsystem's USB device driver match
function used to match all USB devices (by returning true
unconditionally). Unfortunately, this is not correct behaviour and is
likely the root cause of the bug reported by Andrey Konovalov.

USB-IP should only match USB devices that the user-space asked the kernel
to handle via USB-IP, by writing to the match_busid sysfs file, which is
what this commit aims to achieve. This is done by making the match
function check that the passed in USB device was indeed requested by the
user-space to be handled by USB-IP.

Reported-by: Andrey Konovalov <andreyknvl@google.com>
Fixes: 7a2f2974f2 ("usbip: Implement a match function to fix usbip")
Link: https://lore.kernel.org/linux-usb/CAAeHK+zOrHnxjRFs=OE8T=O9208B9HP_oo8RZpyVOZ9AJ54pAA@mail.gmail.com/
Cc: <stable@vger.kernel.org> # 5.8
Cc: Bastien Nocera <hadess@hadess.net>
Cc: Valentina Manea <valentina.manea.m@gmail.com>
Cc: Shuah Khan <shuah@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: <syzkaller@googlegroups.com>
Signed-off-by: M. Vefa Bicakci <m.v.b@runbox.com>
---
 drivers/usb/usbip/stub_dev.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/usbip/stub_dev.c b/drivers/usb/usbip/stub_dev.c
index 9d7d642022d1..3d9c8ff6762e 100644
--- a/drivers/usb/usbip/stub_dev.c
+++ b/drivers/usb/usbip/stub_dev.c
@@ -463,7 +463,20 @@ static void stub_disconnect(struct usb_device *udev)
 
 static bool usbip_match(struct usb_device *udev)
 {
-	return true;
+	bool match;
+	struct bus_id_priv *busid_priv;
+	const char *udev_busid = dev_name(&udev->dev);
+
+	busid_priv = get_busid_priv(udev_busid);
+	if (!busid_priv)
+		return false;
+
+	match = (busid_priv->status != STUB_BUSID_REMOV &&
+		 busid_priv->status != STUB_BUSID_OTHER);
+
+	put_busid_priv(busid_priv);
+
+	return match;
 }
 
 #ifdef CONFIG_PM
-- 
2.26.2

Re: [PATCH 1/2] usbcore/driver: Fix specific driver selection

[Bastien Nocera]

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

Re: [PATCH 1/2] usbcore/driver: Fix specific driver selection

[M. Vefa Bicakci]

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

Re: [PATCH 1/2] usbcore/driver: Fix specific driver selection

[Bastien Nocera]

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;

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

[M. Vefa Bicakci]

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

[PATCH 2/3] usbcore/driver: Fix incorrect downcast

[M. Vefa Bicakci]

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

[PATCH 3/3] usbip: Make the driver's match function specific

[M. Vefa Bicakci]

Prior to this commit, the USB-IP subsystem's USB device driver match
function used to match all USB devices (by returning true
unconditionally). Unfortunately, this is not correct behaviour and is
likely the root cause of the bug reported by Andrey Konovalov.

USB-IP should only match USB devices that the user-space asked the kernel
to handle via USB-IP, by writing to the match_busid sysfs file, which is
what this commit aims to achieve. This is done by making the match
function check that the passed in USB device was indeed requested by the
user-space to be handled by USB-IP.

Reported-by: Andrey Konovalov <andreyknvl@google.com>
Fixes: 7a2f2974f2 ("usbip: Implement a match function to fix usbip")
Link: https://lore.kernel.org/linux-usb/CAAeHK+zOrHnxjRFs=OE8T=O9208B9HP_oo8RZpyVOZ9AJ54pAA@mail.gmail.com/
Cc: <stable@vger.kernel.org> # 5.8
Cc: Bastien Nocera <hadess@hadess.net>
Cc: Valentina Manea <valentina.manea.m@gmail.com>
Cc: Shuah Khan <shuah@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: <syzkaller@googlegroups.com>
Signed-off-by: M. Vefa Bicakci <m.v.b@runbox.com>
---
 drivers/usb/usbip/stub_dev.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/usbip/stub_dev.c b/drivers/usb/usbip/stub_dev.c
index 9d7d642022d1..3d9c8ff6762e 100644
--- a/drivers/usb/usbip/stub_dev.c
+++ b/drivers/usb/usbip/stub_dev.c
@@ -463,7 +463,20 @@ static void stub_disconnect(struct usb_device *udev)
 
 static bool usbip_match(struct usb_device *udev)
 {
-	return true;
+	bool match;
+	struct bus_id_priv *busid_priv;
+	const char *udev_busid = dev_name(&udev->dev);
+
+	busid_priv = get_busid_priv(udev_busid);
+	if (!busid_priv)
+		return false;
+
+	match = (busid_priv->status != STUB_BUSID_REMOV &&
+		 busid_priv->status != STUB_BUSID_OTHER);
+
+	put_busid_priv(busid_priv);
+
+	return match;
 }
 
 #ifdef CONFIG_PM
-- 
2.26.2

Re: [PATCH 2/3] usbcore/driver: Fix incorrect downcast

[Alan Stern]

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

Re: [PATCH 3/3] usbip: Make the driver's match function specific

[Shuah Khan]

On 9/17/20 8:41 AM, M. Vefa Bicakci wrote:
> Prior to this commit, the USB-IP subsystem's USB device driver match
> function used to match all USB devices (by returning true
> unconditionally). Unfortunately, this is not correct behaviour and is
> likely the root cause of the bug reported by Andrey Konovalov.
> 
> USB-IP should only match USB devices that the user-space asked the kernel
> to handle via USB-IP, by writing to the match_busid sysfs file, which is
> what this commit aims to achieve. This is done by making the match
> function check that the passed in USB device was indeed requested by the
> user-space to be handled by USB-IP.
> 

I see two patches 2/2 and 3/3 back to back. What is the difference
between 2/2 and 3/3 versions? They look identical. Please include
changes if any from version to version to make it easier for me to
review.

> Reported-by: Andrey Konovalov <andreyknvl@google.com>
> Fixes: 7a2f2974f2 ("usbip: Implement a match function to fix usbip")
> Link: https://lore.kernel.org/linux-usb/CAAeHK+zOrHnxjRFs=OE8T=O9208B9HP_oo8RZpyVOZ9AJ54pAA@mail.gmail.com/
> Cc: <stable@vger.kernel.org> # 5.8
> Cc: Bastien Nocera <hadess@hadess.net>
> Cc: Valentina Manea <valentina.manea.m@gmail.com>
> Cc: Shuah Khan <shuah@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: <syzkaller@googlegroups.com>
> Signed-off-by: M. Vefa Bicakci <m.v.b@runbox.com>
> ---
>   drivers/usb/usbip/stub_dev.c | 15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/usbip/stub_dev.c b/drivers/usb/usbip/stub_dev.c
> index 9d7d642022d1..3d9c8ff6762e 100644
> --- a/drivers/usb/usbip/stub_dev.c
> +++ b/drivers/usb/usbip/stub_dev.c
> @@ -463,7 +463,20 @@ static void stub_disconnect(struct usb_device *udev)
>   
>   static bool usbip_match(struct usb_device *udev)
>   {
> -	return true;
> +	bool match;
> +	struct bus_id_priv *busid_priv;
> +	const char *udev_busid = dev_name(&udev->dev);
> +
> +	busid_priv = get_busid_priv(udev_busid);
> +	if (!busid_priv)
> +		return false;
> +
> +	match = (busid_priv->status != STUB_BUSID_REMOV &&
> +		 busid_priv->status != STUB_BUSID_OTHER);
> +
> +	put_busid_priv(busid_priv);
> +
> +	return match;
>   }
>   
>   #ifdef CONFIG_PM
> 

Did you happen to run the usbip test on this patch? If not, can you
please run tools/testing/selftests/drivers/usb/usbip/usbip_test.sh
and make sure there are no regressions.

thanks,
-- Shuah

Re: [PATCH 2/3] usbcore/driver: Fix incorrect downcast

[M. Vefa Bicakci]

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

Re: [PATCH 3/3] usbip: Make the driver's match function specific

[M. Vefa Bicakci]

On 17/09/2020 18.21, Shuah Khan wrote:
> On 9/17/20 8:41 AM, M. Vefa Bicakci wrote:
>> Prior to this commit, the USB-IP subsystem's USB device driver match
>> function used to match all USB devices (by returning true
>> unconditionally). Unfortunately, this is not correct behaviour and is
>> likely the root cause of the bug reported by Andrey Konovalov.
>>
>> USB-IP should only match USB devices that the user-space asked the kernel
>> to handle via USB-IP, by writing to the match_busid sysfs file, which is
>> what this commit aims to achieve. This is done by making the match
>> function check that the passed in USB device was indeed requested by the
>> user-space to be handled by USB-IP.
>>
> 
> I see two patches 2/2 and 3/3 back to back. What is the difference
> between 2/2 and 3/3 versions? They look identical. Please include
> changes if any from version to version to make it easier for me to
> review.

Hello Shuah,

Sorry for the delayed reply, and thank you for your interest! I realize
that I did not add notes to the patch series regarding the changes between
v1 and v2, and I forgot to label the second patch series as v2.

Patches 2/2 and 3/3 are the same, as you have mentioned. I was addressing
Bastien's code review comments for patch 1/2, and I split that patch into
two separate patches, which is why the second patch series had 3 patches
as opposed to 2.

I realize that you are missing the context; here is a link to the thread:
   https://lore.kernel.org/linux-usb/359d080c-5cbb-250a-0ebd-aaba5f5c530d@runbox.com/T/

I can copy all patches to you as well, if you would be interested.

All this to say, I am sorry about this small mess, and I will rectify this
with patches I publish in the future.

>> Reported-by: Andrey Konovalov <andreyknvl@google.com>
>> Fixes: 7a2f2974f2 ("usbip: Implement a match function to fix usbip")
>> Link: https://lore.kernel.org/linux-usb/CAAeHK+zOrHnxjRFs=OE8T=O9208B9HP_oo8RZpyVOZ9AJ54pAA@mail.gmail.com/
>> Cc: <stable@vger.kernel.org> # 5.8
>> Cc: Bastien Nocera <hadess@hadess.net>
>> Cc: Valentina Manea <valentina.manea.m@gmail.com>
>> Cc: Shuah Khan <shuah@kernel.org>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Alan Stern <stern@rowland.harvard.edu>
>> Cc: <syzkaller@googlegroups.com>
>> Signed-off-by: M. Vefa Bicakci <m.v.b@runbox.com>
>> ---
>>   drivers/usb/usbip/stub_dev.c | 15 ++++++++++++++-
>>   1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/usbip/stub_dev.c b/drivers/usb/usbip/stub_dev.c
>> index 9d7d642022d1..3d9c8ff6762e 100644
>> --- a/drivers/usb/usbip/stub_dev.c
>> +++ b/drivers/usb/usbip/stub_dev.c
>> @@ -463,7 +463,20 @@ static void stub_disconnect(struct usb_device *udev)
>>   static bool usbip_match(struct usb_device *udev)
>>   {
>> -    return true;
>> +    bool match;
>> +    struct bus_id_priv *busid_priv;
>> +    const char *udev_busid = dev_name(&udev->dev);
>> +
>> +    busid_priv = get_busid_priv(udev_busid);
>> +    if (!busid_priv)
>> +        return false;
>> +
>> +    match = (busid_priv->status != STUB_BUSID_REMOV &&
>> +         busid_priv->status != STUB_BUSID_OTHER);
>> +
>> +    put_busid_priv(busid_priv);
>> +
>> +    return match;
>>   }
>>   #ifdef CONFIG_PM
>>
> 
> Did you happen to run the usbip test on this patch? If not, can you
> please run tools/testing/selftests/drivers/usb/usbip/usbip_test.sh
> and make sure there are no regressions.

Ah, this is a very good point! I have been testing the patches on Qubes OS,
which uses usbip to forward USB devices between VMs. To be honest, I was not
aware of the self-tests for usbip, and I will run the self-tests prior to
publishing the next version of the patch series.

> thanks,
> -- Shuah

Thank you!

Vefa

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

[M. Vefa Bicakci]

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/

Re: [PATCH 3/3] usbip: Make the driver's match function specific

[M. Vefa Bicakci]

On 18/09/2020 12.26, M. Vefa Bicakci wrote:
> On 17/09/2020 18.21, Shuah Khan wrote:
>> On 9/17/20 8:41 AM, M. Vefa Bicakci wrote:
>>> Prior to this commit, the USB-IP subsystem's USB device driver match
>>> function used to match all USB devices (by returning true
>>> unconditionally). Unfortunately, this is not correct behaviour and is
>>> likely the root cause of the bug reported by Andrey Konovalov.
>>>
>>> USB-IP should only match USB devices that the user-space asked the kernel
>>> to handle via USB-IP, by writing to the match_busid sysfs file, which is
>>> what this commit aims to achieve. This is done by making the match
>>> function check that the passed in USB device was indeed requested by the
>>> user-space to be handled by USB-IP.

[snipped by Vefa]

>>> Reported-by: Andrey Konovalov <andreyknvl@google.com>
>>> Fixes: 7a2f2974f2 ("usbip: Implement a match function to fix usbip")
>>> Link: https://lore.kernel.org/linux-usb/CAAeHK+zOrHnxjRFs=OE8T=O9208B9HP_oo8RZpyVOZ9AJ54pAA@mail.gmail.com/
>>> Cc: <stable@vger.kernel.org> # 5.8
>>> Cc: Bastien Nocera <hadess@hadess.net>
>>> Cc: Valentina Manea <valentina.manea.m@gmail.com>
>>> Cc: Shuah Khan <shuah@kernel.org>
>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> Cc: Alan Stern <stern@rowland.harvard.edu>
>>> Cc: <syzkaller@googlegroups.com>
>>> Signed-off-by: M. Vefa Bicakci <m.v.b@runbox.com>
>>> ---
>>>   drivers/usb/usbip/stub_dev.c | 15 ++++++++++++++-
>>>   1 file changed, 14 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/usbip/stub_dev.c b/drivers/usb/usbip/stub_dev.c
>>> index 9d7d642022d1..3d9c8ff6762e 100644
>>> --- a/drivers/usb/usbip/stub_dev.c
>>> +++ b/drivers/usb/usbip/stub_dev.c
>>> @@ -463,7 +463,20 @@ static void stub_disconnect(struct usb_device *udev)
>>>   static bool usbip_match(struct usb_device *udev)
>>>   {
>>> -    return true;
>>> +    bool match;
>>> +    struct bus_id_priv *busid_priv;
>>> +    const char *udev_busid = dev_name(&udev->dev);
>>> +
>>> +    busid_priv = get_busid_priv(udev_busid);
>>> +    if (!busid_priv)
>>> +        return false;
>>> +
>>> +    match = (busid_priv->status != STUB_BUSID_REMOV &&
>>> +         busid_priv->status != STUB_BUSID_OTHER);
>>> +
>>> +    put_busid_priv(busid_priv);
>>> +
>>> +    return match;
>>>   }
>>>   #ifdef CONFIG_PM
>>>
>>
>> Did you happen to run the usbip test on this patch? If not, can you
>> please run tools/testing/selftests/drivers/usb/usbip/usbip_test.sh
>> and make sure there are no regressions.
> 
> Ah, this is a very good point! I have been testing the patches on Qubes OS,
> which uses usbip to forward USB devices between VMs. To be honest, I was not
> aware of the self-tests for usbip, and I will run the self-tests prior to
> publishing the next version of the patch series.

Hello Shuah,

I have just cleaned up the patches and run usbip_test.sh with a kernel without
the patches in this series and with a kernel in this series.

I noticed that there is a change in behaviour due to the fact that the new
match function (usbip_match) does not always return true. This causes the
stub device driver's probe() function to not get called at all, as the new
more selective match function will prevent the stub device driver from being
considered as a potential driver for the device under consideration.

All of this results in the following difference in the logs of the usbip_test.sh,
where the expected kernel log message "usbip-host 2-6: 2-6 is not in match_busid table... skip!"
is not printed by a kernel that includes the patches in this series.

--- unpatched_kernel_log.txt  2020-09-18 17:12:10.654000000 +0300
+++ patched_kernel_log.txt  2020-09-18 17:12:10.654000000 +0300
@@ -213,70 +213,69 @@
      |__ Port 1: Dev 2, If 0, Class=Human Interface Device, Driver=usbhid, 480M
  ==============================================================
  modprobe usbip_host - does it work?
  Should see -busid- is not in match_busid table... skip! dmesg
  ==============================================================
-usbip-host 2-6: 2-6 is not in match_busid table... skip!
  ==============================================================

Do you find this change in behaviour unacceptable? If no, I can remove this
test case from usbip_test.sh with the same patch. If yes, then there is a need
for a different solution to resolve the unexpected negative interaction between
Bastien's work on generic/specific USB device driver selection and usbip
functionality.

Thank you,

Vefa

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

[Alan Stern]

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

Re: [PATCH 3/3] usbip: Make the driver's match function specific

[Shuah Khan]

On 9/18/20 8:31 AM, M. Vefa Bicakci wrote:
> On 18/09/2020 12.26, M. Vefa Bicakci wrote:
>> On 17/09/2020 18.21, Shuah Khan wrote:
>>> On 9/17/20 8:41 AM, M. Vefa Bicakci wrote:
>>>> Prior to this commit, the USB-IP subsystem's USB device driver match
>>>> function used to match all USB devices (by returning true
>>>> unconditionally). Unfortunately, this is not correct behaviour and is
>>>> likely the root cause of the bug reported by Andrey Konovalov.
>>>>
>>>> USB-IP should only match USB devices that the user-space asked the 
>>>> kernel
>>>> to handle via USB-IP, by writing to the match_busid sysfs file, 
>>>> which is
>>>> what this commit aims to achieve. This is done by making the match
>>>> function check that the passed in USB device was indeed requested by 
>>>> the
>>>> user-space to be handled by USB-IP.
> 
> [snipped by Vefa]
> 
>>>> Reported-by: Andrey Konovalov <andreyknvl@google.com>
>>>> Fixes: 7a2f2974f2 ("usbip: Implement a match function to fix usbip")
>>>> Link: 
>>>> https://lore.kernel.org/linux-usb/CAAeHK+zOrHnxjRFs=OE8T=O9208B9HP_oo8RZpyVOZ9AJ54pAA@mail.gmail.com/ 
>>>>
>>>> Cc: <stable@vger.kernel.org> # 5.8
>>>> Cc: Bastien Nocera <hadess@hadess.net>
>>>> Cc: Valentina Manea <valentina.manea.m@gmail.com>
>>>> Cc: Shuah Khan <shuah@kernel.org>
>>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>> Cc: Alan Stern <stern@rowland.harvard.edu>
>>>> Cc: <syzkaller@googlegroups.com>
>>>> Signed-off-by: M. Vefa Bicakci <m.v.b@runbox.com>
>>>> ---
>>>>   drivers/usb/usbip/stub_dev.c | 15 ++++++++++++++-
>>>>   1 file changed, 14 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/usb/usbip/stub_dev.c 
>>>> b/drivers/usb/usbip/stub_dev.c
>>>> index 9d7d642022d1..3d9c8ff6762e 100644
>>>> --- a/drivers/usb/usbip/stub_dev.c
>>>> +++ b/drivers/usb/usbip/stub_dev.c
>>>> @@ -463,7 +463,20 @@ static void stub_disconnect(struct usb_device 
>>>> *udev)
>>>>   static bool usbip_match(struct usb_device *udev)
>>>>   {
>>>> -    return true;
>>>> +    bool match;
>>>> +    struct bus_id_priv *busid_priv;
>>>> +    const char *udev_busid = dev_name(&udev->dev);
>>>> +
>>>> +    busid_priv = get_busid_priv(udev_busid);
>>>> +    if (!busid_priv)
>>>> +        return false;
>>>> +
>>>> +    match = (busid_priv->status != STUB_BUSID_REMOV &&
>>>> +         busid_priv->status != STUB_BUSID_OTHER);
>>>> +
>>>> +    put_busid_priv(busid_priv);
>>>> +
>>>> +    return match;
>>>>   }
>>>>   #ifdef CONFIG_PM
>>>>
>>>
>>> Did you happen to run the usbip test on this patch? If not, can you
>>> please run tools/testing/selftests/drivers/usb/usbip/usbip_test.sh
>>> and make sure there are no regressions.
>>
>> Ah, this is a very good point! I have been testing the patches on 
>> Qubes OS,
>> which uses usbip to forward USB devices between VMs. To be honest, I 
>> was not
>> aware of the self-tests for usbip, and I will run the self-tests prior to
>> publishing the next version of the patch series.
> 
> Hello Shuah,
> 
> I have just cleaned up the patches and run usbip_test.sh with a kernel 
> without
> the patches in this series and with a kernel in this series.
> 
> I noticed that there is a change in behaviour due to the fact that the new
> match function (usbip_match) does not always return true. This causes the
> stub device driver's probe() function to not get called at all, as the new
> more selective match function will prevent the stub device driver from 
> being
> considered as a potential driver for the device under consideration.
> 

Yes. This is the behavior I am concerned about and hence the reason
to use the usbip test to verify this doesn't happen.

With the patch you have the usbip match behavior becomes restrictive
which isn't desirable.

> All of this results in the following difference in the logs of the 
> usbip_test.sh,
> where the expected kernel log message "usbip-host 2-6: 2-6 is not in 
> match_busid table... skip!"
> is not printed by a kernel that includes the patches in this series.
> 
> --- unpatched_kernel_log.txt  2020-09-18 17:12:10.654000000 +0300
> +++ patched_kernel_log.txt  2020-09-18 17:12:10.654000000 +0300
> @@ -213,70 +213,69 @@
>       |__ Port 1: Dev 2, If 0, Class=Human Interface Device, 
> Driver=usbhid, 480M
>   ==============================================================
>   modprobe usbip_host - does it work?
>   Should see -busid- is not in match_busid table... skip! dmesg
>   ==============================================================
> -usbip-host 2-6: 2-6 is not in match_busid table... skip!
>   ==============================================================
> 
> Do you find this change in behaviour unacceptable?

Yeah. This behavior isn't acceptable.

If no, I can remove this
> test case from usbip_test.sh with the same patch. If yes, then there is 
> a need
> for a different solution to resolve the unexpected negative interaction 
> between
> Bastien's work on generic/specific USB device driver selection and usbip
> functionality.
> 

I would recommend finding a different solution. Now that you have the
usbip test handy, you can verify and test for regressions.

thanks,
-- Shuah

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

[M. Vefa Bicakci]

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

Re: [PATCH 3/3] usbip: Make the driver's match function specific

[M. Vefa Bicakci]

On 18/09/2020 18.49, Shuah Khan wrote:
> On 9/18/20 8:31 AM, M. Vefa Bicakci wrote:
>> Hello Shuah,
>>
>> I have just cleaned up the patches and run usbip_test.sh with a kernel without
>> the patches in this series and with a kernel in this series.
>>
>> I noticed that there is a change in behaviour due to the fact that the new
>> match function (usbip_match) does not always return true. This causes the
>> stub device driver's probe() function to not get called at all, as the new
>> more selective match function will prevent the stub device driver from being
>> considered as a potential driver for the device under consideration.
>>
> 
> Yes. This is the behavior I am concerned about and hence the reason
> to use the usbip test to verify this doesn't happen.
> 
> With the patch you have the usbip match behavior becomes restrictive
> which isn't desirable.
> 
>> All of this results in the following difference in the logs of the usbip_test.sh,
>> where the expected kernel log message "usbip-host 2-6: 2-6 is not in match_busid table... skip!"
>> is not printed by a kernel that includes the patches in this series.
>>
>> --- unpatched_kernel_log.txt  2020-09-18 17:12:10.654000000 +0300
>> +++ patched_kernel_log.txt  2020-09-18 17:12:10.654000000 +0300
>> @@ -213,70 +213,69 @@
>>       |__ Port 1: Dev 2, If 0, Class=Human Interface Device, Driver=usbhid, 480M
>>   ==============================================================
>>   modprobe usbip_host - does it work?
>>   Should see -busid- is not in match_busid table... skip! dmesg
>>   ==============================================================
>> -usbip-host 2-6: 2-6 is not in match_busid table... skip!
>>   ==============================================================
>>
>> Do you find this change in behaviour unacceptable?
> 
> Yeah. This behavior isn't acceptable.
> 
> If no, I can remove this
>> test case from usbip_test.sh with the same patch. If yes, then there is a need
>> for a different solution to resolve the unexpected negative interaction between
>> Bastien's work on generic/specific USB device driver selection and usbip
>> functionality.
>>
> 
> I would recommend finding a different solution. Now that you have the
> usbip test handy, you can verify and test for regressions.
> 
> thanks,
> -- Shuah

Thanks for the feedback, Shuah. I spent some time looking at and instrumenting
the code in an attempt to find another solution, but have not found one.

If the generic/specific USB driver selection functionality that Bastien Nocera
introduced is desired to stay in the kernel, then making usbip_match more
specific appears to be the only option for usbip to be functional without
negatively affecting other device drivers.

Should Bastien's work be reverted until a solution to this issue is found?
Would you (or anyone) have any suggestions? I would be happy to work further
on resolving this issue.

I am including a summary of my findings below my signature.

Thank you,

Vefa

=== === ===

(1) If usbip_match stays as is, then the issue reported by Andrey Konovalov at

       https://lore.kernel.org/linux-usb/CAAeHK+zOrHnxjRFs=OE8T=O9208B9HP_oo8RZpyVOZ9AJ54pAA@mail.gmail.com/

     will remain unresolved and will trickle down to Linux 5.9 due to the following
     commit's interaction with the return-true usbip_match function:

       USB: Simplify USB ID table match
       https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=master&id=0ed9498f9ecfde50c93f3f3dd64b4cd5414d9944

     This commit was mis-classified as a simplification rather than a bug-fix
     and was not backported to linux-5.8.y. (I can explain my reasoning if beneficial.)
     After cherry-picking this commit onto 5.8.10, I can reproduce Andrey's issue
     with Andrey's "keyboard" program at: https://github.com/xairy/raw-gadget.git

(2) If the following patches are also included in the kernel to fix two bugs in the
     USB driver selection logic, then a return-true usbip_match function will cause
     usbip's stub driver to be selected as the more specific device driver for all
     USB devices when the usbip_host module is loaded. (This makes all USB devices
     unavailable on my system, until I unload usbip_host and reload {x,e}hci-pci
     modules.)

       usbcore/driver: Fix specific driver selection
       https://marc.info/?l=linux-usb&m=160037262607960&q=mbox

       usbcore/driver: Fix incorrect downcast
       https://lore.kernel.org/linux-usb/20200917144151.355848-2-m.v.b@runbox.com/

     I realize that this impact statement does not match my previous description at

        https://lore.kernel.org/linux-usb/363eab9a-c32a-4c60-4d6b-14ae8d873c52@runbox.com/

     which was because I had forgotten to cherry pick 0ed9498f9e ("USB: Simplify
     USB ID table match"; i.e., the commit discussed in the previous bullet point)
     onto my v5.8.10-based test kernel.

Re: [PATCH 3/3] usbip: Make the driver's match function specific

[M. Vefa Bicakci]

On 19/09/2020 16.54, M. Vefa Bicakci wrote:
> On 18/09/2020 18.49, Shuah Khan wrote:
>> On 9/18/20 8:31 AM, M. Vefa Bicakci wrote:
>>> Hello Shuah,
>>>
>>> I have just cleaned up the patches and run usbip_test.sh with a kernel without
>>> the patches in this series and with a kernel in this series.
>>>
>>> I noticed that there is a change in behaviour due to the fact that the new
>>> match function (usbip_match) does not always return true. This causes the
>>> stub device driver's probe() function to not get called at all, as the new
>>> more selective match function will prevent the stub device driver from being
>>> considered as a potential driver for the device under consideration.
>>>
>>
>> Yes. This is the behavior I am concerned about and hence the reason
>> to use the usbip test to verify this doesn't happen.
>>
>> With the patch you have the usbip match behavior becomes restrictive
>> which isn't desirable.
>>
>>> All of this results in the following difference in the logs of the usbip_test.sh,
>>> where the expected kernel log message "usbip-host 2-6: 2-6 is not in match_busid table... skip!"
>>> is not printed by a kernel that includes the patches in this series.
>>>
>>> --- unpatched_kernel_log.txt  2020-09-18 17:12:10.654000000 +0300
>>> +++ patched_kernel_log.txt  2020-09-18 17:12:10.654000000 +0300
>>> @@ -213,70 +213,69 @@
>>>       |__ Port 1: Dev 2, If 0, Class=Human Interface Device, Driver=usbhid, 480M
>>>   ==============================================================
>>>   modprobe usbip_host - does it work?
>>>   Should see -busid- is not in match_busid table... skip! dmesg
>>>   ==============================================================
>>> -usbip-host 2-6: 2-6 is not in match_busid table... skip!
>>>   ==============================================================
>>>
>>> Do you find this change in behaviour unacceptable?
>>
>> Yeah. This behavior isn't acceptable.
>>
>> If no, I can remove this
>>> test case from usbip_test.sh with the same patch. If yes, then there is a need
>>> for a different solution to resolve the unexpected negative interaction between
>>> Bastien's work on generic/specific USB device driver selection and usbip
>>> functionality.
>>>
>>
>> I would recommend finding a different solution. Now that you have the
>> usbip test handy, you can verify and test for regressions.
>>
>> thanks,
>> -- Shuah
> 
> Thanks for the feedback, Shuah. I spent some time looking at and instrumenting
> the code in an attempt to find another solution, but have not found one.
> 
> If the generic/specific USB driver selection functionality that Bastien Nocera
> introduced is desired to stay in the kernel, then making usbip_match more
> specific appears to be the only option for usbip to be functional without
> negatively affecting other device drivers.
> 
> Should Bastien's work be reverted until a solution to this issue is found?
> Would you (or anyone) have any suggestions? I would be happy to work further
> on resolving this issue.

Hello again,

Status update: I found a solution that should be more acceptable. It involves
some changes to the generic/specialized driver selection code and the removal
of usbip_match altogether, while preserving proper behaviour of the usbip driver,
apple-mfi-fastcharge and dummy_hcd. I intend to publish a new patch set by
tomorrow.

Vefa