linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/4] Fixes for usbip and specialised USB driver selection
@ 2020-09-22 11:06 M. Vefa Bicakci
  2020-09-22 11:07 ` [PATCH v3 1/4] Revert "usbip: Implement a match function to fix usbip" M. Vefa Bicakci
                   ` (5 more replies)
  0 siblings, 6 replies; 16+ messages in thread
From: M. Vefa Bicakci @ 2020-09-22 11:06 UTC (permalink / raw)
  To: linux-usb
  Cc: M. Vefa Bicakci, Bastien Nocera, Valentina Manea, Shuah Khan,
	Greg Kroah-Hartman, Alan Stern, syzkaller

Hello all,

This is the third version of the patch sets originally published in the
e-mail thread thread at [1]. As mentioned in the same e-mail thread with
the e-mail at [2], I was able to find a more acceptable solution to the
issue reported by Andrey Konovalov, where usbip takes over the
dummy_hcd-provided devices set up by the USB fuzzing instance of the
syzkaller fuzzer.

In summary, the approach involves:

* Removal of the usbip_match function.
* Fixing two bugs in the specialised USB driver selection code.
* Accommodating usbip by changing the logic in the specialised USB
  driver selection code, while preserving legacy/previous behaviour.

I have tested this patch set with Greg Kroah-Hartman's usb-next tree
based on v5.9-rc6 with the base commit mentioned below in this e-mail,
and I can report that usbip works as expected, with no regressions in
the usbip_test.sh self-test suite output compared to v4.14.119. I have
also verified that the apple-mfi-fastcharge driver is correctly used
when an iPhone is plugged in to my system. Finally, I can report that
Andrey Konovalov's "keyboard" test program making use of dummy_hcd,
found at [3], also works as expected.

I would appreciate your comments.

Thank you,

Vefa

[1] https://lore.kernel.org/linux-usb/CAAeHK+zOrHnxjRFs=OE8T=O9208B9HP_oo8RZpyVOZ9AJ54pAA@mail.gmail.com/
[2] https://lore.kernel.org/linux-usb/9f332d7b-e33d-ebd0-3154-246fbfb69128@runbox.com/
[3] https://github.com/xairy/raw-gadget

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>

M. Vefa Bicakci (4):
  Revert "usbip: Implement a match function to fix usbip"
  usbcore/driver: Fix specific driver selection
  usbcore/driver: Fix incorrect downcast
  usbcore/driver: Accommodate usbip

 drivers/usb/core/driver.c    | 50 ++++++++++++++++++++++++------------
 drivers/usb/usbip/stub_dev.c |  6 -----
 2 files changed, 34 insertions(+), 22 deletions(-)


base-commit: 55be22adf11b48c80ea181366685ec91a4700b7e
-- 
2.26.2


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v3 1/4] Revert "usbip: Implement a match function to fix usbip"
  2020-09-22 11:06 [PATCH v3 0/4] Fixes for usbip and specialised USB driver selection M. Vefa Bicakci
@ 2020-09-22 11:07 ` M. Vefa Bicakci
  2020-09-22 23:03   ` Shuah Khan
  2020-09-22 11:07 ` [PATCH v3 2/4] usbcore/driver: Fix specific driver selection M. Vefa Bicakci
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 16+ messages in thread
From: M. Vefa Bicakci @ 2020-09-22 11:07 UTC (permalink / raw)
  To: linux-usb
  Cc: M. Vefa Bicakci, stable, Bastien Nocera, Valentina Manea,
	Shuah Khan, Greg Kroah-Hartman, Alan Stern, syzkaller

This commit reverts commit 7a2f2974f265 ("usbip: Implement a match
function to fix usbip").

In summary, commit d5643d2249b2 ("USB: Fix device driver race")
inadvertently broke usbip functionality, which I resolved in an incorrect
manner by introducing a match function to usbip, usbip_match(), that
unconditionally returns true.

However, the usbip_match function, as is, causes usbip to take over
virtual devices used by syzkaller for USB fuzzing, which is a regression
reported by Andrey Konovalov.

Furthermore, in conjunction with the fix of another bug, handled by another
patch titled "usbcore/driver: Fix specific driver selection" in this patch
set, the usbip_match function causes unexpected USB subsystem behaviour
when the usbip_host driver is loaded. The unexpected behaviour can be
qualified as follows:
- If commit 41160802ab8e ("USB: Simplify USB ID table match") is included
  in the kernel, then all USB devices are bound to the usbip_host
  driver, which appears to the user as if all USB devices were
  disconnected.
- If the same commit (41160802ab8e) is not in the kernel (as is the case
  with v5.8.10) then all USB devices are re-probed and re-bound to their
  original device drivers, which appears to the user as a disconnection
  and re-connection of USB devices.

Please note that this commit will make usbip non-operational again,
until yet another patch in this patch set is merged, titled
"usbcore/driver: Accommodate usbip".

Reported-by: Andrey Konovalov <andreyknvl@google.com>
Link: https://lore.kernel.org/linux-usb/CAAeHK+zOrHnxjRFs=OE8T=O9208B9HP_oo8RZpyVOZ9AJ54pAA@mail.gmail.com/
Cc: <stable@vger.kernel.org> # 5.8: 41160802ab8e: USB: Simplify USB ID table match
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>

---
v3: New patch in the patch set.

    Note for stable tree maintainers: I have marked the following commit
    as a dependency of this patch, because that commit resolves a bug that
    the next commit in this patch set uncovers, where if a driver does
    not have an id_table, then its match function is not considered for
    execution at all.
      commit 41160802ab8e ("USB: Simplify USB ID table match")
---
 drivers/usb/usbip/stub_dev.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/usb/usbip/stub_dev.c b/drivers/usb/usbip/stub_dev.c
index 9d7d642022d1..2305d425e6c9 100644
--- a/drivers/usb/usbip/stub_dev.c
+++ b/drivers/usb/usbip/stub_dev.c
@@ -461,11 +461,6 @@ static void stub_disconnect(struct usb_device *udev)
 	return;
 }
 
-static bool usbip_match(struct usb_device *udev)
-{
-	return true;
-}
-
 #ifdef CONFIG_PM
 
 /* These functions need usb_port_suspend and usb_port_resume,
@@ -491,7 +486,6 @@ struct usb_device_driver stub_driver = {
 	.name		= "usbip-host",
 	.probe		= stub_probe,
 	.disconnect	= stub_disconnect,
-	.match		= usbip_match,
 #ifdef CONFIG_PM
 	.suspend	= stub_suspend,
 	.resume		= stub_resume,
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 2/4] usbcore/driver: Fix specific driver selection
  2020-09-22 11:06 [PATCH v3 0/4] Fixes for usbip and specialised USB driver selection M. Vefa Bicakci
  2020-09-22 11:07 ` [PATCH v3 1/4] Revert "usbip: Implement a match function to fix usbip" M. Vefa Bicakci
@ 2020-09-22 11:07 ` M. Vefa Bicakci
  2020-09-22 11:07 ` [PATCH v3 3/4] usbcore/driver: Fix incorrect downcast M. Vefa Bicakci
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 16+ messages in thread
From: M. Vefa Bicakci @ 2020-09-22 11:07 UTC (permalink / raw)
  To: linux-usb
  Cc: M. Vefa Bicakci, stable, Greg Kroah-Hartman, Alan Stern,
	Bastien Nocera, Shuah Khan, Valentina Manea, syzkaller

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 while
investigating the root cause of the issue reported by Andrey Konovalov,
where usbip took over syzkaller's virtual USB devices in an undesired
manner. The report is linked below.

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: Shuah Khan <shuah@kernel.org>
Cc: Valentina Manea <valentina.manea.m@gmail.com>
Cc: <syzkaller@googlegroups.com>
Signed-off-by: M. Vefa Bicakci <m.v.b@runbox.com>

---
v3: No functional changes; only commit message changes.
v2: Following Bastien Nocera's suggestion, this is a new patch,
    split from the patch published at:
      https://lore.kernel.org/linux-usb/20200917095959.174378-1-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);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 3/4] usbcore/driver: Fix incorrect downcast
  2020-09-22 11:06 [PATCH v3 0/4] Fixes for usbip and specialised USB driver selection M. Vefa Bicakci
  2020-09-22 11:07 ` [PATCH v3 1/4] Revert "usbip: Implement a match function to fix usbip" M. Vefa Bicakci
  2020-09-22 11:07 ` [PATCH v3 2/4] usbcore/driver: Fix specific driver selection M. Vefa Bicakci
@ 2020-09-22 11:07 ` M. Vefa Bicakci
  2020-09-25 14:51   ` Greg Kroah-Hartman
  2020-09-22 11:07 ` [PATCH v3 4/4] usbcore/driver: Accommodate usbip M. Vefa Bicakci
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 16+ messages in thread
From: M. Vefa Bicakci @ 2020-09-22 11:07 UTC (permalink / raw)
  To: linux-usb
  Cc: M. Vefa Bicakci, stable, Greg Kroah-Hartman, Alan Stern,
	Bastien Nocera, Shuah Khan, Valentina Manea, syzkaller

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.

This commit avoids the incorrect downcast altogether by comparing
the USB device's driver (i.e., dev->driver) to the generic USB
device driver directly. This method was suggested by Alan Stern.

This bug was found while investigating Andrey Konovalov's report
indicating usbip device driver misbehaviour with the recently merged
generic USB device driver selection feature. The report is linked
below.

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: Shuah Khan <shuah@kernel.org>
Cc: Valentina Manea <valentina.manea.m@gmail.com>
Cc: <syzkaller@googlegroups.com>
Signed-off-by: M. Vefa Bicakci <m.v.b@runbox.com>

---
v3: Simplify the device driver pointer comparison to avoid incorrect
    downcast, as suggested by Alan Stern. Minor edits to the commit
    message.

v2: Following Bastien Nocera's code review comment, use is_usb_device()
    to verify that a device is bound to a USB device driver (as opposed
    to, e.g., a USB interface driver) to avoid incorrect downcast.
---
 drivers/usb/core/driver.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index 950044a6e77f..7d90cbe063ec 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -905,21 +905,14 @@ static int usb_uevent(struct device *dev, struct kobj_uevent_env *env)
 	return 0;
 }
 
-static bool is_dev_usb_generic_driver(struct device *dev)
-{
-	struct usb_device_driver *udd = dev->driver ?
-		to_usb_device_driver(dev->driver) : NULL;
-
-	return udd == &usb_generic_driver;
-}
-
 static int __usb_bus_reprobe_drivers(struct device *dev, void *data)
 {
 	struct usb_device_driver *new_udriver = data;
 	struct usb_device *udev;
 	int ret;
 
-	if (!is_dev_usb_generic_driver(dev))
+	/* Don't reprobe if current driver isn't usb_generic_driver */
+	if (dev->driver != &usb_generic_driver.drvwrap.driver)
 		return 0;
 
 	udev = to_usb_device(dev);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 4/4] usbcore/driver: Accommodate usbip
  2020-09-22 11:06 [PATCH v3 0/4] Fixes for usbip and specialised USB driver selection M. Vefa Bicakci
                   ` (2 preceding siblings ...)
  2020-09-22 11:07 ` [PATCH v3 3/4] usbcore/driver: Fix incorrect downcast M. Vefa Bicakci
@ 2020-09-22 11:07 ` M. Vefa Bicakci
  2020-09-22 23:04   ` Shuah Khan
  2020-09-22 12:38 ` [PATCH v3 0/4] Fixes for usbip and specialised USB driver selection Andrey Konovalov
  2020-10-02  3:11 ` Brooke Basile
  5 siblings, 1 reply; 16+ messages in thread
From: M. Vefa Bicakci @ 2020-09-22 11:07 UTC (permalink / raw)
  To: linux-usb
  Cc: M. Vefa Bicakci, stable, Bastien Nocera, Valentina Manea,
	Shuah Khan, Greg Kroah-Hartman, Alan Stern, syzkaller

Commit 88b7381a939d ("USB: Select better matching USB drivers when
available") inadvertently broke usbip functionality. The commit in
question allows USB device drivers to be explicitly matched with
USB devices via the use of driver-provided identifier tables and
match functions, which is useful for a specialised device driver
to be chosen for a device that can also be handled by another,
more generic, device driver.

Prior, the USB device section of usb_device_match() had an
unconditional "return 1" statement, which allowed user-space to bind
USB devices to the usbip_host device driver, if desired. However,
the aforementioned commit changed the default/fallback return
value to zero. This breaks device drivers such as usbip_host, so
this commit restores the legacy behaviour, but only if a device
driver does not have an id_table and a match() function.

In addition, if usb_device_match is called for a device driver
and device pair where the device does not match the id_table of the
device driver in question, then the device driver will be disqualified
for the device. This allows avoiding the default case of "return 1",
which prevents undesirable probe() calls to a driver even though
its id_table did not match the device.

Finally, this commit changes the specialised-driver-to-generic-driver
transition code so that when a device driver returns -ENODEV, a more
generic device driver is only considered if the current device driver
does not have an id_table and a match() function. This ensures that
"generic" drivers such as usbip_host will not be considered specialised
device drivers and will not cause the device to be locked in to the
generic device driver, when a more specialised device driver could be
tried.

All of these changes restore usbip functionality without regressions,
ensure that the specialised/generic device driver selection logic works
as expected with the usb and apple-mfi-fastcharge drivers, and do not
negatively affect the use of devices provided by dummy_hcd.

Fixes: 88b7381a939d ("USB: Select better matching USB drivers when available")
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>

---
v3: New patch in this patch set.
    For reviewers' awareness, another option for usb_device_match would
    be as follows. This allows the driver's match() function to act
    as a fallback in case the driver has an id_table, but the id_table
    does not include the device.

	if (!udrv->id_table && !udrv->match) {
		/* Allow usbip and similar drivers to bind to
		 * arbitrary devices; let their probe functions
		 * decide.
		 */
		return 1;
	}

	if (udrv->id_table &&
	    usb_device_match_id(udev, udrv->id_table) != NULL)
		return 1;

	if (udrv->match && udrv->match(udev))
		return 1;

	return 0;
---
 drivers/usb/core/driver.c | 37 +++++++++++++++++++++++++++++++------
 1 file changed, 31 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index 7d90cbe063ec..98b7449c11f3 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -269,8 +269,30 @@ static int usb_probe_device(struct device *dev)
 	if (error)
 		return error;
 
+	/* Probe the USB device with the driver in hand, but only
+	 * defer to a generic driver in case the current USB
+	 * device driver has an id_table or a match function; i.e.,
+	 * when the device driver was explicitly matched against
+	 * a device.
+	 *
+	 * If the device driver does not have either of these,
+	 * then we assume that it can bind to any device and is
+	 * not truly a more specialized/non-generic driver, so a
+	 * return value of -ENODEV should not force the device
+	 * to be handled by the generic USB driver, as there
+	 * can still be another, more specialized, device driver.
+	 *
+	 * This accommodates the usbip driver.
+	 *
+	 * TODO: What if, in the future, there are multiple
+	 * specialized USB device drivers for a particular device?
+	 * In such cases, there is a need to try all matching
+	 * specialised device drivers prior to setting the
+	 * use_generic_driver bit.
+	 */
 	error = udriver->probe(udev);
-	if (error == -ENODEV && udriver != &usb_generic_driver) {
+	if (error == -ENODEV && udriver != &usb_generic_driver &&
+	    (udriver->id_table || udriver->match)) {
 		udev->use_generic_driver = 1;
 		return -EPROBE_DEFER;
 	}
@@ -831,14 +853,17 @@ static int usb_device_match(struct device *dev, struct device_driver *drv)
 		udev = to_usb_device(dev);
 		udrv = to_usb_device_driver(drv);
 
-		if (udrv->id_table &&
-		    usb_device_match_id(udev, udrv->id_table) != NULL) {
-			return 1;
-		}
+		if (udrv->id_table)
+			return usb_device_match_id(udev, udrv->id_table) != NULL;
 
 		if (udrv->match)
 			return udrv->match(udev);
-		return 0;
+
+		/* If the device driver under consideration does not have a
+		 * id_table or a match function, then let the driver's probe
+		 * function decide.
+		 */
+		return 1;
 
 	} else if (is_usb_interface(dev)) {
 		struct usb_interface *intf;
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 0/4] Fixes for usbip and specialised USB driver selection
  2020-09-22 11:06 [PATCH v3 0/4] Fixes for usbip and specialised USB driver selection M. Vefa Bicakci
                   ` (3 preceding siblings ...)
  2020-09-22 11:07 ` [PATCH v3 4/4] usbcore/driver: Accommodate usbip M. Vefa Bicakci
@ 2020-09-22 12:38 ` Andrey Konovalov
  2020-09-22 12:52   ` M. Vefa Bicakci
  2020-10-02  3:11 ` Brooke Basile
  5 siblings, 1 reply; 16+ messages in thread
From: Andrey Konovalov @ 2020-09-22 12:38 UTC (permalink / raw)
  To: M. Vefa Bicakci
  Cc: USB list, Bastien Nocera, Valentina Manea, Shuah Khan,
	Greg Kroah-Hartman, Alan Stern, syzkaller

On Tue, Sep 22, 2020 at 1:07 PM M. Vefa Bicakci <m.v.b@runbox.com> wrote:
>
> Hello all,
>
> This is the third version of the patch sets originally published in the
> e-mail thread thread at [1]. As mentioned in the same e-mail thread with
> the e-mail at [2], I was able to find a more acceptable solution to the
> issue reported by Andrey Konovalov, where usbip takes over the
> dummy_hcd-provided devices set up by the USB fuzzing instance of the
> syzkaller fuzzer.
>
> In summary, the approach involves:
>
> * Removal of the usbip_match function.
> * Fixing two bugs in the specialised USB driver selection code.
> * Accommodating usbip by changing the logic in the specialised USB
>   driver selection code, while preserving legacy/previous behaviour.
>
> I have tested this patch set with Greg Kroah-Hartman's usb-next tree
> based on v5.9-rc6 with the base commit mentioned below in this e-mail,
> and I can report that usbip works as expected, with no regressions in
> the usbip_test.sh self-test suite output compared to v4.14.119. I have
> also verified that the apple-mfi-fastcharge driver is correctly used
> when an iPhone is plugged in to my system. Finally, I can report that
> Andrey Konovalov's "keyboard" test program making use of dummy_hcd,
> found at [3], also works as expected.
>
> I would appreciate your comments.
>
> Thank you,
>
> Vefa
>
> [1] https://lore.kernel.org/linux-usb/CAAeHK+zOrHnxjRFs=OE8T=O9208B9HP_oo8RZpyVOZ9AJ54pAA@mail.gmail.com/
> [2] https://lore.kernel.org/linux-usb/9f332d7b-e33d-ebd0-3154-246fbfb69128@runbox.com/
> [3] https://github.com/xairy/raw-gadget
>
> 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>
>
> M. Vefa Bicakci (4):
>   Revert "usbip: Implement a match function to fix usbip"
>   usbcore/driver: Fix specific driver selection
>   usbcore/driver: Fix incorrect downcast
>   usbcore/driver: Accommodate usbip
>
>  drivers/usb/core/driver.c    | 50 ++++++++++++++++++++++++------------
>  drivers/usb/usbip/stub_dev.c |  6 -----
>  2 files changed, 34 insertions(+), 22 deletions(-)

Hi Vefa,

This fixes the issue that I've been having.

Tested-by: Andrey Konovalov <andreyknvl@google.com>

for the series.

Thank you!

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 0/4] Fixes for usbip and specialised USB driver selection
  2020-09-22 12:38 ` [PATCH v3 0/4] Fixes for usbip and specialised USB driver selection Andrey Konovalov
@ 2020-09-22 12:52   ` M. Vefa Bicakci
  0 siblings, 0 replies; 16+ messages in thread
From: M. Vefa Bicakci @ 2020-09-22 12:52 UTC (permalink / raw)
  To: Andrey Konovalov
  Cc: USB list, Bastien Nocera, Valentina Manea, Shuah Khan,
	Greg Kroah-Hartman, Alan Stern, syzkaller

On 22/09/2020 15.38, Andrey Konovalov wrote:
> On Tue, Sep 22, 2020 at 1:07 PM M. Vefa Bicakci <m.v.b@runbox.com> wrote:
>>
>> Hello all,
>>
>> This is the third version of the patch sets originally published in the
>> e-mail thread thread at [1]. As mentioned in the same e-mail thread with
>> the e-mail at [2], I was able to find a more acceptable solution to the
>> issue reported by Andrey Konovalov, where usbip takes over the
>> dummy_hcd-provided devices set up by the USB fuzzing instance of the
>> syzkaller fuzzer.
>>
>> In summary, the approach involves:
>>
>> * Removal of the usbip_match function.
>> * Fixing two bugs in the specialised USB driver selection code.
>> * Accommodating usbip by changing the logic in the specialised USB
>>    driver selection code, while preserving legacy/previous behaviour.
>>
>> I have tested this patch set with Greg Kroah-Hartman's usb-next tree
>> based on v5.9-rc6 with the base commit mentioned below in this e-mail,
>> and I can report that usbip works as expected, with no regressions in
>> the usbip_test.sh self-test suite output compared to v4.14.119. I have
>> also verified that the apple-mfi-fastcharge driver is correctly used
>> when an iPhone is plugged in to my system. Finally, I can report that
>> Andrey Konovalov's "keyboard" test program making use of dummy_hcd,
>> found at [3], also works as expected.
>>
>> I would appreciate your comments.
>>
>> Thank you,
>>
>> Vefa
>>
>> [1] https://lore.kernel.org/linux-usb/CAAeHK+zOrHnxjRFs=OE8T=O9208B9HP_oo8RZpyVOZ9AJ54pAA@mail.gmail.com/
>> [2] https://lore.kernel.org/linux-usb/9f332d7b-e33d-ebd0-3154-246fbfb69128@runbox.com/
>> [3] https://github.com/xairy/raw-gadget
>>
>> 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>
>>
>> M. Vefa Bicakci (4):
>>    Revert "usbip: Implement a match function to fix usbip"
>>    usbcore/driver: Fix specific driver selection
>>    usbcore/driver: Fix incorrect downcast
>>    usbcore/driver: Accommodate usbip
>>
>>   drivers/usb/core/driver.c    | 50 ++++++++++++++++++++++++------------
>>   drivers/usb/usbip/stub_dev.c |  6 -----
>>   2 files changed, 34 insertions(+), 22 deletions(-)
> 
> Hi Vefa,
> 
> This fixes the issue that I've been having.
> 
> Tested-by: Andrey Konovalov <andreyknvl@google.com>
> 
> for the series.
> 
> Thank you!

Thank you as well, Andrey, for confirming that these patches
resolve the negative interaction between usbip and dummy_hcd!
I appreciate your effort in testing the patches.

Vefa

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 1/4] Revert "usbip: Implement a match function to fix usbip"
  2020-09-22 11:07 ` [PATCH v3 1/4] Revert "usbip: Implement a match function to fix usbip" M. Vefa Bicakci
@ 2020-09-22 23:03   ` Shuah Khan
  0 siblings, 0 replies; 16+ messages in thread
From: Shuah Khan @ 2020-09-22 23:03 UTC (permalink / raw)
  To: M. Vefa Bicakci, linux-usb
  Cc: stable, Bastien Nocera, Valentina Manea, Shuah Khan,
	Greg Kroah-Hartman, Alan Stern, syzkaller, Shuah Khan

On 9/22/20 5:07 AM, M. Vefa Bicakci wrote:
> This commit reverts commit 7a2f2974f265 ("usbip: Implement a match
> function to fix usbip").
> 
> In summary, commit d5643d2249b2 ("USB: Fix device driver race")
> inadvertently broke usbip functionality, which I resolved in an incorrect
> manner by introducing a match function to usbip, usbip_match(), that
> unconditionally returns true.
> 
> However, the usbip_match function, as is, causes usbip to take over
> virtual devices used by syzkaller for USB fuzzing, which is a regression
> reported by Andrey Konovalov.
> 
> Furthermore, in conjunction with the fix of another bug, handled by another
> patch titled "usbcore/driver: Fix specific driver selection" in this patch
> set, the usbip_match function causes unexpected USB subsystem behaviour
> when the usbip_host driver is loaded. The unexpected behaviour can be
> qualified as follows:
> - If commit 41160802ab8e ("USB: Simplify USB ID table match") is included
>    in the kernel, then all USB devices are bound to the usbip_host
>    driver, which appears to the user as if all USB devices were
>    disconnected.
> - If the same commit (41160802ab8e) is not in the kernel (as is the case
>    with v5.8.10) then all USB devices are re-probed and re-bound to their
>    original device drivers, which appears to the user as a disconnection
>    and re-connection of USB devices.
> 
> Please note that this commit will make usbip non-operational again,
> until yet another patch in this patch set is merged, titled
> "usbcore/driver: Accommodate usbip".
> 
> Reported-by: Andrey Konovalov <andreyknvl@google.com>
> Link: https://lore.kernel.org/linux-usb/CAAeHK+zOrHnxjRFs=OE8T=O9208B9HP_oo8RZpyVOZ9AJ54pAA@mail.gmail.com/
> Cc: <stable@vger.kernel.org> # 5.8: 41160802ab8e: USB: Simplify USB ID table match
> 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>
> 
> ---
> v3: New patch in the patch set.
> 
>      Note for stable tree maintainers: I have marked the following commit
>      as a dependency of this patch, because that commit resolves a bug that
>      the next commit in this patch set uncovers, where if a driver does
>      not have an id_table, then its match function is not considered for
>      execution at all.
>        commit 41160802ab8e ("USB: Simplify USB ID table match")
> ---
>   drivers/usb/usbip/stub_dev.c | 6 ------
>   1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/usb/usbip/stub_dev.c b/drivers/usb/usbip/stub_dev.c
> index 9d7d642022d1..2305d425e6c9 100644
> --- a/drivers/usb/usbip/stub_dev.c
> +++ b/drivers/usb/usbip/stub_dev.c
> @@ -461,11 +461,6 @@ static void stub_disconnect(struct usb_device *udev)
>   	return;
>   }
>   
> -static bool usbip_match(struct usb_device *udev)
> -{
> -	return true;
> -}
> -
>   #ifdef CONFIG_PM
>   
>   /* These functions need usb_port_suspend and usb_port_resume,
> @@ -491,7 +486,6 @@ struct usb_device_driver stub_driver = {
>   	.name		= "usbip-host",
>   	.probe		= stub_probe,
>   	.disconnect	= stub_disconnect,
> -	.match		= usbip_match,
>   #ifdef CONFIG_PM
>   	.suspend	= stub_suspend,
>   	.resume		= stub_resume,
> 

Thank you for finding a solution that works for usbip

Acked-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 4/4] usbcore/driver: Accommodate usbip
  2020-09-22 11:07 ` [PATCH v3 4/4] usbcore/driver: Accommodate usbip M. Vefa Bicakci
@ 2020-09-22 23:04   ` Shuah Khan
  2020-09-23  6:08     ` M. Vefa Bicakci
  0 siblings, 1 reply; 16+ messages in thread
From: Shuah Khan @ 2020-09-22 23:04 UTC (permalink / raw)
  To: M. Vefa Bicakci, linux-usb
  Cc: stable, Bastien Nocera, Valentina Manea, Shuah Khan,
	Greg Kroah-Hartman, Alan Stern, syzkaller, Shuah Khan

On 9/22/20 5:07 AM, M. Vefa Bicakci wrote:
> Commit 88b7381a939d ("USB: Select better matching USB drivers when
> available") inadvertently broke usbip functionality. The commit in
> question allows USB device drivers to be explicitly matched with
> USB devices via the use of driver-provided identifier tables and
> match functions, which is useful for a specialised device driver
> to be chosen for a device that can also be handled by another,
> more generic, device driver.
> 
> Prior, the USB device section of usb_device_match() had an
> unconditional "return 1" statement, which allowed user-space to bind
> USB devices to the usbip_host device driver, if desired. However,
> the aforementioned commit changed the default/fallback return
> value to zero. This breaks device drivers such as usbip_host, so
> this commit restores the legacy behaviour, but only if a device
> driver does not have an id_table and a match() function.
> 
> In addition, if usb_device_match is called for a device driver
> and device pair where the device does not match the id_table of the
> device driver in question, then the device driver will be disqualified
> for the device. This allows avoiding the default case of "return 1",
> which prevents undesirable probe() calls to a driver even though
> its id_table did not match the device.
> 
> Finally, this commit changes the specialised-driver-to-generic-driver
> transition code so that when a device driver returns -ENODEV, a more
> generic device driver is only considered if the current device driver
> does not have an id_table and a match() function. This ensures that
> "generic" drivers such as usbip_host will not be considered specialised
> device drivers and will not cause the device to be locked in to the
> generic device driver, when a more specialised device driver could be
> tried.
> 
> All of these changes restore usbip functionality without regressions,
> ensure that the specialised/generic device driver selection logic works
> as expected with the usb and apple-mfi-fastcharge drivers, and do not
> negatively affect the use of devices provided by dummy_hcd.
> 
> Fixes: 88b7381a939d ("USB: Select better matching USB drivers when available")
> 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>
> 
> ---
> v3: New patch in this patch set.
>      For reviewers' awareness, another option for usb_device_match would
>      be as follows. This allows the driver's match() function to act
>      as a fallback in case the driver has an id_table, but the id_table
>      does not include the device.
> 
> 	if (!udrv->id_table && !udrv->match) {
> 		/* Allow usbip and similar drivers to bind to
> 		 * arbitrary devices; let their probe functions
> 		 * decide.
> 		 */
> 		return 1;
> 	}
> 
> 	if (udrv->id_table &&
> 	    usb_device_match_id(udev, udrv->id_table) != NULL)
> 		return 1;
> 
> 	if (udrv->match && udrv->match(udev))
> 		return 1;
> 
> 	return 0;
> ---
>   drivers/usb/core/driver.c | 37 +++++++++++++++++++++++++++++++------
>   1 file changed, 31 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index 7d90cbe063ec..98b7449c11f3 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -269,8 +269,30 @@ static int usb_probe_device(struct device *dev)
>   	if (error)
>   		return error;
>   
> +	/* Probe the USB device with the driver in hand, but only
> +	 * defer to a generic driver in case the current USB
> +	 * device driver has an id_table or a match function; i.e.,
> +	 * when the device driver was explicitly matched against
> +	 * a device.
> +	 *
> +	 * If the device driver does not have either of these,
> +	 * then we assume that it can bind to any device and is
> +	 * not truly a more specialized/non-generic driver, so a
> +	 * return value of -ENODEV should not force the device
> +	 * to be handled by the generic USB driver, as there
> +	 * can still be another, more specialized, device driver.
> +	 *
> +	 * This accommodates the usbip driver.
> +	 *
> +	 * TODO: What if, in the future, there are multiple
> +	 * specialized USB device drivers for a particular device?
> +	 * In such cases, there is a need to try all matching
> +	 * specialised device drivers prior to setting the
> +	 * use_generic_driver bit.
> +	 */
>   	error = udriver->probe(udev);
> -	if (error == -ENODEV && udriver != &usb_generic_driver) {
> +	if (error == -ENODEV && udriver != &usb_generic_driver &&
> +	    (udriver->id_table || udriver->match)) {
>   		udev->use_generic_driver = 1;
>   		return -EPROBE_DEFER;
>   	}
> @@ -831,14 +853,17 @@ static int usb_device_match(struct device *dev, struct device_driver *drv)
>   		udev = to_usb_device(dev);
>   		udrv = to_usb_device_driver(drv);
>   
> -		if (udrv->id_table &&
> -		    usb_device_match_id(udev, udrv->id_table) != NULL) {
> -			return 1;
> -		}
> +		if (udrv->id_table)
> +			return usb_device_match_id(udev, udrv->id_table) != NULL;
>   
>   		if (udrv->match)
>   			return udrv->match(udev);
> -		return 0;
> +
> +		/* If the device driver under consideration does not have a
> +		 * id_table or a match function, then let the driver's probe
> +		 * function decide.
> +		 */
> +		return 1;
>   
>   	} else if (is_usb_interface(dev)) {
>   		struct usb_interface *intf;
> 

Thank you for finding a solution that works for usbip case.

Acked-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 4/4] usbcore/driver: Accommodate usbip
  2020-09-22 23:04   ` Shuah Khan
@ 2020-09-23  6:08     ` M. Vefa Bicakci
  0 siblings, 0 replies; 16+ messages in thread
From: M. Vefa Bicakci @ 2020-09-23  6:08 UTC (permalink / raw)
  To: Shuah Khan, linux-usb
  Cc: stable, Bastien Nocera, Valentina Manea, Shuah Khan,
	Greg Kroah-Hartman, Alan Stern, syzkaller

On 23/09/2020 02.04, Shuah Khan wrote:
> On 9/22/20 5:07 AM, M. Vefa Bicakci wrote:
>> Commit 88b7381a939d ("USB: Select better matching USB drivers when
>> available") inadvertently broke usbip functionality. The commit in
>> question allows USB device drivers to be explicitly matched with
>> USB devices via the use of driver-provided identifier tables and
>> match functions, which is useful for a specialised device driver
>> to be chosen for a device that can also be handled by another,
>> more generic, device driver.
>>
>> Prior, the USB device section of usb_device_match() had an
>> unconditional "return 1" statement, which allowed user-space to bind
>> USB devices to the usbip_host device driver, if desired. However,
>> the aforementioned commit changed the default/fallback return
>> value to zero. This breaks device drivers such as usbip_host, so
>> this commit restores the legacy behaviour, but only if a device
>> driver does not have an id_table and a match() function.
>>
>> In addition, if usb_device_match is called for a device driver
>> and device pair where the device does not match the id_table of the
>> device driver in question, then the device driver will be disqualified
>> for the device. This allows avoiding the default case of "return 1",
>> which prevents undesirable probe() calls to a driver even though
>> its id_table did not match the device.
>>
>> Finally, this commit changes the specialised-driver-to-generic-driver
>> transition code so that when a device driver returns -ENODEV, a more
>> generic device driver is only considered if the current device driver
>> does not have an id_table and a match() function. This ensures that
>> "generic" drivers such as usbip_host will not be considered specialised
>> device drivers and will not cause the device to be locked in to the
>> generic device driver, when a more specialised device driver could be
>> tried.
>>
>> All of these changes restore usbip functionality without regressions,
>> ensure that the specialised/generic device driver selection logic works
>> as expected with the usb and apple-mfi-fastcharge drivers, and do not
>> negatively affect the use of devices provided by dummy_hcd.
>>
>> Fixes: 88b7381a939d ("USB: Select better matching USB drivers when available")
>> 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>
>>
>> ---
>> v3: New patch in this patch set.
>>      For reviewers' awareness, another option for usb_device_match would
>>      be as follows. This allows the driver's match() function to act
>>      as a fallback in case the driver has an id_table, but the id_table
>>      does not include the device.
>>
>>     if (!udrv->id_table && !udrv->match) {
>>         /* Allow usbip and similar drivers to bind to
>>          * arbitrary devices; let their probe functions
>>          * decide.
>>          */
>>         return 1;
>>     }
>>
>>     if (udrv->id_table &&
>>         usb_device_match_id(udev, udrv->id_table) != NULL)
>>         return 1;
>>
>>     if (udrv->match && udrv->match(udev))
>>         return 1;
>>
>>     return 0;
>> ---
>>   drivers/usb/core/driver.c | 37 +++++++++++++++++++++++++++++++------
>>   1 file changed, 31 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
>> index 7d90cbe063ec..98b7449c11f3 100644
>> --- a/drivers/usb/core/driver.c
>> +++ b/drivers/usb/core/driver.c
>> @@ -269,8 +269,30 @@ static int usb_probe_device(struct device *dev)
>>       if (error)
>>           return error;
>> +    /* Probe the USB device with the driver in hand, but only
>> +     * defer to a generic driver in case the current USB
>> +     * device driver has an id_table or a match function; i.e.,
>> +     * when the device driver was explicitly matched against
>> +     * a device.
>> +     *
>> +     * If the device driver does not have either of these,
>> +     * then we assume that it can bind to any device and is
>> +     * not truly a more specialized/non-generic driver, so a
>> +     * return value of -ENODEV should not force the device
>> +     * to be handled by the generic USB driver, as there
>> +     * can still be another, more specialized, device driver.
>> +     *
>> +     * This accommodates the usbip driver.
>> +     *
>> +     * TODO: What if, in the future, there are multiple
>> +     * specialized USB device drivers for a particular device?
>> +     * In such cases, there is a need to try all matching
>> +     * specialised device drivers prior to setting the
>> +     * use_generic_driver bit.
>> +     */
>>       error = udriver->probe(udev);
>> -    if (error == -ENODEV && udriver != &usb_generic_driver) {
>> +    if (error == -ENODEV && udriver != &usb_generic_driver &&
>> +        (udriver->id_table || udriver->match)) {
>>           udev->use_generic_driver = 1;
>>           return -EPROBE_DEFER;
>>       }
>> @@ -831,14 +853,17 @@ static int usb_device_match(struct device *dev, struct device_driver *drv)
>>           udev = to_usb_device(dev);
>>           udrv = to_usb_device_driver(drv);
>> -        if (udrv->id_table &&
>> -            usb_device_match_id(udev, udrv->id_table) != NULL) {
>> -            return 1;
>> -        }
>> +        if (udrv->id_table)
>> +            return usb_device_match_id(udev, udrv->id_table) != NULL;
>>           if (udrv->match)
>>               return udrv->match(udev);
>> -        return 0;
>> +
>> +        /* If the device driver under consideration does not have a
>> +         * id_table or a match function, then let the driver's probe
>> +         * function decide.
>> +         */
>> +        return 1;
>>       } else if (is_usb_interface(dev)) {
>>           struct usb_interface *intf;
>>
> 
> Thank you for finding a solution that works for usbip case.
> 
> Acked-by: Shuah Khan <skhan@linuxfoundation.org>
> 
> thanks,
> -- Shuah

Thank you for the feedback, Shuah!

Vefa

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 3/4] usbcore/driver: Fix incorrect downcast
  2020-09-22 11:07 ` [PATCH v3 3/4] usbcore/driver: Fix incorrect downcast M. Vefa Bicakci
@ 2020-09-25 14:51   ` Greg Kroah-Hartman
  2020-09-25 16:31     ` M. Vefa Bicakci
  0 siblings, 1 reply; 16+ messages in thread
From: Greg Kroah-Hartman @ 2020-09-25 14:51 UTC (permalink / raw)
  To: M. Vefa Bicakci
  Cc: linux-usb, stable, Alan Stern, Bastien Nocera, Shuah Khan,
	Valentina Manea, syzkaller

On Tue, Sep 22, 2020 at 02:07:02PM +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.
> 
> This commit avoids the incorrect downcast altogether by comparing
> the USB device's driver (i.e., dev->driver) to the generic USB
> device driver directly. This method was suggested by Alan Stern.
> 
> This bug was found while investigating Andrey Konovalov's report
> indicating usbip device driver misbehaviour with the recently merged
> generic USB device driver selection feature. The report is linked
> below.
> 
> Fixes: d5643d2249 ("USB: Fix device driver race")

Nit, this should have been:
	Fixes: d5643d2249b2 ("USB: Fix device driver race")

I'll go fix it up as my scripts are rejecting it as-is...

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 3/4] usbcore/driver: Fix incorrect downcast
  2020-09-25 14:51   ` Greg Kroah-Hartman
@ 2020-09-25 16:31     ` M. Vefa Bicakci
  2020-09-26  5:37       ` Greg Kroah-Hartman
  0 siblings, 1 reply; 16+ messages in thread
From: M. Vefa Bicakci @ 2020-09-25 16:31 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-usb, stable, Alan Stern, Bastien Nocera, Shuah Khan,
	Valentina Manea, syzkaller

On 9/25/20 5:51 PM, Greg Kroah-Hartman wrote:
> On Tue, Sep 22, 2020 at 02:07:02PM +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.
>>
>> This commit avoids the incorrect downcast altogether by comparing
>> the USB device's driver (i.e., dev->driver) to the generic USB
>> device driver directly. This method was suggested by Alan Stern.
>>
>> This bug was found while investigating Andrey Konovalov's report
>> indicating usbip device driver misbehaviour with the recently merged
>> generic USB device driver selection feature. The report is linked
>> below.
>>
>> Fixes: d5643d2249 ("USB: Fix device driver race")
> 
> Nit, this should have been:
> 	Fixes: d5643d2249b2 ("USB: Fix device driver race")
> 
> I'll go fix it up as my scripts are rejecting it as-is...

Noted; sorry for missing this. I will use 12 characters from now on.

I also wanted to thank you for committing the patches.

Vefa

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 3/4] usbcore/driver: Fix incorrect downcast
  2020-09-25 16:31     ` M. Vefa Bicakci
@ 2020-09-26  5:37       ` Greg Kroah-Hartman
  0 siblings, 0 replies; 16+ messages in thread
From: Greg Kroah-Hartman @ 2020-09-26  5:37 UTC (permalink / raw)
  To: M. Vefa Bicakci
  Cc: linux-usb, stable, Alan Stern, Bastien Nocera, Shuah Khan,
	Valentina Manea, syzkaller

On Fri, Sep 25, 2020 at 07:31:44PM +0300, M. Vefa Bicakci wrote:
> On 9/25/20 5:51 PM, Greg Kroah-Hartman wrote:
> > On Tue, Sep 22, 2020 at 02:07:02PM +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.
> > > 
> > > This commit avoids the incorrect downcast altogether by comparing
> > > the USB device's driver (i.e., dev->driver) to the generic USB
> > > device driver directly. This method was suggested by Alan Stern.
> > > 
> > > This bug was found while investigating Andrey Konovalov's report
> > > indicating usbip device driver misbehaviour with the recently merged
> > > generic USB device driver selection feature. The report is linked
> > > below.
> > > 
> > > Fixes: d5643d2249 ("USB: Fix device driver race")
> > 
> > Nit, this should have been:
> > 	Fixes: d5643d2249b2 ("USB: Fix device driver race")
> > 
> > I'll go fix it up as my scripts are rejecting it as-is...
> 
> Noted; sorry for missing this. I will use 12 characters from now on.

No worries.  There's a nice git configuration line you can do that is
documented in the submitting patches file in the kernel documentation
directory, if you want to use that.  I have a alias that does it easily
as well as it gets annoying to have to type:

	git show -s --abbrev-commit --abbrev=12 --pretty=format:"%h (\"%s\")%n"

A lot :)

> I also wanted to thank you for committing the patches.

Thanks for fixing this all up!

greg k-h

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 0/4] Fixes for usbip and specialised USB driver selection
  2020-09-22 11:06 [PATCH v3 0/4] Fixes for usbip and specialised USB driver selection M. Vefa Bicakci
                   ` (4 preceding siblings ...)
  2020-09-22 12:38 ` [PATCH v3 0/4] Fixes for usbip and specialised USB driver selection Andrey Konovalov
@ 2020-10-02  3:11 ` Brooke Basile
  2020-10-02  9:00   ` M. Vefa Bicakci
  5 siblings, 1 reply; 16+ messages in thread
From: Brooke Basile @ 2020-10-02  3:11 UTC (permalink / raw)
  To: M. Vefa Bicakci, linux-usb
  Cc: Bastien Nocera, Valentina Manea, Shuah Khan, Greg Kroah-Hartman,
	Alan Stern, syzkaller

On 9/22/20 7:06 AM, M. Vefa Bicakci wrote:
> Hello all,
> 
> This is the third version of the patch sets originally published in the
> e-mail thread thread at [1]. As mentioned in the same e-mail thread with
> the e-mail at [2], I was able to find a more acceptable solution to the
> issue reported by Andrey Konovalov, where usbip takes over the
> dummy_hcd-provided devices set up by the USB fuzzing instance of the
> syzkaller fuzzer.
> 
> In summary, the approach involves:
> 
> * Removal of the usbip_match function.
> * Fixing two bugs in the specialised USB driver selection code.
> * Accommodating usbip by changing the logic in the specialised USB
>    driver selection code, while preserving legacy/previous behaviour.
> 
> I have tested this patch set with Greg Kroah-Hartman's usb-next tree
> based on v5.9-rc6 with the base commit mentioned below in this e-mail,
> and I can report that usbip works as expected, with no regressions in
> the usbip_test.sh self-test suite output compared to v4.14.119. I have
> also verified that the apple-mfi-fastcharge driver is correctly used
> when an iPhone is plugged in to my system. Finally, I can report that
> Andrey Konovalov's "keyboard" test program making use of dummy_hcd,
> found at [3], also works as expected.
> 
> I would appreciate your comments.
> 
> Thank you,
> 
> Vefa
> 
> [1] https://lore.kernel.org/linux-usb/CAAeHK+zOrHnxjRFs=OE8T=O9208B9HP_oo8RZpyVOZ9AJ54pAA@mail.gmail.com/
> [2] https://lore.kernel.org/linux-usb/9f332d7b-e33d-ebd0-3154-246fbfb69128@runbox.com/
> [3] https://github.com/xairy/raw-gadget
> 
> 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>
> 
> M. Vefa Bicakci (4):
>    Revert "usbip: Implement a match function to fix usbip"
>    usbcore/driver: Fix specific driver selection
>    usbcore/driver: Fix incorrect downcast
>    usbcore/driver: Accommodate usbip
> 
>   drivers/usb/core/driver.c    | 50 ++++++++++++++++++++++++------------
>   drivers/usb/usbip/stub_dev.c |  6 -----
>   2 files changed, 34 insertions(+), 22 deletions(-)
> 
> 
> base-commit: 55be22adf11b48c80ea181366685ec91a4700b7e
> 

Hi,

I ran into this issue when trying to use two different FTDI serial TTL 
cables on my laptop, running 5.9-rc7.

This patch set fixes the issue.

Oddly, I was unable to reproduce the issue on another laptop, also 
running 5.9-rc7.

Tested-by: Brooke Basile <brookebasile@gmail.com>

Thank you!
Brooke Basile


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 0/4] Fixes for usbip and specialised USB driver selection
  2020-10-02  3:11 ` Brooke Basile
@ 2020-10-02  9:00   ` M. Vefa Bicakci
  2020-10-02  9:05     ` Brooke Basile
  0 siblings, 1 reply; 16+ messages in thread
From: M. Vefa Bicakci @ 2020-10-02  9:00 UTC (permalink / raw)
  To: Brooke Basile, linux-usb
  Cc: Bastien Nocera, Valentina Manea, Shuah Khan, Greg Kroah-Hartman,
	Alan Stern, syzkaller

On 10/2/20 6:11 AM, Brooke Basile wrote:
> On 9/22/20 7:06 AM, M. Vefa Bicakci wrote:
>> Hello all,
>>
>> This is the third version of the patch sets originally published in the
>> e-mail thread thread at [1]. As mentioned in the same e-mail thread with
>> the e-mail at [2], I was able to find a more acceptable solution to the
>> issue reported by Andrey Konovalov, where usbip takes over the
>> dummy_hcd-provided devices set up by the USB fuzzing instance of the
>> syzkaller fuzzer.
>>
>> In summary, the approach involves:
>>
>> * Removal of the usbip_match function.
>> * Fixing two bugs in the specialised USB driver selection code.
>> * Accommodating usbip by changing the logic in the specialised USB
>>    driver selection code, while preserving legacy/previous behaviour.
>>
>> I have tested this patch set with Greg Kroah-Hartman's usb-next tree
>> based on v5.9-rc6 with the base commit mentioned below in this e-mail,
>> and I can report that usbip works as expected, with no regressions in
>> the usbip_test.sh self-test suite output compared to v4.14.119. I have
>> also verified that the apple-mfi-fastcharge driver is correctly used
>> when an iPhone is plugged in to my system. Finally, I can report that
>> Andrey Konovalov's "keyboard" test program making use of dummy_hcd,
>> found at [3], also works as expected.
>>
>> I would appreciate your comments.
>>
>> Thank you,
>>
>> Vefa
>>
>> [1] https://lore.kernel.org/linux-usb/CAAeHK+zOrHnxjRFs=OE8T=O9208B9HP_oo8RZpyVOZ9AJ54pAA@mail.gmail.com/
>> [2] https://lore.kernel.org/linux-usb/9f332d7b-e33d-ebd0-3154-246fbfb69128@runbox.com/
>> [3] https://github.com/xairy/raw-gadget
>>
>> 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>
>>
>> M. Vefa Bicakci (4):
>>    Revert "usbip: Implement a match function to fix usbip"
>>    usbcore/driver: Fix specific driver selection
>>    usbcore/driver: Fix incorrect downcast
>>    usbcore/driver: Accommodate usbip
>>
>>   drivers/usb/core/driver.c    | 50 ++++++++++++++++++++++++------------
>>   drivers/usb/usbip/stub_dev.c |  6 -----
>>   2 files changed, 34 insertions(+), 22 deletions(-)
>>
>>
>> base-commit: 55be22adf11b48c80ea181366685ec91a4700b7e
>>
> 
> Hi,
> 
> I ran into this issue when trying to use two different FTDI serial TTL cables on my laptop, running 5.9-rc7.
> 
> This patch set fixes the issue.
> 
> Oddly, I was unable to reproduce the issue on another laptop, also running 5.9-rc7.
> 
> Tested-by: Brooke Basile <brookebasile@gmail.com>
> 
> Thank you!
> Brooke Basile

Hello Brooke,

Thank you for the feedback! Greg Kroah-Hartman has committed the patches
to the usb-linus branch of the USB git tree about a week ago, so it may
unfortunately be a bit late to add your Tested-by tag to the patch series.
Nevertheless, I appreciate your success report!

In case you are interested, the committed patches can be seen here:
   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/log/?h=usb-linus&id=3fce39601a1a34d940cf62858ee01ed9dac5d459

Thanks again,

Vefa

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 0/4] Fixes for usbip and specialised USB driver selection
  2020-10-02  9:00   ` M. Vefa Bicakci
@ 2020-10-02  9:05     ` Brooke Basile
  0 siblings, 0 replies; 16+ messages in thread
From: Brooke Basile @ 2020-10-02  9:05 UTC (permalink / raw)
  To: M. Vefa Bicakci, linux-usb
  Cc: Bastien Nocera, Valentina Manea, Shuah Khan, Greg Kroah-Hartman,
	Alan Stern, syzkaller

On 10/2/20 5:00 AM, M. Vefa Bicakci wrote:
> On 10/2/20 6:11 AM, Brooke Basile wrote:
>> On 9/22/20 7:06 AM, M. Vefa Bicakci wrote:
>>> Hello all,
>>>
>>> This is the third version of the patch sets originally published in the
>>> e-mail thread thread at [1]. As mentioned in the same e-mail thread with
>>> the e-mail at [2], I was able to find a more acceptable solution to the
>>> issue reported by Andrey Konovalov, where usbip takes over the
>>> dummy_hcd-provided devices set up by the USB fuzzing instance of the
>>> syzkaller fuzzer.
>>>
>>> In summary, the approach involves:
>>>
>>> * Removal of the usbip_match function.
>>> * Fixing two bugs in the specialised USB driver selection code.
>>> * Accommodating usbip by changing the logic in the specialised USB
>>>    driver selection code, while preserving legacy/previous behaviour.
>>>
>>> I have tested this patch set with Greg Kroah-Hartman's usb-next tree
>>> based on v5.9-rc6 with the base commit mentioned below in this e-mail,
>>> and I can report that usbip works as expected, with no regressions in
>>> the usbip_test.sh self-test suite output compared to v4.14.119. I have
>>> also verified that the apple-mfi-fastcharge driver is correctly used
>>> when an iPhone is plugged in to my system. Finally, I can report that
>>> Andrey Konovalov's "keyboard" test program making use of dummy_hcd,
>>> found at [3], also works as expected.
>>>
>>> I would appreciate your comments.
>>>
>>> Thank you,
>>>
>>> Vefa
>>>
>>> [1] 
>>> https://lore.kernel.org/linux-usb/CAAeHK+zOrHnxjRFs=OE8T=O9208B9HP_oo8RZpyVOZ9AJ54pAA@mail.gmail.com/ 
>>>
>>> [2] 
>>> https://lore.kernel.org/linux-usb/9f332d7b-e33d-ebd0-3154-246fbfb69128@runbox.com/ 
>>>
>>> [3] https://github.com/xairy/raw-gadget
>>>
>>> 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>
>>>
>>> M. Vefa Bicakci (4):
>>>    Revert "usbip: Implement a match function to fix usbip"
>>>    usbcore/driver: Fix specific driver selection
>>>    usbcore/driver: Fix incorrect downcast
>>>    usbcore/driver: Accommodate usbip
>>>
>>>   drivers/usb/core/driver.c    | 50 ++++++++++++++++++++++++------------
>>>   drivers/usb/usbip/stub_dev.c |  6 -----
>>>   2 files changed, 34 insertions(+), 22 deletions(-)
>>>
>>>
>>> base-commit: 55be22adf11b48c80ea181366685ec91a4700b7e
>>>
>>
>> Hi,
>>
>> I ran into this issue when trying to use two different FTDI serial TTL 
>> cables on my laptop, running 5.9-rc7.
>>
>> This patch set fixes the issue.
>>
>> Oddly, I was unable to reproduce the issue on another laptop, also 
>> running 5.9-rc7.
>>
>> Tested-by: Brooke Basile <brookebasile@gmail.com>
>>
>> Thank you!
>> Brooke Basile
> 
> Hello Brooke,
> 
> Thank you for the feedback! Greg Kroah-Hartman has committed the patches
> to the usb-linus branch of the USB git tree about a week ago, so it may
> unfortunately be a bit late to add your Tested-by tag to the patch series.
> Nevertheless, I appreciate your success report!
> 
> In case you are interested, the committed patches can be seen here:
>    
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/log/?h=usb-linus&id=3fce39601a1a34d940cf62858ee01ed9dac5d459 
> 
> 
> Thanks again,
> 
> Vefa
> 

Vefa,

Ah, no worries at all!  Sorry, I didn't see this on LKML so I assumed it 
hadn't been merged yet.
Thanks for the link!

Best,
Brooke Basile



^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2020-10-02  9:05 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-22 11:06 [PATCH v3 0/4] Fixes for usbip and specialised USB driver selection M. Vefa Bicakci
2020-09-22 11:07 ` [PATCH v3 1/4] Revert "usbip: Implement a match function to fix usbip" M. Vefa Bicakci
2020-09-22 23:03   ` Shuah Khan
2020-09-22 11:07 ` [PATCH v3 2/4] usbcore/driver: Fix specific driver selection M. Vefa Bicakci
2020-09-22 11:07 ` [PATCH v3 3/4] usbcore/driver: Fix incorrect downcast M. Vefa Bicakci
2020-09-25 14:51   ` Greg Kroah-Hartman
2020-09-25 16:31     ` M. Vefa Bicakci
2020-09-26  5:37       ` Greg Kroah-Hartman
2020-09-22 11:07 ` [PATCH v3 4/4] usbcore/driver: Accommodate usbip M. Vefa Bicakci
2020-09-22 23:04   ` Shuah Khan
2020-09-23  6:08     ` M. Vefa Bicakci
2020-09-22 12:38 ` [PATCH v3 0/4] Fixes for usbip and specialised USB driver selection Andrey Konovalov
2020-09-22 12:52   ` M. Vefa Bicakci
2020-10-02  3:11 ` Brooke Basile
2020-10-02  9:00   ` M. Vefa Bicakci
2020-10-02  9:05     ` Brooke Basile

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).