Re: [PATCH v2 3/3] USB: cdc-acm: fix TIOCGSERIAL implementation
Am Donnerstag, den 08.04.2021, 15:16 +0200 schrieb Johan Hovold:
> TIOCSSERIAL is a horrid, underspecified, legacy interface which for most
> serial devices is only useful for setting the close_delay and
> closing_wait parameters.
>
> The xmit_fifo_size parameter could be used to set the hardware transmit
> fifo size of a legacy UART when it could not be detected, but the
> interface is limited to eight bits and should be left unset when it is
> not used.
>
> Similarly, baud_base could be used to set the UART base clock when it
> could not be detected, but might as well be left unset when it is not
> known (which is the case for CDC).
>
> Fix the cdc-acm TIOCGSERIAL implementation by dropping its custom
> interpretation of the unused xmit_fifo_size and baud_base fields, which
> overflowed the former with the URB buffer size and set the latter to the
> current line speed. Also return the port line number, which is the only
> other value used besides the close parameters.
>
> Note that the current line speed can still be retrieved through the
> standard termios interfaces.
>
> Fixes: 18c75720e667 ("USB: allow users to run setserial with cdc-acm")
> Signed-off-by: Johan Hovold
Acked-by: Oliver Neukum
Re: [PATCH v2 2/3] USB: cdc-acm: fix unprivileged TIOCCSERIAL
Am Donnerstag, den 08.04.2021, 15:16 +0200 schrieb Johan Hovold:
> TIOCSSERIAL is a horrid, underspecified, legacy interface which for most
> serial devices is only useful for setting the close_delay and
> closing_wait parameters.
>
> A non-privileged user has only ever been able to set the since long
> deprecated ASYNC_SPD flags and trying to change any other *supported*
> feature should result in -EPERM being returned. Setting the current
> values for any supported features should return success.
>
> Fix the cdc-acm implementation which instead indicated that the
> TIOCSSERIAL ioctl was not even implemented when a non-privileged user
> set the current values.
>
> Fixes: ba2d8ce9db0a ("cdc-acm: implement TIOCSSERIAL to avoid blocking
> close(2)")
> Signed-off-by: Johan Hovold
Acked-by: Oliver Neukum
Re: [PATCH v2 1/3] Revert "USB: cdc-acm: fix rounding error in TIOCSSERIAL"
Am Donnerstag, den 08.04.2021, 15:16 +0200 schrieb Johan Hovold: > This reverts commit b401f8c4f492cbf74f3f59c9141e5be3071071bb. > > The offending commit claimed that trying to set the values reported back > by TIOCGSERIAL as a regular user could result in an -EPERM error when HZ > is 250, but that was never the case. > > With HZ=250, the default 0.5 second value of close_delay is converted to > 125 jiffies when set and is converted back to 50 centiseconds by > TIOCGSERIAL as expected (not 12 cs as was claimed, even if that was the > case before an earlier fix). > > Comparing the internal current and new jiffies values is just fine to > determine if the value is about to change so drop the bogus workaround > (which was also backported to stable). > > For completeness: With different default values for these parameters or > with a HZ value not divisible by two, the lack of rounding when setting > the default values in tty_port_init() could result in an -EPERM being > returned, but this is hardly something we need to worry about. > > Cc: Anthony Mallet > Cc: sta...@vger.kernel.org > Signed-off-by: Johan Hovold Acked-by: Oliver Neukum
Re: [PATCH 3/3] USB: cdc-acm: fix TIOCGSERIAL implementation
Am Donnerstag, den 08.04.2021, 13:54 +0200 schrieb Johan Hovold: > On Thu, Apr 08, 2021 at 01:34:12PM +0200, Oliver Neukum wrote: > > Am Donnerstag, den 08.04.2021, 11:48 +0200 schrieb Johan Hovold: > > > On Thu, Apr 08, 2021 at 10:36:46AM +0200, Oliver Neukum wrote: > > > > Am Mittwoch, den 07.04.2021, 12:28 +0200 schrieb Johan Hovold: > > > > Well, the devices report it. It is part of the standard. > > > > > > No, the standard doesn't include anything about a baud-base clock > > > AFAICT. > > > > Unfortunately it does. > > dwDTERate - chapter 6.3.11 - table 17 > > That's not the base clock rate, that's just the currently configured > line speed which you can read from termios > > If we does this wrongly, we should certainly fix it, but just removing > > the reporting doesn't look right to me. > > The driver got its interpretation of baud_base wrong, and CDC doesn't > even have a concept of base clock rate so removing it is the right thing > to do. > > Again, baud_base is really only relevant with legacy UARTs and when > using the deprecated ASYNC_SPD_CUST. > > And if the user wants to knows the current line speed we have a > different interface for that. Hi, thank you, that clarifies things. I am happy with the patch itself, but could I ask you to do two things: 1. Edit the commit description making clear that the difference between the base clock rate and the line speed. 2. Mark the patch specially to NOT be included in stable. We may have users misusing the current API. Regards Oliver
Re: [PATCH 2/3] USB: cdc-acm: fix unprivileged TIOCCSERIAL
Am Donnerstag, den 08.04.2021, 11:42 +0200 schrieb Johan Hovold: > On Thu, Apr 08, 2021 at 09:48:38AM +0200, Oliver Neukum wrote: > > Am Mittwoch, den 07.04.2021, 12:28 +0200 schrieb Johan Hovold: > > > TIOCSSERIAL is a horrid, underspecified, legacy interface which for most > > > serial devices is only useful for setting the close_delay and > > > closing_wait parameters. > > > > > > A non-privileged user has only ever been able to set the since long > > > deprecated ASYNC_SPD flags and trying to change any other *supported* > > > feature should result in -EPERM being returned. Setting the current > > > values for any supported features should return success. > > > > > > Fix the cdc-acm implementation which instead indicated that the > > > TIOCSSERIAL ioctl was not even implemented when a non-privileged user > > > set the current values. > > > > Hi, > > > > the idea here was that you are setting something else, if you are > > not changing a parameter that can be changed. That conclusion is > > dubious, but at the same time, this implementation can change > > only these two parameters. So can the test really be dropped > > as opposed to be modified? > > The de-facto standard for how to handle change requests for > non-supported features (e.g. changing the I/O port or IRQ) is to simply > ignore them and return 0. > > For most (non-legacy) serial devices the only relevant parameters are > close_delay and closing_wait. And as we need to return -EPERM when a > non-privileged user tries to change these, we cannot drop the test. > > (And returning -EOPNOTSUPP was never correct as the ioctl is indeed > supported.) OK, thanks for clarification. Yes the fix makes sense. Regards Oliver
Re: [PATCH 3/3] USB: cdc-acm: fix TIOCGSERIAL implementation
Am Donnerstag, den 08.04.2021, 11:48 +0200 schrieb Johan Hovold: > On Thu, Apr 08, 2021 at 10:36:46AM +0200, Oliver Neukum wrote: > > Am Mittwoch, den 07.04.2021, 12:28 +0200 schrieb Johan Hovold: > > Well, the devices report it. It is part of the standard. > > No, the standard doesn't include anything about a baud-base clock > AFAICT. Unfortunately it does. dwDTERate - chapter 6.3.11 - table 17 If we does this wrongly, we should certainly fix it, but just removing the reporting doesn't look right to me. Regards Oliver
Re: [PATCH 3/4] USB: serial: add support for multi-interface functions
Am Donnerstag, den 01.04.2021, 09:46 +0200 schrieb Johan Hovold: > On Wed, Mar 31, 2021 at 01:21:15PM +0200, Oliver Neukum wrote: > > Am Mittwoch, den 31.03.2021, 09:08 +0200 schrieb Oliver Neukum: > > on the third hand, the more I look at this, would you mind putting > > sibling_release() with a modified name into usbcore? This functionality > > is not limited to serial drivers. btusb needs it; cdc-acm needs it; > > usbaudio neds it. We have code duplication. > > Tell me about it. ;) Unfortunately, drivers all tend to handle this > slightly different, for example, using a disconnected flag, some claim > more than one other interface, others look like they may be using their > interface data as a flag for other purposes, etc. > > At some point we could unify all this but until then I don't think > putting only half of an interface into core makes much sense. OK, very well, then let's look at this from a fundamental point and design a bit. First, can we disregard the case of more than two interfaces? Regards Oliver
Re: [PATCH 1/2] USB:ehci:Add a whitelist for EHCI controllers
Am Donnerstag, den 08.04.2021, 17:11 +0800 schrieb Longfang Liu: > Some types of EHCI controllers do not have SBRN registers. > By comparing the white list, the operation of reading the SBRN > registers is skipped. > > Subsequent EHCI controller types without SBRN registers can be > directly added to the white list. Hi, shouldn't this set a flag for a missing functionality? Regards Oliver
Re: [PATCH 2/3] USB: cdc-acm: fix unprivileged TIOCCSERIAL
Am Mittwoch, den 07.04.2021, 12:28 +0200 schrieb Johan Hovold: > TIOCSSERIAL is a horrid, underspecified, legacy interface which for most > serial devices is only useful for setting the close_delay and > closing_wait parameters. > > A non-privileged user has only ever been able to set the since long > deprecated ASYNC_SPD flags and trying to change any other *supported* > feature should result in -EPERM being returned. Setting the current > values for any supported features should return success. > > Fix the cdc-acm implementation which instead indicated that the > TIOCSSERIAL ioctl was not even implemented when a non-privileged user > set the current values. Hi, the idea here was that you are setting something else, if you are not changing a parameter that can be changed. That conclusion is dubious, but at the same time, this implementation can change only these two parameters. So can the test really be dropped as opposed to be modified? Regards Oliver
Re: [PATCH 3/3] USB: cdc-acm: fix TIOCGSERIAL implementation
Am Mittwoch, den 07.04.2021, 12:28 +0200 schrieb Johan Hovold:
> TIOCSSERIAL is a horrid, underspecified, legacy interface which for most
> serial devices is only useful for setting the close_delay and
> closing_wait parameters.
>
> The xmit_fifo_size parameter could be used to set the hardware transmit
> fifo size of a legacy UART when it could not be detected, but the
> interface is limited to eight bits and should be left unset when it is
> not used.
OK.
> Similarly, baud_base could be used to set the uart base clock when it
> could not be detected, but might as well be left unset when it is not
> known.
Well, the devices report it. It is part of the standard.
> Fix the cdc-acm TIOCGSERIAL implementation by dropping its custom
> interpretation of the unused xmit_fifo_size and baud_base fields, which
> overflowed the former with the URB buffer size and set the latter to the
> current line speed. Also return the port line number, which is the only
> other value used besides the close parameters.
>
> Fixes: 18c75720e667 ("USB: allow users to run setserial with cdc-acm")
> Signed-off-by: Johan Hovold
> ---
> drivers/usb/class/cdc-acm.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
> index 43e31dad4831..b74713518b3a 100644
> --- a/drivers/usb/class/cdc-acm.c
> +++ b/drivers/usb/class/cdc-acm.c
> @@ -929,8 +929,7 @@ static int get_serial_info(struct tty_struct *tty, struct
> serial_struct *ss)
> {
> struct acm *acm = tty->driver_data;
>
> - ss->xmit_fifo_size = acm->writesize;
> - ss->baud_base = le32_to_cpu(acm->line.dwDTERate);
If we do this, we have a value that can be set, but is not reported.
That looks a bit strange to me.
Regards
Oliver
Re: [PATCH net-next v4 0/4] usbnet: speed reporting for devices without MDIO
Am Dienstag, den 06.04.2021, 18:01 + schrieb Grant Grundler: > > Ethernet does not support > > different rates in each direction. So if RX and TX are different, i > > would actually say something is broken. > > Agreed. The question is: Is there data or some heuristics we can use > to determine if the physical layer/link is ethernet? > I'm pessimistic we will be able to since this is at odds with the > intent of the CDC spec. Yes, CDC intends to hide that. We can conclude that an asymmetric link rules out ethernet, but anything else is difficult. Regards Oliver
Re: [PATCH 3/4] USB: serial: add support for multi-interface functions
Am Mittwoch, den 31.03.2021, 09:08 +0200 schrieb Oliver Neukum: > Am Dienstag, den 30.03.2021, 17:22 +0200 schrieb Johan Hovold: > > On Tue, Mar 30, 2021 at 04:44:32PM +0200, Oliver Neukum wrote: > > > Am Dienstag, den 30.03.2021, 16:38 +0200 schrieb Johan Hovold: > > > > @@ -1115,6 +1161,8 @@ static void usb_serial_disconnect(struct > > > > usb_interface *interface) > > > > if (serial->type->disconnect) > > > > serial->type->disconnect(serial); > > > > > > > > + release_sibling(serial, interface); > > > > + > > > > /* let the last holder of this object cause it to be cleaned up > > > > */ > > > > usb_serial_put(serial); > > > > dev_info(dev, "device disconnected\n"); > > > > > > Hi, > > > > > > does this assume you are called for the original interface first? > > > > No, I handle either interface being unbound first (e.g. see > > release_sibling()). > > > > > I am afraid that is an assumption you cannot make. In fact, if somebody > > > is doing odd things with sysfs you cannot even assume both will see a > > > disconnect() > > > > Right, but disconnect() will still be called also for the sibling > > interface as part of release_sibling() above. > > OK, sorry I overlooked that. Hi, on the third hand, the more I look at this, would you mind putting sibling_release() with a modified name into usbcore? This functionality is not limited to serial drivers. btusb needs it; cdc-acm needs it; usbaudio neds it. We have code duplication. Regards Oliver
Re: [PATCH 3/4] USB: serial: add support for multi-interface functions
Am Dienstag, den 30.03.2021, 17:22 +0200 schrieb Johan Hovold: > On Tue, Mar 30, 2021 at 04:44:32PM +0200, Oliver Neukum wrote: > > Am Dienstag, den 30.03.2021, 16:38 +0200 schrieb Johan Hovold: > > > @@ -1115,6 +1161,8 @@ static void usb_serial_disconnect(struct > > > usb_interface *interface) > > > if (serial->type->disconnect) > > > serial->type->disconnect(serial); > > > > > > + release_sibling(serial, interface); > > > + > > > /* let the last holder of this object cause it to be cleaned up */ > > > usb_serial_put(serial); > > > dev_info(dev, "device disconnected\n"); > > > > Hi, > > > > does this assume you are called for the original interface first? > > No, I handle either interface being unbound first (e.g. see > release_sibling()). > > > I am afraid that is an assumption you cannot make. In fact, if somebody > > is doing odd things with sysfs you cannot even assume both will see a > > disconnect() > > Right, but disconnect() will still be called also for the sibling > interface as part of release_sibling() above. OK, sorry I overlooked that. Regards Oliver
Re: [PATCH 3/4] USB: serial: add support for multi-interface functions
Am Dienstag, den 30.03.2021, 16:38 +0200 schrieb Johan Hovold: > @@ -1115,6 +1161,8 @@ static void usb_serial_disconnect(struct usb_interface > *interface) > if (serial->type->disconnect) > serial->type->disconnect(serial); > > + release_sibling(serial, interface); > + > /* let the last holder of this object cause it to be cleaned up */ > usb_serial_put(serial); > dev_info(dev, "device disconnected\n"); Hi, does this assume you are called for the original interface first? I am afraid that is an assumption you cannot make. In fact, if somebody is doing odd things with sysfs you cannot even assume both will see a disconnect() Regards Oliver
Re: [PATCH 7/7] USB: cdc-acm: always claim data interface
Am Donnerstag, den 18.03.2021, 16:52 +0100 schrieb Johan Hovold:
> Make sure to always claim the data interface and bail out if it's
> already bound to another driver or isn't authorised.
Hi,
Thanks for the fixes. All previous ones are good work.
this one is problematic I am afraid.
acm_probe() has a test for the availability of the interface:
if (!combined_interfaces && usb_interface_claimed(data_interface)) {
/* valid in this context */
dev_dbg(>dev, "The data interface isn't available\n");
return -EBUSY;
}
That check is ancient. BKL still existed. If you want to remove it
and do proper error handling, that would be good. But if you do
error handling, the check has to go, too.
Regards
Oliver
Re: [PATCH 6/7] USB: cdc-acm: use negation for NULL checks
Am Donnerstag, den 18.03.2021, 16:52 +0100 schrieb Johan Hovold: > Use negation consistently throughout the driver for NULL checks. > > Signed-off-by: Johan Hovold Acked-by: Oliver Neukum
Re: [PATCH 5/7] USB: cdc-acm: clean up probe error labels
Am Donnerstag, den 18.03.2021, 16:52 +0100 schrieb Johan Hovold: > Name the probe error labels after what they do rather than using > sequence numbers which is harder to review and maintain (e.g. may > require renaming unrelated labels when a label is added or removed). > > Signed-off-by: Johan Hovold Acked-by: Oliver Neukum
Re: [PATCH 4/7] USB: cdc-acm: drop redundant driver-data reset
Am Donnerstag, den 18.03.2021, 16:51 +0100 schrieb Johan Hovold: > There's no need to clear the interface driver data on failed probe (and > driver core will clear it anyway). > > Signed-off-by: Johan Hovold Acked-by: Oliver Neukum
Re: [PATCH 3/7] USB: cdc-acm: drop redundant driver-data assignment
Am Donnerstag, den 18.03.2021, 16:51 +0100 schrieb Johan Hovold: > The interface driver data has already been set by > usb_driver_claim_interface() so drop the redundant subsequent > assignment. > > Signed-off-by: Johan Hovold Acked-by: Oliver Neukum
Re: [PATCH 2/7] USB: cdc-acm: fix use-after-free after probe failure
Am Donnerstag, den 18.03.2021, 16:51 +0100 schrieb Johan Hovold:
> If tty-device registration fails the driver would fail to release the
> data interface. When the device is later disconnected, the disconnect
> callback would still be called for the data interface and would go about
> releasing already freed resources.
>
> Fixes: c93d81955005 ("usb: cdc-acm: fix error handling in acm_probe()")
> Cc: sta...@vger.kernel.org # 3.9
> Cc: Alexey Khoroshilov
> Signed-off-by: Johan Hovold ]
Acked-by: Oliver Neukum
Re: [PATCH 1/7] USB: cdc-acm: fix double free on probe failure
Am Donnerstag, den 18.03.2021, 16:51 +0100 schrieb Johan Hovold:
> If tty-device registration fails the driver copy of any Country
> Selection functional descriptor would end up being freed twice; first
> explicitly in the error path and then again in the tty-port destructor.
>
> Drop the first erroneous free that was left when fixing a tty-port
> resource leak.
>
> Fixes: cae2bc768d17 ("usb: cdc-acm: Decrement tty port's refcount if probe()
> fail")
> Cc: sta...@vger.kernel.org # 4.19
> Cc: Jaejoong Kim
> Signed-off-by: Johan Hovold
Acked-by: Oliver Neukum
Re: usb: cdc-acm: BUG kmalloc-128 Poison overwritten
Am Montag, den 08.03.2021, 09:50 +0100 schrieb Bruno Thomsen: > > Tested-by: Bruno Thomsen > > I have not observed any oops with patches applied. Patches have seen > more than 10 weeks of runtime testing across multiple devices. Hi, that is good news. I shall send them upstream. Regards Oliver
Re: usb: cdc-acm: BUG kmalloc-128 Poison overwritten
Am Mittwoch, den 24.02.2021, 16:21 +0100 schrieb Bruno Thomsen:
Hi,
> No, this is not a regression from 5.10. It seems that many attempts to
> fix cdc-acm in the 5.x kernel series have failed to fix the root cause of
> these oops. I have not seen this on 4.14 and 4.19, but I have observed
> it on at least 5.3 and newer kernels in slight variations.
> I guess this is because cdc-acm is very common in the embedded
> ARM world and rarely used on servers or laptops. Combined with
> ARM devices still commonly use 4.x LTS kernels. Not sure if
> hardening options on the kernel has increased change of reproducing
> oops.
OK, so this is not an additional problem.
According to your logs, an URB that should have been killed wasn't.
> I am ready to test new patches and will continue to report oops
Could you test the attached patches?
Regards
Oliver
From 307097e80657ca44ac99da8efc8397070b1aff3f Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Thu, 18 Feb 2021 13:42:40 +0100
Subject: [PATCH 1/2] cdc-wdm: untangle a circular dependency between callback
and softint
We have a cycle of callbacks scheduling works which submit
URBs with thos callbacks. This needs to be blocked, stopped
and unblocked to untangle the circle..
Signed-off-by: Oliver Neukum
---
drivers/usb/class/cdc-wdm.c | 30 ++
1 file changed, 22 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 508b1c3f8b73..d1e4a7379beb 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -321,12 +321,23 @@ static void wdm_int_callback(struct urb *urb)
}
-static void kill_urbs(struct wdm_device *desc)
+static void poison_urbs(struct wdm_device *desc)
{
/* the order here is essential */
- usb_kill_urb(desc->command);
- usb_kill_urb(desc->validity);
- usb_kill_urb(desc->response);
+ usb_poison_urb(desc->command);
+ usb_poison_urb(desc->validity);
+ usb_poison_urb(desc->response);
+}
+
+static void unpoison_urbs(struct wdm_device *desc)
+{
+ /*
+ * the order here is not essential
+ * it is symmetrical just to be nice
+ */
+ usb_unpoison_urb(desc->response);
+ usb_unpoison_urb(desc->validity);
+ usb_unpoison_urb(desc->command);
}
static void free_urbs(struct wdm_device *desc)
@@ -741,11 +752,12 @@ static int wdm_release(struct inode *inode, struct file *file)
if (!desc->count) {
if (!test_bit(WDM_DISCONNECTING, >flags)) {
dev_dbg(>intf->dev, "wdm_release: cleanup\n");
- kill_urbs(desc);
+ poison_urbs(desc);
spin_lock_irq(>iuspin);
desc->resp_count = 0;
spin_unlock_irq(>iuspin);
desc->manage_power(desc->intf, 0);
+ unpoison_urbs(desc);
} else {
/* must avoid dev_printk here as desc->intf is invalid */
pr_debug(KBUILD_MODNAME " %s: device gone - cleaning up\n", __func__);
@@ -1037,9 +1049,9 @@ static void wdm_disconnect(struct usb_interface *intf)
wake_up_all(>wait);
mutex_lock(>rlock);
mutex_lock(>wlock);
+ poison_urbs(desc);
cancel_work_sync(>rxwork);
cancel_work_sync(>service_outs_intr);
- kill_urbs(desc);
mutex_unlock(>wlock);
mutex_unlock(>rlock);
@@ -1080,9 +1092,10 @@ static int wdm_suspend(struct usb_interface *intf, pm_message_t message)
set_bit(WDM_SUSPENDING, >flags);
spin_unlock_irq(>iuspin);
/* callback submits work - order is essential */
- kill_urbs(desc);
+ poison_urbs(desc);
cancel_work_sync(>rxwork);
cancel_work_sync(>service_outs_intr);
+ unpoison_urbs(desc);
}
if (!PMSG_IS_AUTO(message)) {
mutex_unlock(>wlock);
@@ -1140,7 +1153,7 @@ static int wdm_pre_reset(struct usb_interface *intf)
wake_up_all(>wait);
mutex_lock(>rlock);
mutex_lock(>wlock);
- kill_urbs(desc);
+ poison_urbs(desc);
cancel_work_sync(>rxwork);
cancel_work_sync(>service_outs_intr);
return 0;
@@ -1151,6 +1164,7 @@ static int wdm_post_reset(struct usb_interface *intf)
struct wdm_device *desc = wdm_find_device(intf);
int rv;
+ unpoison_urbs(desc);
clear_bit(WDM_OVERFLOW, >flags);
clear_bit(WDM_RESETTING, >flags);
rv = recover_from_urb_loss(desc);
--
2.26.2
From 3eeb644af140174ebad6ddce5526bcaf42ccd9c9 Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Thu, 18 Feb 2021 13:52:28 +0100
Subject: [PATCH 2/2] cdc-acm: untangle a circular dependency between callback
and softint
We have a cycle of callbacks scheduling works which submit
URBs with thos callbacks. This needs to be blocked, stopped
and unblocked to untangle the circle.
Signed-off-by: Oliver Neukum
---
drivers/usb/class/cdc-acm.c | 41 +
1 file changed, 28 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 781905745812..235fd1f654a4 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@
Re: [PATCH] usbnet: ipheth: fix connectivity with iOS 14
Am Sonntag, den 21.02.2021, 10:42 + schrieb Sam Bingner: > There seems to be a problem with this patch: > > Whenever the iPhone sends a packet to the tethered device that is 1500 bytes > long, it gets the error "ipheth 1-1:4.2: ipheth_rcvbulk_callback: urb status: > -79" on the connected device and stops passing traffic. I am able to bring > it back up by shutting and unshutting the interface, but the same thing > happens very quickly. I noticed that this patch dropped the max USB packet > size from 1516 to 1514 bytes, so I decided to try lowering the MTU to 1498; > this made the connection reliable and no more errors occurred. > > It appears to me that the iPhone is still sending out 1516 bytes over USB for > a 1500 byte packet and this patch makes USB abort when that happens? I could > duplicate reliably by sending a ping from the iphone (ping -s 1472) to the > connected device, or vice versa as the reply would then break it. > > I apologize if this reply doesn't end up where it should - I tried to reply > to the last message in this thread but I wasn't actually *on* the thread so I > had to just build it as much as possible myself. Is this a regression? Does it work after reverting the patch? Which version of iOS? Regards Oliver
Re: usb: cdc-acm: BUG kmalloc-128 Poison overwritten
Am Donnerstag, den 18.02.2021, 16:52 +0100 schrieb Bruno Thomsen:
> Den fre. 12. feb. 2021 kl. 16.33 skrev Bruno Thomsen
> :
> > Hi,
> >
> > I have been experience random kernel oops in the cdc-acm driver on
> > imx7 (arm arch). Normally it happens during the first 1-3min runtime
> > after power-on. Below oops is from 5.8.17 mainline kernel with an
> > extra patch back-ported in an attempt to fix it:
> > 38203b8385 ("usb: cdc-acm: fix cooldown mechanism")
>
> I can now boot board with 5.11 kernel without any extra patches and
> it produce similar issue. Hopefully that make the oops more useful.
> Issue has been seen on multiple devices, so I don't think it's a bad
> hardware issue.
Hi,
is this a regression from 5.10?
Regards
Oliver
Re: [PATCH 2/2] net: usbnet: use new tasklet API
Am Samstag, den 23.01.2021, 18:32 +0100 schrieb Emil Renner Berthing:
> From: Emil Renner Berthing
>
> This converts the driver to use the new tasklet API introduced in
> commit 12cc923f1ccc ("tasklet: Introduce new initialization API")
>
> Signed-off-by: Emil Renner Berthing
Acked-by: Oliver Neukum
Re: [PATCH 1/2] net: usbnet: initialize tasklet using tasklet_init
Am Samstag, den 23.01.2021, 18:32 +0100 schrieb Emil Renner Berthing: > From: Emil Renner Berthing > > Initialize tasklet using tasklet_init() rather than open-coding it. > > Signed-off-by: Emil Renner Berthing Acked-by: Oliver Neukum
Re: [PATCH] usbnet: fix the indentation of one code snippet
Am Samstag, den 23.01.2021, 13:11 +0800 schrieb Dongliang Mu: > Every line of code should start with tab (8 characters) > > Signed-off-by: Dongliang Mu Acked-by: Oliver Neukum
Re: [PATCH v2] can: mcba_usb: Fix memory leak when cancelling urb
Am Montag, den 11.01.2021, 10:49 + schrieb Bui Quang Minh: > In mcba_usb_read_bulk_callback(), when we don't resubmit or fails to > resubmit the urb, we need to deallocate the transfer buffer that is > allocated in mcba_usb_start(). > > Reported-by: syzbot+57281c762a3922e14...@syzkaller.appspotmail.com > Signed-off-by: Bui Quang Minh > --- > v1: add memory leak fix when not resubmitting urb > v2: add memory leak fix when failing to resubmit urb > > drivers/net/can/usb/mcba_usb.c | 11 --- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c > index df54eb7d4b36..30236e640116 100644 > --- a/drivers/net/can/usb/mcba_usb.c > +++ b/drivers/net/can/usb/mcba_usb.c > @@ -584,6 +584,8 @@ static void mcba_usb_read_bulk_callback(struct urb *urb) > case -EPIPE: > case -EPROTO: > case -ESHUTDOWN: > + usb_free_coherent(urb->dev, urb->transfer_buffer_length, > + urb->transfer_buffer, urb->transfer_dma); > return; > Can you call usb_free_coherent() in what can be hard IRQ context? Regards Oliver
Re: 回复: KASAN: use-after-free Read in service_outstanding_interrupt
Am Dienstag, den 05.01.2021, 04:50 + schrieb Zhang, Qiang: > > > 发件人: Oliver Neukum > 发送时间: 2021年1月5日 0:28 > 收件人: syzbot; andreyk...@google.com; gre...@linuxfoundation.org; > gustavo...@kernel.org; ingras...@epigenesys.com; lee.jo...@linaro.org; > linux-kernel@vger.kernel.org; linux-...@vger.kernel.org; > penguin-ker...@i-love.sakura.ne.jp; syzkaller-b...@googlegroups.com > 主题: Re: KASAN: use-after-free Read in service_outstanding_interrupt > > Am Donnerstag, den 17.12.2020, 19:21 -0800 schrieb syzbot: > > syzbot has found a reproducer for the following issue on: > > > > HEAD commit:5e60366d Merge tag 'fallthrough-fixes-clang-5.11-rc1' of g.. > > git tree: > > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing > > console output: https://syzkaller.appspot.com/x/log.txt?x=12c5b62350 > > kernel config: https://syzkaller.appspot.com/x/.config?x=5cea7506b7139727 > > dashboard link: https://syzkaller.appspot.com/bug?extid=9e04e2df4a32fb661daf > > compiler: gcc (GCC) 10.1.0-syz 20200507 > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=175adf0750 > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1672680f50 > > > > IMPORTANT: if you fix the issue, please add the following tag to the commit: > > Reported-by: >syzbot+9e04e2df4a32fb661...@syzkaller.appspotmail.com > > > > #syz test: https://github.com/google/kasan.git 5e60366d > > > > Hello Oliver > > this use-after-free still exists，It can be seen from calltrace that it is > usb_device's object has been released when disconnect, > can add a reference count to usb_device's object to avoid this problem Hi, thanks for your analysis. I think you are correct in your analysis, but I am afraid your fix is not correct. The driver is submitting an URB to a disconnected device. Your fix would prevent a crash, which is definitely good, but we still cannot do that, because the device may be owned by another driver or usbfs at that time. Regards Oliver
Re: KASAN: use-after-free Read in service_outstanding_interrupt
Am Donnerstag, den 17.12.2020, 19:21 -0800 schrieb syzbot:
> syzbot has found a reproducer for the following issue on:
>
> HEAD commit:5e60366d Merge tag 'fallthrough-fixes-clang-5.11-rc1' of g..
> git tree:
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
> console output: https://syzkaller.appspot.com/x/log.txt?x=12c5b62350
> kernel config: https://syzkaller.appspot.com/x/.config?x=5cea7506b7139727
> dashboard link: https://syzkaller.appspot.com/bug?extid=9e04e2df4a32fb661daf
> compiler: gcc (GCC) 10.1.0-syz 20200507
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=175adf0750
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1672680f50
>
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+9e04e2df4a32fb661...@syzkaller.appspotmail.com
#syz test: https://github.com/google/kasan.git 5e60366d
>From f51e3c5a202f3abc805edd64b21a68d29dd9d60e Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Mon, 4 Jan 2021 17:26:33 +0100
Subject: [PATCH] cdc-wdm: poison URBs upon disconnect
We have a chicken and egg issue between interrupt and work.
This should break the cycle.
Signed-off-by: Oliver Neukum
---
drivers/usb/class/cdc-wdm.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 02d0cfd23bb2..14eddda35280 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -324,9 +324,9 @@ static void wdm_int_callback(struct urb *urb)
static void kill_urbs(struct wdm_device *desc)
{
/* the order here is essential */
- usb_kill_urb(desc->command);
- usb_kill_urb(desc->validity);
- usb_kill_urb(desc->response);
+ usb_poison_urb(desc->command);
+ usb_poison_urb(desc->validity);
+ usb_poison_urb(desc->response);
}
static void free_urbs(struct wdm_device *desc)
--
2.26.2
Re: [PATCH] usb/mos7720: process deferred urbs in a workqueue
Am Donnerstag, den 05.11.2020, 22:17 -0800 schrieb Davidlohr Bueso: > @@ -1888,16 +1732,8 @@ static void mos7720_release(struct usb_serial *serial) > usb_set_serial_data(serial, NULL); > mos_parport->serial = NULL; > > - /* if tasklet currently scheduled, wait for it to complete */ > - tasklet_kill(_parport->urb_tasklet); > - > - /* unlink any urbs sent by the tasklet */ > - spin_lock_irqsave(_parport->listlock, flags); > - list_for_each_entry(urbtrack, > - _parport->active_urbs, > - urblist_entry) > - usb_unlink_urb(urbtrack->urb); > - spin_unlock_irqrestore(_parport->listlock, flags); > + /* if work is currently scheduled, wait for it to complete */ > + cancel_work_sync(_parport->work); > parport_del_port(mos_parport->pp); > > kref_put(_parport->ref_count, destroy_mos_parport); Hi, do you really want to cancel as opposed to wait for work in release()? Regards Oliver
Re: [PATCH v3] net: usb: usbnet: update __usbnet_{read|write}_cmd() to use new API
Am Sonntag, den 01.11.2020, 03:05 +0530 schrieb Anant Thazhemadam:
> Currently, __usbnet_{read|write}_cmd() use usb_control_msg().
> However, this could lead to potential partial reads/writes being
> considered valid, and since most of the callers of
> usbnet_{read|write}_cmd() don't take partial reads/writes into account
> (only checking for negative error number is done), and this can lead to
> issues.
>
Hi,
plesae send this as a post of its own. We cannot take a new set
as a reply to an older set.
Regards
Oliver
Re: [PATCH] usb: cdc-acm: fix cooldown mechanism
Am Montag, den 19.10.2020, 19:07 +0200 schrieb Jerome Brunet:
> Commit a4e7279cd1d1 ("cdc-acm: introduce a cool down") is causing
> regression if there is some USB error, such as -EPROTO.
>
> This has been reported on some samples of the Odroid-N2 using the Combee II
> Zibgee USB dongle.
>
> > struct acm *acm = container_of(work, struct acm, work)
>
> is incorrect in case of a delayed work and causes warnings, usually from
> the workqueue:
>
> > WARNING: CPU: 0 PID: 0 at kernel/workqueue.c:1474 __queue_work+0x480/0x528.
>
> When this happens, USB eventually stops working completely after a while.
> Also the ACM_ERROR_DELAY bit is never set, so the cooldown mechanism
> previously introduced cannot be triggered and acm_submit_read_urb() is
> never called.
>
> This changes makes the cdc-acm driver use a single delayed work, fixing the
> pointer arithmetic in acm_softint() and set the ACM_ERROR_DELAY when the
> cooldown mechanism appear to be needed.
>
> Fixes: a4e7279cd1d1 ("cdc-acm: introduce a cool down")
> Reported-by: Pascal Vizeli
> Cc: Oliver Neukum
> Signed-off-by: Jerome Brunet
Acked-by: Oliver Neukum
[tip: perf/urgent] media: usbtv: Fix refcounting mixup
The following commit has been merged into the perf/urgent branch of tip:
Commit-ID: 1eaed405fcd22e52a438b619e6ea85539f1c150a
Gitweb:
https://git.kernel.org/tip/1eaed405fcd22e52a438b619e6ea85539f1c150a
Author:Oliver Neukum
AuthorDate:Thu, 24 Sep 2020 11:14:10 +02:00
Committer: Greg Kroah-Hartman
CommitterDate: Sat, 17 Oct 2020 08:31:21 +02:00
media: usbtv: Fix refcounting mixup
commit bf65f8aabdb37bc1a785884374e919477fe13e10 upstream.
The premature free in the error path is blocked by V4L
refcounting, not USB refcounting. Thanks to
Ben Hutchings for review.
[v2] corrected attributions
Signed-off-by: Oliver Neukum
Fixes: 50e704453553 ("media: usbtv: prevent double free in error case")
CC: sta...@vger.kernel.org
Reported-by: Ben Hutchings
Signed-off-by: Hans Verkuil
Signed-off-by: Mauro Carvalho Chehab
Signed-off-by: Greg Kroah-Hartman
---
drivers/media/usb/usbtv/usbtv-core.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/media/usb/usbtv/usbtv-core.c
b/drivers/media/usb/usbtv/usbtv-core.c
index ee9c656..2308c0b 100644
--- a/drivers/media/usb/usbtv/usbtv-core.c
+++ b/drivers/media/usb/usbtv/usbtv-core.c
@@ -113,7 +113,8 @@ static int usbtv_probe(struct usb_interface *intf,
usbtv_audio_fail:
/* we must not free at this point */
- usb_get_dev(usbtv->udev);
+ v4l2_device_get(>v4l2_dev);
+ /* this will undo the v4l2_device_get() */
usbtv_video_free(usbtv);
usbtv_video_fail:
Re: INFO: task hung in hub_port_init
Am Dienstag, den 06.10.2020, 01:19 -0700 schrieb syzbot:
Hi,
> HEAD commit:d3d45f82 Merge tag 'pinctrl-v5.9-2' of git://git.kernel.or..
> git tree: upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=14c3b3db90
> kernel config: https://syzkaller.appspot.com/x/.config?x=89ab6a0c48f30b49
> dashboard link: https://syzkaller.appspot.com/bug?extid=74d6ef051d3d2eacf428
> compiler: gcc (GCC) 10.1.0-syz 20200507
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=153bf5bd90
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=124c92af90
>
> The issue was bisected to:
>
> commit 6dcf45e514974a1ff10755015b5e06746a033e5f
> Author: Niklas Söderlund
> Date: Mon Jan 9 15:34:04 2017 +
>
> sh_eth: use correct name for ECMR_MPDE bit
I am afraid this has bisected a race condition into neverland.
> bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=152bb76050
> final oops: https://syzkaller.appspot.com/x/report.txt?x=172bb76050
> console output: https://syzkaller.appspot.com/x/log.txt?x=132bb76050
>
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+74d6ef051d3d2eacf...@syzkaller.appspotmail.com
> Fixes: 6dcf45e51497 ("sh_eth: use correct name for ECMR_MPDE bit")
>
> INFO: task kworker/0:0:5 blocked for more than 143 seconds.
> Not tainted 5.9.0-rc7-syzkaller #0
> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> task:kworker/0:0 state:D stack:27664 pid:5 ppid: 2
> flags:0x4000
> Workqueue: usb_hub_wq hub_event
> Call Trace:
> context_switch kernel/sched/core.c:3778 [inline]
> __schedule+0xec9/0x2280 kernel/sched/core.c:4527
> schedule+0xd0/0x2a0 kernel/sched/core.c:4602
By this time urb_dequeue() has been killed and has returned.
> usb_kill_urb.part.0+0x197/0x220 drivers/usb/core/urb.c:696
> usb_kill_urb+0x7c/0x90 drivers/usb/core/urb.c:691
> usb_start_wait_urb+0x24a/0x2b0 drivers/usb/core/message.c:64
> usb_internal_control_msg drivers/usb/core/message.c:102 [inline]
> usb_control_msg+0x31c/0x4a0 drivers/usb/core/message.c:153
> hub_port_init+0x11ae/0x2d80 drivers/usb/core/hub.c:4689
> hub_port_connect drivers/usb/core/hub.c:5140 [inline]
> hub_port_connect_change drivers/usb/core/hub.c:5348 [inline]
> port_event drivers/usb/core/hub.c:5494 [inline]
>
This looks like it should.
Which HC driver are you using for these tests? It looks like
the HCD is not acting on urb_dequeue(), rather than a locking
issue.
Regards
Oliver
Re: [PATCH] Bluetooth: btusb: Avoid unnecessary reset upon system resume
Am Donnerstag, den 24.09.2020, 20:33 -0700 schrieb Abhishek Pandit- Subedi: > Runtime suspend always requires remote wakeup to be set No, not entirely. Btusb requires remote wakeup between open() and close(). On a closed device it is not set to save more power. > and reset > resume isn't used there. reset_resume() is basically incompatible with remote wakeup. Remote wakeup implies that you will ask the device to perform some action after wakeup. A reset destroys that capability. > During system suspend, when remote wakeup is not set, RTL8822CE loses > the FW loaded by the driver and any state currently in the controller. That is true after every suspend, whether runtime or system. The device throws away its firmware when it does not need it. You have control over this feature. Hence the quirk enables remote wakeup upon close(). That is the most important part of the original patch. > This causes the kernel and the controller state to go out of sync. > One of the issues we observed on the Realtek controller without the > reset resume quirk was that paired or connected devices would just > stop working after resume. The logic on whether reset_resume() should be used is imperfect. > > In general, laptops will cut off the USB power during S3. > > When AC is plugged, some laptops cuts USB power off and some don't. This > > also applies to many desktops. Not to mention there can be BIOS options to > > control USB power under S3/S4/S5... > > > > So we don't know beforehand. Technically we tell the BIOS before suspend the system whether a device should wake up the system during sleep and hope that the BIOS is clever enough not to cut power to them. Compliance is mixed. > In your case, since you don't need to enforce the 'Remote Wakeup' bit, > if you unset the BTUSB_WAKEUP_DISABLE for that VID:PID, you should get > the desirable behavior (which is actually the default behavior; remote > wake will always be asserted instead of only during Runtime Suspend). Well, no. Only between open() and close() Please always test the case of Bluetooth not being used. I know it is not sexy, but surprisingly common. Regards Oliver
Re: [PATCH 3/4] net: usb: rtl8150: use usb_control_msg_recv() and usb_control_msg_send()
Am Mittwoch, den 23.09.2020, 20:02 +0530 schrieb Himadri Pandya: > I meant that it was stupid to change it without properly understanding > the significance of GFP_NOIO in this context. > > So now, do we re-write the wrapper functions with flag passed as a parameter? Hi, I hope I set you in CC for a patch set doing exactly that. Do not let me or other maintainers discourage you from writing patches. Look at it this way. Had you not written this patch, I would not have looked into the matter. Patches are supposed to be reviewed. If you want additional information, just ask. We do not want people discouraged from writing substantial patches. Regards Oliver
Re: [PATCH 3/4] net: usb: rtl8150: use usb_control_msg_recv() and usb_control_msg_send()
Am Mittwoch, den 23.09.2020, 17:48 +0300 schrieb Petko Manolov: > > This internally uses kmemdup() with GFP_KERNEL. > > You cannot make this change. The API does not support it. > > I am afraid we will have to change the API first, before more > > such changes are done. > > One possible fix is to add yet another argument to usb_control_msg_recv(), > which > would be the GFP_XYZ flag to pass on to kmemdup(). Up to Greg, of course. Hi, submitted. The problem is those usages that are very hard to trace. I'd dislike to just slab GFP_NOIO on them for no obvious reason. Regards Oliver
Re: [PATCH 3/4] net: usb: rtl8150: use usb_control_msg_recv() and usb_control_msg_send()
Am Mittwoch, den 23.09.2020, 19:36 +0530 schrieb Himadri Pandya: > On Wed, Sep 23, 2020 at 3:52 PM Oliver Neukum wrote: > > > > Am Mittwoch, den 23.09.2020, 14:35 +0530 schrieb Himadri Pandya: > > GFP_NOIO is used here for a reason. You need to use this helper > > while in contexts of error recovery and runtime PM. > > > > Understood. Apologies for proposing such a stupid change. Hi, sorry if you concluded that the patch was stupid. That was not my intent. It was the best the API allowed for. If an API makes it easy to make a mistake, the problem is with the API, not the developer. Regards Oliver
Re: [PATCH 3/4] net: usb: rtl8150: use usb_control_msg_recv() and usb_control_msg_send()
Am Mittwoch, den 23.09.2020, 14:35 +0530 schrieb Himadri Pandya:
Hi,
> Many usage of usb_control_msg() do not have proper error check on return
> value leaving scope for bugs on short reads. New usb_control_msg_recv()
> and usb_control_msg_send() nicely wraps usb_control_msg() with proper
> error check. Hence use the wrappers instead of calling usb_control_msg()
> directly.
>
> Signed-off-by: Himadri Pandya
Nacked-by: Oliver Neukum
> ---
> drivers/net/usb/rtl8150.c | 32 ++--
> 1 file changed, 6 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> index 733f120c852b..e3002b675921 100644
> --- a/drivers/net/usb/rtl8150.c
> +++ b/drivers/net/usb/rtl8150.c
> @@ -152,36 +152,16 @@ static const char driver_name [] = "rtl8150";
> */
> static int get_registers(rtl8150_t * dev, u16 indx, u16 size, void *data)
> {
> - void *buf;
> - int ret;
> -
> - buf = kmalloc(size, GFP_NOIO);
GFP_NOIO is used here for a reason. You need to use this helper
while in contexts of error recovery and runtime PM.
> - if (!buf)
> - return -ENOMEM;
> -
> - ret = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
> - RTL8150_REQ_GET_REGS, RTL8150_REQT_READ,
> - indx, 0, buf, size, 500);
> - if (ret > 0 && ret <= size)
> - memcpy(data, buf, ret);
> - kfree(buf);
> - return ret;
> + return usb_control_msg_recv(dev->udev, 0, RTL8150_REQ_GET_REGS,
> + RTL8150_REQT_READ, indx, 0, data,
> + size, 500);
This internally uses kmemdup() with GFP_KERNEL.
You cannot make this change. The API does not support it.
I am afraid we will have to change the API first, before more
such changes are done.
I would suggest dropping the whole series for now.
Regards
Oliver
Re: [PATCH] usb: yurex: Rearrange code not to need GFP_ATOMIC
Am Montag, den 21.09.2020, 14:52 +0200 schrieb Pavel Machek: > Hi! > > > > Task goes to TASK_INTERRUPTIBLE > > > > > if (retval >= 0) > > > timeout = schedule_timeout(YUREX_WRITE_TIMEOUT); > > > > Task turns into Sleeping Beauty until timeout > > Is there way to do the allocations for submit_urb before the No. In theory you do not even know which HC will get the URB. Preallocating resources is impossible. I do consider this a design bug in the usbcore API. > prepare_to_wait? GFP_ATOMIC would be nice to avoid... and doing > GFP_ATOMIC from normal process context just because of task_state > seems ... wrong. Well, then you will need to change the rest of the logic and use a struct completion. Give the age and practical relevance of the driver I would recommend against making such drastic changes and let it just be in its awkward but correct state. Regards Oliver
Re: [PATCH] usb: yurex: Rearrange code not to need GFP_ATOMIC
Am Sonntag, den 20.09.2020, 10:44 +0200 schrieb Pavel Machek: > Move prepare to wait around, so that normal GFP_KERNEL allocation can > be used. > > Signed-off-by: Pavel Machek (CIP) > Acked-by: Alan Stern Ehm. Please recheck. > diff --git a/drivers/usb/misc/yurex.c b/drivers/usb/misc/yurex.c > index b2e09883c7e2..071f1debebba 100644 > --- a/drivers/usb/misc/yurex.c > +++ b/drivers/usb/misc/yurex.c > @@ -489,10 +489,10 @@ static ssize_t yurex_write(struct file *file, const > char __user *user_buffer, > } > > /* send the data as the control msg */ > - prepare_to_wait(>waitq, , TASK_INTERRUPTIBLE); > dev_dbg(>interface->dev, "%s - submit %c\n", __func__, > dev->cntl_buffer[0]); > - retval = usb_submit_urb(dev->cntl_urb, GFP_ATOMIC); > + retval = usb_submit_urb(dev->cntl_urb, GFP_KERNEL); URB completes here. wake_up() returns the task to RUNNING. > + prepare_to_wait(>waitq, , TASK_INTERRUPTIBLE); Task goes to TASK_INTERRUPTIBLE > if (retval >= 0) > timeout = schedule_timeout(YUREX_WRITE_TIMEOUT); Task turns into Sleeping Beauty until timeout > finish_wait(>waitq, ); And here task goes into error reporting as it checks timeout. Regards Oliver
Re: [PATCH v2] usb: serial: Repair FTDI FT232R bricked eeprom
Am Mittwoch, den 09.09.2020, 13:34 -0600 schrieb James Hilliard:
> This patch detects and reverses the effects of the malicious FTDI
> Windows driver version 2.12.00(FTDIgate).
Hi,
this raises questions.
Should we do this unconditionally without asking?
Does this belong into kernel space?
> +static int ftdi_repair_brick(struct usb_serial_port *port)
> +{
> + struct ftdi_private *priv = usb_get_serial_port_data(port);
> + int orig_latency;
> + int rv;
> + u16 *eeprom_data;
> + u16 checksum;
> + int eeprom_size;
> + int result;
> +
> + switch (priv->chip_type) {
> + case FT232RL:
> + eeprom_size = 0x40;
> + break;
> + default:
> + /* Unsupported for brick repair */
> + return 0;
> + }
> +
> + /* Latency timer needs to be 0x77 to unlock EEPROM programming */
> + if (priv->latency != 0x77) {
> + orig_latency = priv->latency;
> + priv->latency = 0x77;
> + rv = write_latency_timer(port);
> + priv->latency = orig_latency;
> + if (rv < 0)
> + return -EIO;
> + }
Do you really want to change this without returning to the original?
Regards
Oliver
Re: [PATCH] cdc-acm: rework notification_buffer resizing
Am Samstag, den 01.08.2020, 08:21 -0700 schrieb t...@redhat.com:
> From: Tom Rix
>
> Clang static analysis reports this error
>
> cdc-acm.c:409:3: warning: Use of memory after it is freed
> acm_process_notification(acm, (unsigned char *)dr);
>
> There are three problems, the first one is that dr is not reset
>
> The variable dr is set with
>
> if (acm->nb_index)
> dr = (struct usb_cdc_notification *)acm->notification_buffer;
>
> But if the notification_buffer is too small it is resized with
>
> if (acm->nb_size) {
> kfree(acm->notification_buffer);
> acm->nb_size = 0;
> }
> alloc_size = roundup_pow_of_two(expected_size);
> /*
>* kmalloc ensures a valid notification_buffer after a
>* use of kfree in case the previous allocation was too
>* small. Final freeing is done on disconnect.
>*/
> acm->notification_buffer =
> kmalloc(alloc_size, GFP_ATOMIC);
>
> dr should point to the new acm->notification_buffer.
>
> The second problem is any data in the notification_buffer is lost
> when the pointer is freed. In the normal case, the current data
> is accumulated in the notification_buffer here.
>
> memcpy(>notification_buffer[acm->nb_index],
> urb->transfer_buffer, copy_size);
>
> When a resize happens, anything before
> notification_buffer[acm->nb_index] is garbage.
>
> The third problem is the acm->nb_index is not reset on a
> resizing buffer error.
>
> So switch resizing to using krealloc and reassign dr and
> reset nb_index.
>
> Fixes: ea2583529cd1 ("cdc-acm: reassemble fragmented notifications")
>
> Signed-off-by: Tom Rix
Acked-by: Oliver Neukum
Re: [PATCH] cdc-acm: rework notification_buffer resizing
Am Samstag, den 01.08.2020, 08:21 -0700 schrieb t...@redhat.com: > From: Tom Rix > > Clang static analysis reports this error > > cdc-acm.c:409:3: warning: Use of memory after it is freed > acm_process_notification(acm, (unsigned char *)dr); > > There are three problems, the first one is that dr is not reset Hi, thank you, good catch. Regards Oliver
Re: [PATCH v2] Bluetooth: btusb: Reset port on cmd timeout
Am Mittwoch, den 24.06.2020, 11:11 -0700 schrieb Abhishek Pandit- Subedi: > QCA_ROME sometimes gets into a state where it is unresponsive to > commands. Since it doesn't have support for a reset gpio, reset the usb > port when this occurs instead. Hi, on first glance this looks like an unbalanced PM reference. It is not because the operation is suicidal, but this deserves a comment, unless you want to get a note telling you that you caused an imbalance every few weeks. Regards Oliver
Re: KASAN: use-after-free Read in usblp_bulk_read
Am Donnerstag, den 30.04.2020, 11:11 -0400 schrieb Alan Stern: > KASAN is documented. The difficulty is that this race is obviously > hard to trigger, and without the ability to reproduce it we can't run > diagnostics to find the underlying cause. > > We can't even ask syzbot to try running tests for us; without a valid > reproducer it won't agree to rerun the original test program. Very well. We are not going to find it without exceptional luck. Yet there may be a real issue, too. We simply do not know. How about the attached patch? Regards Oliver From 5ed23e0029cf10cf8dbdd790a190d7e2113560ae Mon Sep 17 00:00:00 2001 From: Oliver Neukum Date: Wed, 6 May 2020 11:05:41 +0200 Subject: [PATCH] usblp: poison URBs upon disconnect syzkaller reported an UB that should have been killed to be active. We do not understand it, but this should fix the issue if it is real. Signed-off-by: Oliver Neukum --- drivers/usb/class/usblp.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/usb/class/usblp.c b/drivers/usb/class/usblp.c index 0d8e3f3804a3..084c48c5848f 100644 --- a/drivers/usb/class/usblp.c +++ b/drivers/usb/class/usblp.c @@ -468,7 +468,8 @@ static int usblp_release(struct inode *inode, struct file *file) usb_autopm_put_interface(usblp->intf); if (!usblp->present) /* finish cleanup from disconnect */ - usblp_cleanup(usblp); + usblp_cleanup(usblp); /* any URBs must be dead */ + mutex_unlock(_mutex); return 0; } @@ -1375,9 +1376,11 @@ static void usblp_disconnect(struct usb_interface *intf) usblp_unlink_urbs(usblp); mutex_unlock(>mut); + usb_poison_anchored_urbs(>urbs); if (!usblp->used) usblp_cleanup(usblp); + mutex_unlock(_mutex); } -- 2.16.4
Re: KASAN: slab-out-of-bounds Read in hfa384x_usbin_callback
Am Freitag, den 20.03.2020, 12:28 -0700 schrieb syzbot: > Hello, > > syzbot found the following crash on: > > HEAD commit:e17994d1 usb: core: kcov: collect coverage from usb comple.. > git tree: https://github.com/google/kasan.git usb-fuzzer > console output: https://syzkaller.appspot.com/x/log.txt?x=11d74573e0 > kernel config: https://syzkaller.appspot.com/x/.config?x=5d64370c438bc60 > dashboard link: https://syzkaller.appspot.com/bug?extid=7d42d68643a35f71ac8a > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=15fa561de0 > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=15d74573e0 > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > Reported-by: syzbot+7d42d68643a35f71a...@syzkaller.appspotmail.com > Hi, is this bug still active and can a test be run on it? I requested one yesterday. If my analysis is correct this bug has security implications, so it is kind of important. Regards Oliver
Re: KASAN: slab-out-of-bounds Write in betop_probe
Am Montag, den 10.02.2020, 17:16 -0800 schrieb syzbot: > Hello, > > syzbot found the following crash on: > > HEAD commit:e5cd56e9 usb: gadget: add raw-gadget interface > git tree: https://github.com/google/kasan.git usb-fuzzer > console output: https://syzkaller.appspot.com/x/log.txt?x=1517fed9e0 > kernel config: https://syzkaller.appspot.com/x/.config?x=8cff427cc8996115 > dashboard link: https://syzkaller.appspot.com/bug?extid=07efed3bc5a1407bd742 > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=147026b5e0 > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1683b6b5e0 > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > Reported-by: syzbot+07efed3bc5a1407bd...@syzkaller.appspotmail.com > > betop 0003:20BC:5500.0001: unknown main item tag 0x0 > betop 0003:20BC:5500.0001: hidraw0: USB HID v0.00 Device [HID 20bc:5500] on > usb-dummy_hcd.0-1/input0 > == > BUG: KASAN: slab-out-of-bounds in set_bit > include/asm-generic/bitops/instrumented-atomic.h:28 [inline] > BUG: KASAN: slab-out-of-bounds in betopff_init drivers/hid/hid-betopff.c:99 > [inline] > BUG: KASAN: slab-out-of-bounds in betop_probe+0x396/0x570 > drivers/hid/hid-betopff.c:134 > Write of size 8 at addr 8881d4f43ac0 by task kworker/1:2/94 > > Freed by task 12: > save_stack+0x1b/0x80 mm/kasan/common.c:72 > set_track mm/kasan/common.c:80 [inline] > kasan_set_free_info mm/kasan/common.c:337 [inline] > __kasan_slab_free+0x117/0x160 mm/kasan/common.c:476 > slab_free_hook mm/slub.c:1444 [inline] > slab_free_freelist_hook mm/slub.c:1477 [inline] > slab_free mm/slub.c:3024 [inline] > kfree+0xd5/0x300 mm/slub.c:3976 > urb_destroy drivers/usb/core/urb.c:26 [inline] > kref_put include/linux/kref.h:65 [inline] > Hi, this indicates that I am confused. Why are we getting an out-of-bounds on a freed region? Is this a strange way of reporting access to already freed memory? Regards Oliver
Re: KASAN: slab-out-of-bounds Read in hfa384x_usbin_callback
Am Freitag, den 20.03.2020, 12:28 -0700 schrieb syzbot:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit:e17994d1 usb: core: kcov: collect coverage from usb comple..
> git tree: https://github.com/google/kasan.git usb-fuzzer
> console output: https://syzkaller.appspot.com/x/log.txt?x=11d74573e0
> kernel config: https://syzkaller.appspot.com/x/.config?x=5d64370c438bc60
> dashboard link: https://syzkaller.appspot.com/bug?extid=7d42d68643a35f71ac8a
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=15fa561de0
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=15d74573e0
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+7d42d68643a35f71a...@syzkaller.appspotmail.com
>
> ==
> BUG: KASAN: slab-out-of-bounds in memcpy include/linux/string.h:381 [inline]
> BUG: KASAN: slab-out-of-bounds in skb_put_data include/linux/skbuff.h:2284
> [inline]
> BUG: KASAN: slab-out-of-bounds in hfa384x_int_rxmonitor
> drivers/staging/wlan-ng/hfa384x_usb.c:3412 [inline]
> BUG: KASAN: slab-out-of-bounds in hfa384x_usbin_rx
> drivers/staging/wlan-ng/hfa384x_usb.c:3312 [inline]
> BUG: KASAN: slab-out-of-bounds in hfa384x_usbin_callback+0x1993/0x2360
> drivers/staging/wlan-ng/hfa384x_usb.c:3026
> Read of size 19671 at addr 8881d226413c by task swapper/0/0
#syz test: https://github.com/google/kasan.git e17994d1From 6dbcac8c4b645600161feafc5576657905f15d65 Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Tue, 5 May 2020 13:46:26 +0200
Subject: [PATCH] hfa384x_usb: fix buffer overflow
The driver trusts the data_len coming from the hardware
without verification. That means that this opens
a vector by which an attacker can smash 64K of the heap.
Signed-off-by: Oliver Neukum
---
drivers/staging/wlan-ng/hfa384x_usb.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/wlan-ng/hfa384x_usb.c b/drivers/staging/wlan-ng/hfa384x_usb.c
index fa1bf8b069fd..5b6497d8c9e2 100644
--- a/drivers/staging/wlan-ng/hfa384x_usb.c
+++ b/drivers/staging/wlan-ng/hfa384x_usb.c
@@ -3353,9 +3353,9 @@ static void hfa384x_int_rxmonitor(struct wlandevice *wlandev,
struct hfa384x_usb_rxfrm *rxfrm)
{
struct hfa384x_rx_frame *rxdesc = >desc;
- unsigned int hdrlen = 0;
- unsigned int datalen = 0;
- unsigned int skblen = 0;
+ unsigned int hdrlen;
+ unsigned int datalen;
+ unsigned int skblen;
u8 *datap;
u16 fc;
struct sk_buff *skb;
@@ -3413,8 +3413,10 @@ static void hfa384x_int_rxmonitor(struct wlandevice *wlandev,
*/
skb_put_data(skb, >frame_control, hdrlen);
- /* If any, copy the data from the card to the skb */
- if (datalen > 0) {
+ /* If any, copy the data from the card to the skb,
+ * as long as it fits, lest we smash a buffer
+ */
+ if (datalen > 0 && datalen <= skblen - hdrlen) {
datap = skb_put_data(skb, rxfrm->data, datalen);
/* check for unencrypted stuff if WEP bit set. */
--
2.16.4
Re: general protection fault in go7007_usb_probe
Am Dienstag, den 21.04.2020, 16:36 -0700 schrieb syzbot:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit:e9010320 usb: cdns3: gadget: make a bunch of functions sta..
> git tree: https://github.com/google/kasan.git usb-fuzzer
> console output: https://syzkaller.appspot.com/x/log.txt?x=1263a93010
> kernel config: https://syzkaller.appspot.com/x/.config?x=bd14feb44652cfaf
> dashboard link: https://syzkaller.appspot.com/bug?extid=cabfa4b5b05ff6be4ef0
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
>
> Unfortunately, I don't have any reproducer for this crash yet.
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+cabfa4b5b05ff6be4...@syzkaller.appspotmail.com
#syz test: https://github.com/google/kasan.git e9010320
From a401b9ef89c4cfbdeb3993d68b51423e908be7a5 Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Wed, 22 Apr 2020 13:49:55 +0200
Subject: [PATCH] go7007: add sanity checking for endpoints
A malicious USB device may lack endpoints the driver assumes to exist
Accessing them leads to NULL pointer accesses. This patch introduces
sanity checking.
Signed-off-by: Oliver Neukum
Fixes: 866b8695d67e8 ("Staging: add the go7007 video driver")
---
drivers/media/usb/go7007/go7007-usb.c | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/media/usb/go7007/go7007-usb.c b/drivers/media/usb/go7007/go7007-usb.c
index f889c9d740cd..dbf0455d5d50 100644
--- a/drivers/media/usb/go7007/go7007-usb.c
+++ b/drivers/media/usb/go7007/go7007-usb.c
@@ -1132,6 +1132,10 @@ static int go7007_usb_probe(struct usb_interface *intf,
go->hpi_ops = _usb_onboard_hpi_ops;
go->hpi_context = usb;
+ ep = usb->usbdev->ep_in[4];
+ if (!ep)
+ return -ENODEV;
+
/* Allocate the URB and buffer for receiving incoming interrupts */
usb->intr_urb = usb_alloc_urb(0, GFP_KERNEL);
if (usb->intr_urb == NULL)
@@ -1141,7 +1145,6 @@ static int go7007_usb_probe(struct usb_interface *intf,
if (usb->intr_urb->transfer_buffer == NULL)
goto allocfail;
- ep = usb->usbdev->ep_in[4];
if (usb_endpoint_type(>desc) == USB_ENDPOINT_XFER_BULK)
usb_fill_bulk_urb(usb->intr_urb, usb->usbdev,
usb_rcvbulkpipe(usb->usbdev, 4),
@@ -1263,9 +1266,13 @@ static int go7007_usb_probe(struct usb_interface *intf,
/* Allocate the URBs and buffers for receiving the video stream */
if (board->flags & GO7007_USB_EZUSB) {
+ if (!usb->usbdev->ep_in[6])
+ goto allocfail;
v_urb_len = 1024;
video_pipe = usb_rcvbulkpipe(usb->usbdev, 6);
} else {
+ if (!usb->usbdev->ep_in[1])
+ goto allocfail;
v_urb_len = 512;
video_pipe = usb_rcvbulkpipe(usb->usbdev, 1);
}
@@ -1285,6 +1292,8 @@ static int go7007_usb_probe(struct usb_interface *intf,
/* Allocate the URBs and buffers for receiving the audio stream */
if ((board->flags & GO7007_USB_EZUSB) &&
(board->main_info.flags & GO7007_BOARD_HAS_AUDIO)) {
+ if (!usb->usbdev->ep_in[8])
+ goto allocfail;
for (i = 0; i < 8; ++i) {
usb->audio_urbs[i] = usb_alloc_urb(0, GFP_KERNEL);
if (usb->audio_urbs[i] == NULL)
--
2.16.4
Re: [PATCH] xhci: Prevent runtime suspend all the time with XHCI_RESET_ON_RESUME quirk
Am Montag, den 04.05.2020, 17:19 +0800 schrieb Kai-Heng Feng: > Etron EJ168 USB 3.0 Host Controller stops working after S3, if it was > runtime suspended previously: > [ 370.080359] pci :02:00.0: can't change power state from D3cold to D0 > (config space inaccessible) Apparently this controller has issues with D3cold > [ 370.080477] xhci_hcd :04:00.0: can't change power state from D3cold to > D0 (config space inaccessible) > [ 370.080532] pcieport :00:1c.0: DPC: containment event, status:0x1f05 > source:0x0200 > [ 370.080533] pcieport :00:1c.0: DPC: ERR_FATAL detected > [ 370.080536] xhci_hcd :04:00.0: can't change power state from D3hot to > D0 (config space inaccessible) > [ 370.080552] xhci_hcd :04:00.0: AER: can't recover (no error_detected > callback) > [ 370.080566] usb usb3: root hub lost power or was reset > [ 370.080566] usb usb4: root hub lost power or was reset > [ 370.080572] xhci_hcd :04:00.0: Host halt failed, -19 > [ 370.080574] xhci_hcd :04:00.0: Host not accessible, reset failed. > [ 370.080575] xhci_hcd :04:00.0: PCI post-resume error -19! > [ 370.080586] xhci_hcd :04:00.0: HC died; cleaning up > > This can be fixed by not runtime suspend the controller at all. > > So instead of conditionally runtime suspend the controller, always > prevent runtime suspend with XHCI_RESET_ON_RESUME quirk. What does that do to other controllers that can do runtime suspend under the current scheme? Regards Oliver
Re: general protection fault in go7007_usb_probe
Am Dienstag, den 21.04.2020, 16:36 -0700 schrieb syzbot:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit:e9010320 usb: cdns3: gadget: make a bunch of functions sta..
> git tree: https://github.com/google/kasan.git usb-fuzzer
> console output: https://syzkaller.appspot.com/x/log.txt?x=1263a93010
> kernel config: https://syzkaller.appspot.com/x/.config?x=bd14feb44652cfaf
> dashboard link: https://syzkaller.appspot.com/bug?extid=cabfa4b5b05ff6be4ef0
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
>
> Unfortunately, I don't have any reproducer for this crash yet.
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+cabfa4b5b05ff6be4...@syzkaller.appspotmail.com
#syz test: https://github.com/google/kasan.git e9010320
From a006a724881bfb592db61ba17db1e217b9615cc4 Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Wed, 22 Apr 2020 13:49:55 +0200
Subject: [PATCH] go7007: add only insanity checking
A malicious USB device may lack endpoints the driver assumes to exist
Accessing them leads to NULL pointer accesses. This patch introduces
sanity checking.
Signed-off-by: Oliver Neukum
Fixes: 866b8695d67e8 ("Staging: add the go7007 video driver")
---
drivers/media/usb/go7007/go7007-usb.c | 21 -
1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/drivers/media/usb/go7007/go7007-usb.c b/drivers/media/usb/go7007/go7007-usb.c
index f889c9d740cd..a45af149cadc 100644
--- a/drivers/media/usb/go7007/go7007-usb.c
+++ b/drivers/media/usb/go7007/go7007-usb.c
@@ -1132,6 +1132,20 @@ static int go7007_usb_probe(struct usb_interface *intf,
go->hpi_ops = _usb_onboard_hpi_ops;
go->hpi_context = usb;
+ if (!usb->usbdev) {
+ printk(KERN_ERR"It is full of stars!\n");
+ BUG();
+ }
+
+ if (!usb->usbdev->ep_in) {
+ printk(KERN_ERR"It is also full of stars!\n");
+ BUG();
+ }
+
+ ep = usb->usbdev->ep_in[4];
+ if (!ep)
+ return -ENODEV;
+
/* Allocate the URB and buffer for receiving incoming interrupts */
usb->intr_urb = usb_alloc_urb(0, GFP_KERNEL);
if (usb->intr_urb == NULL)
@@ -1141,7 +1155,6 @@ static int go7007_usb_probe(struct usb_interface *intf,
if (usb->intr_urb->transfer_buffer == NULL)
goto allocfail;
- ep = usb->usbdev->ep_in[4];
if (usb_endpoint_type(>desc) == USB_ENDPOINT_XFER_BULK)
usb_fill_bulk_urb(usb->intr_urb, usb->usbdev,
usb_rcvbulkpipe(usb->usbdev, 4),
@@ -1263,9 +1276,13 @@ static int go7007_usb_probe(struct usb_interface *intf,
/* Allocate the URBs and buffers for receiving the video stream */
if (board->flags & GO7007_USB_EZUSB) {
+ if (!usb->usbdev->ep_in[6])
+ goto allocfail;
v_urb_len = 1024;
video_pipe = usb_rcvbulkpipe(usb->usbdev, 6);
} else {
+ if (!usb->usbdev->ep_in[1])
+ goto allocfail;
v_urb_len = 512;
video_pipe = usb_rcvbulkpipe(usb->usbdev, 1);
}
@@ -1285,6 +1302,8 @@ static int go7007_usb_probe(struct usb_interface *intf,
/* Allocate the URBs and buffers for receiving the audio stream */
if ((board->flags & GO7007_USB_EZUSB) &&
(board->main_info.flags & GO7007_BOARD_HAS_AUDIO)) {
+ if (!usb->usbdev->ep_in[8])
+ goto allocfail;
for (i = 0; i < 8; ++i) {
usb->audio_urbs[i] = usb_alloc_urb(0, GFP_KERNEL);
if (usb->audio_urbs[i] == NULL)
--
2.16.4
Re: KASAN: use-after-free Read in usblp_bulk_read
Am Dienstag, den 21.04.2020, 08:35 -0700 schrieb syzbot: > Hello, > > syzbot found the following crash on: > > HEAD commit:0fa84af8 Merge tag 'usb-serial-5.7-rc1' of https://git.ker.. > git tree: https://github.com/google/kasan.git usb-fuzzer > console output: https://syzkaller.appspot.com/x/log.txt?x=126f75d7e0 > kernel config: https://syzkaller.appspot.com/x/.config?x=6b9c154b0c23aecf > dashboard link: https://syzkaller.appspot.com/bug?extid=be5b5f86a162a6c281e6 > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > > Unfortunately, I don't have any reproducer for this crash yet. > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > Reported-by: syzbot+be5b5f86a162a6c28...@syzkaller.appspotmail.com > > usblp0: nonzero read bulk status received: -71 OK, we have this report and nobody understands it. If I may summarize: 1. We do not conclusively know how the URB was submitted 2. We are clear about which memory was freed and accessed 3. We agree that the URB should have been unlinked Do we agree on what we agree on? Theories: A. There is a race that would allow disconnect() and resume() to run concurrently B. There is a race in usblp which affects 'used' C. There is a bug in the virtual driver that can make unlinking an URB fail What do you think? How to investigate this further and is it worth it? Do we have documentation on what KASAN does? Regards Oliver
Re: KMSAN: uninit-value in ax88172a_bind
Am Montag, den 14.10.2019, 22:10 -0700 schrieb syzbot:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit:fa169025 kmsan: get rid of unused static functions in kmsa..
> git tree: https://github.com/google/kmsan.git master
> console output: https://syzkaller.appspot.com/x/log.txt?x=1432a65360
> kernel config: https://syzkaller.appspot.com/x/.config?x=49548798e87d32d7
> dashboard link: https://syzkaller.appspot.com/bug?extid=a8d4acdad35e6bbca308
> compiler: clang version 9.0.0 (/home/glider/llvm/clang
> 80fee25776c2fb61e74c1ecb1a523375c2500b69)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=14743a6f60
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=125bdbc760
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+a8d4acdad35e6bbca...@syzkaller.appspotmail.com
#syz test: https://github.com/google/kmsan.git fa169025
From a6fd7a04a330a8bfad836b20843ea5fe26e0ae38 Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Thu, 17 Oct 2019 15:12:33 +0200
Subject: [PATCH] asix: fix information leak on short answers
If a malicious device gives a short MAC it can elicit up to
5 bytes of leaked memory out of the driver. We need to check for
ETH_ALEN.
Signed-off-by: Oliver Neukum
---
drivers/net/usb/ax88172a.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/usb/ax88172a.c b/drivers/net/usb/ax88172a.c
index 011bd4cb546e..af3994e0853b 100644
--- a/drivers/net/usb/ax88172a.c
+++ b/drivers/net/usb/ax88172a.c
@@ -196,7 +196,7 @@ static int ax88172a_bind(struct usbnet *dev, struct usb_interface *intf)
/* Get the MAC address */
ret = asix_read_cmd(dev, AX_CMD_READ_NODE_ID, 0, 0, ETH_ALEN, buf, 0);
- if (ret < 0) {
+ if (ret < ETH_ALEN) {
netdev_err(dev->net, "Failed to read MAC address: %d\n", ret);
goto free;
}
--
2.16.4
Re: [RFC PATCH 20/22] thunderbolt: Add support for USB tunnels
Am Dienstag, den 01.10.2019, 14:38 +0300 schrieb Mika Westerberg:
> From: Rajmohan Mani
>
> USB4 added a capability to tunnel USB 3.x protocol over the USB4
> fabric. USB4 device routers may include integrated SuperSpeed HUB or a
> function or both. USB tunneling follows PCIe so that the tunnel is
> created between the parent and the child router from USB downstream
> adapter port to USB upstream adapter port over a single USB4 link.
>
> This adds support for USB tunneling and also capability to discover
> existing USB tunnels (for example created by connection manager in boot
> firmware).
>
> Signed-off-by: Rajmohan Mani
> Co-developed-by: Mika Westerberg
> Signed-off-by: Mika Westerberg
> ---
> drivers/thunderbolt/switch.c | 35
> drivers/thunderbolt/tb.c | 153 ++--
> drivers/thunderbolt/tb.h | 15
> drivers/thunderbolt/tb_regs.h | 9 +-
> drivers/thunderbolt/tunnel.c | 158 +-
> drivers/thunderbolt/tunnel.h | 9 ++
> drivers/thunderbolt/usb4.c| 41 -
> 7 files changed, 393 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
> index 58e3f54ddbb9..5a3236fefb76 100644
> --- a/drivers/thunderbolt/switch.c
> +++ b/drivers/thunderbolt/switch.c
> @@ -1025,11 +1025,46 @@ bool tb_port_is_enabled(struct tb_port *port)
> case TB_TYPE_DP_HDMI_OUT:
> return tb_dp_port_is_enabled(port);
>
> + case TB_TYPE_USB_UP:
> + case TB_TYPE_USB_DOWN:
> + return tb_usb_port_is_enabled(port);
> +
> default:
> return false;
> }
> }
>
> +/**
> + * tb_usb_port_is_enabled() - Is the USB adapter port enabled
> + * @port: USB port to check
> + */
> +bool tb_usb_port_is_enabled(struct tb_port *port)
Should these functions be called tb_usb3_port... ?
This looks like all USB would need this.
> -static const char * const tb_tunnel_names[] = { "PCI", "DP", "DMA" };
> +static const char * const tb_tunnel_names[] = { "PCI", "DP", "DMA", "USB"};
USB3 ?
Regards
Oliver
Re: [RFC PATCH 06/22] thunderbolt: Add support for lane bonding
Am Mittwoch, den 02.10.2019, 17:30 +0300 schrieb Mika Westerberg: > On Wed, Oct 02, 2019 at 04:21:06PM +0200, Oliver Neukum wrote: > > Am Dienstag, den 01.10.2019, 15:53 +0300 schrieb Mika Westerberg: > > > > > > > > Are we only going to be allowed to "bond" two links together? Or will > > > > we be doing more than 2 in the future? If more, then we might want to > > > > think of a different way to specify these... > > > > > > AFAICT only two lanes are available in USB4. This goes over USB type-C > > > using the two lanes there. > > > > > > Of course I don't know if in future there will be USB4 1.1 or something > > > that adds more lanes so if you think there is a better way to specify > > > these, I'm happy to implement that instead :) > > > > If this ever can become asymmetric this interface is going to turn > > around and bite. > > Don't think it can be asymmetric but I'm open to all ideas how to make > it more flexible :-) Split the the attributes into link_speed_rx and link_speed_tx. For link_width likewise. We had the same issue with USB. Regards Oliver
Re: [RFC PATCH 06/22] thunderbolt: Add support for lane bonding
Am Dienstag, den 01.10.2019, 15:53 +0300 schrieb Mika Westerberg: > > > > Are we only going to be allowed to "bond" two links together? Or will > > we be doing more than 2 in the future? If more, then we might want to > > think of a different way to specify these... > > AFAICT only two lanes are available in USB4. This goes over USB type-C > using the two lanes there. > > Of course I don't know if in future there will be USB4 1.1 or something > that adds more lanes so if you think there is a better way to specify > these, I'm happy to implement that instead :) If this ever can become asymmetric this interface is going to turn around and bite. Regards Oliver
Re: [RFC PATCH 05/22] thunderbolt: Add helper macros to iterate over switch ports
Am Dienstag, den 01.10.2019, 14:38 +0300 schrieb Mika Westerberg:
> @@ -1975,10 +1972,8 @@ void tb_switch_suspend(struct tb_switch *sw)
> if (err)
> return;
>
> - for (i = 1; i <= sw->config.max_port_number; i++) {
> - if (tb_port_has_remote(>ports[i]))
> - tb_switch_suspend(sw->ports[i].remote->sw);
> - }
> + tb_switch_for_each_remote_port(sw, i)
> + tb_switch_suspend(sw->ports[i].remote->sw);
This macro looks a bit prone to misunderstanding.
I guess the function would be better if the test could be seen.
Regards
Oliver
Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4
Am Dienstag, den 01.10.2019, 18:07 +0300 schrieb Mika Westerberg: Hi, > OK, but does that break existing .configs? I mean if you have already > CONFIG_THUNDERBOLT in your .config/defconfig does it now just get > dropped silently? People will have to look at this new stuff anyway. > For example firewire has CONFIG_FIREWIRE even though the "standard" name > is IEEE 1394. I was thinking maybe we can do the same for > USB4/Thunderbolt USB and Thunderbolt used to be distinct protocols. Whereas Firewire was just a colloquial name for IEEE1394. Please be wordy here. "Unified support for USB4 and Thunderbolt4" Regards Oliver
Re: WARNING in _chaoskey_fill/usb_submit_urb
Am Montag, den 23.09.2019, 07:31 -0700 schrieb syzbot:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit:e0bd8d79 usb-fuzzer: main usb gadget fuzzer driver
> git tree: https://github.com/google/kasan.git usb-fuzzer
> console output: https://syzkaller.appspot.com/x/log.txt?x=1452c6a160
> kernel config: https://syzkaller.appspot.com/x/.config?x=8847e5384a16f66a
> dashboard link: https://syzkaller.appspot.com/bug?extid=f5349b421c6213d34ce2
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=16342d4560
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=166769b160
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+f5349b421c6213d34...@syzkaller.appspotmail.com
#syz test: https://github.com/google/kasan.git e0bd8d79
From b80b39a2565a80f16ce007982babe753e225ea83 Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Mon, 30 Sep 2019 15:19:13 +0200
Subject: [PATCH] USB: chaoskey: fix error case of a timeout
In case of a timeout communication with the device needs to be ended
from the host side, lest we overwrite an active URB
Signed-off-by: Oliver Neukum
---
drivers/usb/misc/chaoskey.c | 17 ++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/misc/chaoskey.c b/drivers/usb/misc/chaoskey.c
index cf5828ce927a..3cb7e1b7d454 100644
--- a/drivers/usb/misc/chaoskey.c
+++ b/drivers/usb/misc/chaoskey.c
@@ -383,13 +383,17 @@ static int _chaoskey_fill(struct chaoskey *dev)
!dev->reading,
(started ? NAK_TIMEOUT : ALEA_FIRST_TIMEOUT) );
- if (result < 0)
+ if (result < 0) {
+ usb_kill_urb(dev->urb);
goto out;
+ }
- if (result == 0)
+ if (result == 0) {
result = -ETIMEDOUT;
- else
+ usb_kill_urb(dev->urb);
+ } else {
result = dev->valid;
+ }
out:
/* Let the device go back to sleep eventually */
usb_autopm_put_interface(dev->interface);
@@ -525,7 +529,14 @@ static int chaoskey_suspend(struct usb_interface *interface,
static int chaoskey_resume(struct usb_interface *interface)
{
+ struct chaoskey *dev;
+ struct usb_device *udev = interface_to_usbdev(interface);
+
usb_dbg(interface, "resume");
+ dev = usb_get_intfdata(interface);
+ if (le16_to_cpu(udev->descriptor.idVendor) == ALEA_VENDOR_ID)
+ dev->reads_started = false;
+
return 0;
}
#else
--
2.16.4
Re: WARNING in _chaoskey_fill/usb_submit_urb
Am Montag, den 23.09.2019, 07:31 -0700 schrieb syzbot: > Hello, > > syzbot found the following crash on: > > HEAD commit:e0bd8d79 usb-fuzzer: main usb gadget fuzzer driver > git tree: https://github.com/google/kasan.git usb-fuzzer > console output: https://syzkaller.appspot.com/x/log.txt?x=1452c6a160 > kernel config: https://syzkaller.appspot.com/x/.config?x=8847e5384a16f66a > dashboard link: https://syzkaller.appspot.com/bug?extid=f5349b421c6213d34ce2 > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=16342d4560 > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=166769b160 > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > Reported-by: syzbot+f5349b421c6213d34...@syzkaller.appspotmail.com #syz test: https://github.com/google/kasan.git e0bd8d79From 27b0085768b55f2ed8faf4f1254023a03dc3eb24 Mon Sep 17 00:00:00 2001 From: Oliver Neukum Date: Mon, 30 Sep 2019 15:19:13 +0200 Subject: [PATCH] USB: chaoskey: fix error case of a timeout In case of a timeout communication with the device needs to be ended from the host side, lest we overwrite an active URB Signed-off-by: Oliver Neukum --- drivers/usb/misc/chaoskey.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/usb/misc/chaoskey.c b/drivers/usb/misc/chaoskey.c index cf5828ce927a..850f46cbace2 100644 --- a/drivers/usb/misc/chaoskey.c +++ b/drivers/usb/misc/chaoskey.c @@ -391,6 +391,8 @@ static int _chaoskey_fill(struct chaoskey *dev) else result = dev->valid; out: + /* in case of a timeout */ + usb_kill_urb(dev->urb); /* Let the device go back to sleep eventually */ usb_autopm_put_interface(dev->interface); -- 2.16.4
Re: 4f5368b5541a902f6596558b05f5c21a9770dd32 causes regression
Am Mittwoch, den 25.09.2019, 21:48 +0200 schrieb Daniel Vetter: > which undoes any side-effect of the patch you're pointing at. I am > rather surprised though that this results in a hard-lookup for you, > did you confirm the bisect by reverting that commit on top of latest > upstream? Hi, yes, a straight revert fixes the issue. Regards Oliver
4f5368b5541a902f6596558b05f5c21a9770dd32 causes regression
Hi, I am seeing a hard lockup during boot with this patch. I am using only the laptop's internal display. The last message I see is: kvm: disabled by BIOS Regards Oliver devices are: 00:00.0 Host bridge [0600]: Intel Corporation Xeon E3-1200 v5/E3-1500 v5/6th Gen Core Processor Host Bridge/DRAM Registers [8086:1910] (rev 07) Subsystem: Hewlett-Packard Company Device [103c:80d5] Flags: bus master, fast devsel, latency 0 Capabilities: [e0] Vendor Specific Information: Len=10 Kernel driver in use: skl_uncore 00:01.0 PCI bridge [0604]: Intel Corporation Xeon E3-1200 v5/E3-1500 v5/6th Gen Core Processor PCIe Controller (x16) [8086:1901] (rev 07) (prog-if 00 [Normal decode]) Flags: bus master, fast devsel, latency 0, IRQ 120 Bus: primary=00, secondary=01, subordinate=01, sec-latency=0 I/O behind bridge: 3000-3fff [size=4K] Memory behind bridge: dc00-dc0f [size=1M] Prefetchable memory behind bridge: 4000-4fff [size=256M] Capabilities: [88] Subsystem: Hewlett-Packard Company Device [103c:80d5] Capabilities: [80] Power Management version 3 Capabilities: [90] MSI: Enable+ Count=1/1 Maskable- 64bit- Capabilities: [a0] Express Root Port (Slot+), MSI 00 Capabilities: [100] Virtual Channel Capabilities: [140] Root Complex Link Capabilities: [d94] #19 Kernel driver in use: pcieport 00:02.0 VGA compatible controller [0300]: Intel Corporation HD Graphics 530 [8086:191b] (rev 06) (prog-if 00 [VGA controller]) Subsystem: Hewlett-Packard Company Device [103c:80d5] Flags: bus master, fast devsel, latency 0, IRQ 130 Memory at db00 (64-bit, non-prefetchable) [size=16M] Memory at 5000 (64-bit, prefetchable) [size=256M] I/O ports at 6000 [size=64] [virtual] Expansion ROM at 000c [disabled] [size=128K] Capabilities: [40] Vendor Specific Information: Len=0c Capabilities: [70] Express Root Complex Integrated Endpoint, MSI 00 Capabilities: [ac] MSI: Enable+ Count=1/1 Maskable- 64bit- Capabilities: [d0] Power Management version 2 Capabilities: [100] Process Address Space ID (PASID) Capabilities: [200] Address Translation Service (ATS) Capabilities: [300] Page Request Interface (PRI) Kernel driver in use: i915 Kernel modules: i915 00:14.0 USB controller [0c03]: Intel Corporation Sunrise Point-H USB 3.0 xHCI Controller [8086:a12f] (rev 31) (prog-if 30 [XHCI]) Subsystem: Hewlett-Packard Company Device [103c:80d5] Flags: bus master, medium devsel, latency 0, IRQ 125 Memory at dc32 (64-bit, non-prefetchable) [size=64K] Capabilities: [70] Power Management version 2 Capabilities: [80] MSI: Enable+ Count=1/8 Maskable- 64bit+ Kernel driver in use: xhci_hcd Kernel modules: xhci_pci
Re: [PATCH 0/2] Reset realtek bluetooth devices during user suspend
Am Dienstag, den 17.09.2019, 14:27 -0700 schrieb Abhishek Pandit- Subedi: > On a Realtek USB bluetooth device, I wanted a simple and consistent way > to put the device in reset during suspend (2 reasons: to save power and The device really uses less power if you reset it before suspendening it? > disable BT as a wakeup source). Resetting it in the suspend callback Then do not enable it. Something is strange. > causes a detach and the resume callback is not called. Hence the changes > in this series to do the reset in suspend_noirq. > > I looked into using PERSIST and reset on resume but those seem mainly > for misbehaving devices that reset themselves. No, not really. It is also for device that need to be reset if you use the RESET_RESUME quirk. But that is on resume(). You could introduce a new flag. But the _noirq method is kind of a hack, as USB really cannot operate without interrupts. Regards Oliver
Re: divide error in usbnet_update_max_qlen
Am Montag, den 16.09.2019, 06:29 -0700 schrieb syzbot:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit:f0df5c1b usb-fuzzer: main usb gadget fuzzer driver
> git tree: https://github.com/google/kasan.git usb-fuzzer
> console output: https://syzkaller.appspot.com/x/log.txt?x=117659fa60
> kernel config: https://syzkaller.appspot.com/x/.config?x=5c6633fa4ed00be5
> dashboard link: https://syzkaller.appspot.com/bug?extid=6102c120be558c885f04
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=12107ba960
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=146014e660
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+6102c120be558c885...@syzkaller.appspotmail.com
#syz test: https://github.com/google/kasan.git f0df5c1bFrom 57c2443b2a7678a6f7f6437f741f49f06a5104fb Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Tue, 17 Sep 2019 11:51:55 +0200
Subject: [PATCH] usbnet: sanity checking of packet sizes
Malicious devices can set this to zero and we divide by it.
Introduce sanity checking.
Signed-off-by: Oliver Neukum
---
drivers/net/usb/usbnet.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 58952a79b05f..e44849499b89 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -339,6 +339,8 @@ void usbnet_update_max_qlen(struct usbnet *dev)
{
enum usb_device_speed speed = dev->udev->speed;
+ if (!dev->rx_urb_size || !dev->hard_mtu)
+ goto insanity;
switch (speed) {
case USB_SPEED_HIGH:
dev->rx_qlen = MAX_QUEUE_MEMORY / dev->rx_urb_size;
@@ -355,6 +357,7 @@ void usbnet_update_max_qlen(struct usbnet *dev)
dev->tx_qlen = 5 * MAX_QUEUE_MEMORY / dev->hard_mtu;
break;
default:
+insanity:
dev->rx_qlen = dev->tx_qlen = 4;
}
}
--
2.16.4
Re: WARNING in hso_free_net_device
Am Donnerstag, den 05.09.2019, 22:05 -0400 schrieb Hui Peng: > > On 9/5/2019 7:24 AM, Andrey Konovalov wrote: > > On Thu, Sep 5, 2019 at 4:20 AM Hui Peng wrote: > > > > > > Can you guys have a look at the attached patch? > > > > Let's try it: > > > > #syz test: https://github.com/google/kasan.git eea39f24 > > > > FYI: there are two more reports coming from this driver, which might > > (or might not) have the same root cause. One of them has a suggested > > fix by Oliver. > > > > https://syzkaller.appspot.com/bug?extid=67b2bd0e34f952d0321e > > https://syzkaller.appspot.com/bug?extid=93f2f45b19519b289613 > > > > I think they are different, though similar. > This one is resulted from unregistering a network device. > These 2 are resulted from unregistering a tty device. Hi, looks like it. That may indeed be the issue. Please try to have syzbot test your patch and we will know more. Regards Oliver
Re: [PATCH] usb: storage: Add ums-cros-aoa driver
Am Dienstag, den 03.09.2019, 11:19 +0200 schrieb Greg KH: > On Tue, Sep 03, 2019 at 10:46:14AM +0200, Oliver Neukum wrote: > > Am Montag, den 02.09.2019, 18:47 +0200 schrieb Greg KH: > > > > > > This should work just fine today. Add a new device id to the "new_id" > > > file and then tell the driver to bind. That's pretty much the same as a > > > "force_bind", right? > > > > That looks like a race condition by design to me. > > How? You have one of these files and potentially multiple devices to be bound. You need a locking scheme. As soon as the acts of specifying and binding are distinct. Regards Oliver
Re: [PATCH] usb: storage: Add ums-cros-aoa driver
Am Montag, den 02.09.2019, 18:47 +0200 schrieb Greg KH: > > This should work just fine today. Add a new device id to the "new_id" > file and then tell the driver to bind. That's pretty much the same as a > "force_bind", right? That looks like a race condition by design to me. Regards Oliver
Re: [PATCH] HID: quirks: Disable runtime suspend on Microsoft Corp. Basic Optical Mouse v2.0
Am Donnerstag, den 22.08.2019, 17:17 +0800 schrieb Kai-Heng Feng: > The optical sensor of the mouse gets turned off when it's runtime > suspended, so moving the mouse can't wake the mouse up, despite that > USB remote wakeup is successfully set. > > Introduce a new quirk to prevent the mouse from getting runtime > suspended. Hi, I am afraid this is a bad approach in principle. The device behaves according to spec. And it behaves like most hardware. If you do not want runtime PM for such devices, do not switch it on. The refcounting needs to be done correctly. This patch does something that udev should do and in a questionable manner. Regards Oliver
Re: KMSAN: uninit-value in smsc75xx_bind
Am Freitag, den 09.08.2019, 01:48 -0700 schrieb syzbot:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit:beaab8a3 fix KASAN build
> git tree: kmsan
[..]
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0x191/0x1f0 lib/dump_stack.c:113
> kmsan_report+0x162/0x2d0 mm/kmsan/kmsan_report.c:109
> __msan_warning+0x75/0xe0 mm/kmsan/kmsan_instr.c:294
> smsc75xx_wait_ready drivers/net/usb/smsc75xx.c:976 [inline]
> smsc75xx_bind+0x541/0x12d0 drivers/net/usb/smsc75xx.c:1483
>
> Local variable description: buf.i93@smsc75xx_bind
> Variable was created at:
> __smsc75xx_read_reg drivers/net/usb/smsc75xx.c:83 [inline]
> smsc75xx_wait_ready drivers/net/usb/smsc75xx.c:969 [inline]
> smsc75xx_bind+0x44c/0x12d0 drivers/net/usb/smsc75xx.c:1483
> usbnet_probe+0x10d3/0x3950 drivers/net/usb/usbnet.c:1722
Hi,
this looks like a false positive to me.
The offending code is likely this:
if (size) {
buf = kmalloc(size, GFP_KERNEL);
if (!buf)
goto out;
}
err = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
cmd, reqtype, value, index, buf, size,
USB_CTRL_GET_TIMEOUT);
which uses 'buf' uninitialized. But it is used for input.
What is happening here?
Regards
Oliver
Re: KASAN: slab-out-of-bounds Read in usbnet_generic_cdc_bind
Am Montag, den 12.08.2019, 14:27 +0200 schrieb Andrey Konovalov: > On > This one is funny, we do sizeof(struct usb_cdc_mdlm_desc *) instead of > sizeof(struct usb_cdc_mdlm_desc) and the same for > usb_cdc_mdlm_detail_desc in cdc_parse_cdc_header(). You are right. Old copy & paste error presumably. Patch is on the way. Regards Oliver
Re: WARNING in usbhid_raw_request/usb_submit_urb
Am Dienstag, den 13.08.2019, 12:26 +0800 schrieb Hillf Danton:
> [respin with the mess in Cc list cleaned up]
> Followup of commit e3e14de50dff ("HID: fix start/stop cycle in usbhid driver")
>
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -1214,6 +1214,8 @@ static void usbhid_stop(struct hid_devic
>
> hid->claimed = 0;
>
> + if (!usbhid->urbin) /* freeing buffers only once */
> + return;
> usb_free_urb(usbhid->urbin);
> usb_free_urb(usbhid->urbctrl);
> usb_free_urb(usbhid->urbout);
This looks rather suspicious. Why is stop() called multiple times?
Do we have a refcounting issue? If not, what controls locking?
Regards
Oliver
Re: KASAN: use-after-free Read in device_release_driver_internal
Am Dienstag, den 06.08.2019, 14:50 +0200 schrieb Andrey Konovalov:
> On Tue, Aug 6, 2019 at 2:36 PM Oliver Neukum wrote:
> >
> > Am Donnerstag, den 01.08.2019, 14:47 -0400 schrieb Alan Stern:
> > >
> > > I think this must be caused by an unbalanced refcount. That is,
> > > something must drop one more reference to the device than it takes.
> > > That would explain why the invalid access occurs inside a single
> > > bus_remove_device() call, between the klist_del() and
> > > device_release_driver().
> > >
> > > The kernel log indicates that the device was probed by rndis_wlan,
> > > rndis_host, and cdc_acm, all of which got errors because of the
> > > device's bogus descriptors. Probably one of them is messing up the
> > > refcount.
> >
> > Hi,
> >
> > you made me look at cdc-acm. I suspect
> >
> > cae2bc768d176bfbdad7035bbcc3cdc973eb7984 ("usb: cdc-acm: Decrement tty
> > port's refcount if probe() fail")
> >
> > is buggy decrementing the refcount on the interface in destroy()
> > even before the refcount is increased.
> >
> > Unfortunately I cannot tell from the bug report how many and which
> > interfaces the emulated test device has. Hence it is unclear to me,
> > when exactly probe() would fail cdc-acm.
> >
> > If you agree. I am attaching a putative fix.
>
> Let's see if it fixes the issue.
>
> #syz fix: https://github.com/google/kasan.git 6a3599ce
Hi,
did this ever produce a result? I saw none.
Regards
Oliver
Re: KASAN: use-after-free Read in cpia2_usb_disconnect
Am Mittwoch, den 03.07.2019, 10:10 -0700 schrieb Eric Biggers: > > Who are you talking to? If you want syzbot to test your patch, follow the > directions at > https://github.com/google/syzkaller/blob/master/docs/syzbot.md#testing-patches Thanks. I am trying this out now. Regards Oliver
Re: KASAN: use-after-free Read in cpia2_usb_disconnect
Am Dienstag, den 02.07.2019, 18:01 -0700 schrieb syzbot:
> syzbot has found a reproducer for the following crash on:
>
> HEAD commit:7829a896 usb-fuzzer: main usb gadget fuzzer driver
> git tree: https://github.com/google/kasan.git usb-fuzzer
> console output: https://syzkaller.appspot.com/x/log.txt?x=11e19043a0
> kernel config: https://syzkaller.appspot.com/x/.config?x=f6d4561982f71f63
> dashboard link: https://syzkaller.appspot.com/bug?extid=0c90fc937c84f97d0aa6
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=147d42eda0
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=104c268ba0
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+0c90fc937c84f97d0...@syzkaller.appspotmail.com
>
> cpia2: Message: count = 1, register[0] = 0x0
> cpia2: Unexpected error: -19
> ==
> BUG: KASAN: use-after-free in cpia2_usb_disconnect+0x1a4/0x1c0
> drivers/media/usb/cpia2/cpia2_usb.c:898
> Read of size 8 at addr 8881cf6c4e50 by task kworker/1:1/22
Please try this:
>From a0a73298fc23acb95e7b6487e960be707563eb34 Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Wed, 8 May 2019 12:36:40 +0200
Subject: [PATCH] cpia2_usb: first wake up, then free in disconnect
Kasan reported a use after free in cpia2_usb_disconnect()
It first freed everything and then woke up those waiting.
The reverse order is correct.
Signed-off-by: Oliver Neukum
Reported-by: syzbot+0c90fc937c84f97d0...@syzkaller.appspotmail.com
Fixes: 6c493f8b28c67 ("[media] cpia2: major overhaul to get it in a working
state again")
---
drivers/media/usb/cpia2/cpia2_usb.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/media/usb/cpia2/cpia2_usb.c
b/drivers/media/usb/cpia2/cpia2_usb.c
index e5d8dee38fe4..44bd7e5ad3eb 100644
--- a/drivers/media/usb/cpia2/cpia2_usb.c
+++ b/drivers/media/usb/cpia2/cpia2_usb.c
@@ -902,7 +902,6 @@ static void cpia2_usb_disconnect(struct usb_interface *intf)
cpia2_unregister_camera(cam);
v4l2_device_disconnect(>v4l2_dev);
mutex_unlock(>v4l2_lock);
- v4l2_device_put(>v4l2_dev);
if(cam->buffers) {
DBG("Wakeup waiting processes\n");
@@ -911,6 +910,8 @@ static void cpia2_usb_disconnect(struct usb_interface *intf)
wake_up_interruptible(>wq_stream);
}
+ v4l2_device_put(>v4l2_dev);
+
LOG("CPiA2 camera disconnected.\n");
}
--
2.16.4
Re: [PATCH] usb: image: microtek: Unneeded variable: "err". Return "0" on line 616
Am Montag, den 01.07.2019, 23:29 +0530 schrieb Hariprasad Kelam: > Fix below issue reported by coccicheck > drivers/usb/image/microtek.c:569:5-8: Unneeded variable: "err". Return > "0" on line 616 > > We can not change return type of mts_scsi_queuecommand_lck as it is part > of DEF_SCSI_QCMD > > Signed-off-by: Hariprasad Kelam Acked-by: Oliver Neukum
Re: [RFC] deadlock with flush_work() in UAS
Am Mittwoch, den 26.06.2019, 10:38 -0400 schrieb Alan Stern: > On Wed, 26 Jun 2019, Oliver Neukum wrote: > > > Am Montag, den 24.06.2019, 10:22 -0400 schrieb Alan Stern: > > > But that pattern makes no sense; a driver would never use it. The > > > driver would just do the reset itself. > > > > Correct. But UAS and storage themselves still need to use > > WQ_MEM_RECLAIM for their workqueues, don't they? > > Perhaps so for uas. usb-storage uses a work queue only for scanning > targets, which doesn't interfere with the block I/O pathway. Are you sure? What about hub_tt_work? As far as I can tell, hub_quiesce will flush it, hence it is used in error handling. Regards Oliver
Re: [RFC] deadlock with flush_work() in UAS
Am Montag, den 24.06.2019, 10:22 -0400 schrieb Alan Stern: > But that pattern makes no sense; a driver would never use it. The > driver would just do the reset itself. Correct. But UAS and storage themselves still need to use WQ_MEM_RECLAIM for their workqueues, don't they? Regards Oliver
Re: [RFC] deadlock with flush_work() in UAS
Am Donnerstag, den 20.06.2019, 07:10 -0700 schrieb Tejun Heo: > Hello, > > On Tue, Jun 18, 2019 at 11:59:39AM -0400, Alan Stern wrote: > > > > Even if you disagree, perhaps we should have a global workqueue with a > > > > permanently set noio flag. It could be shared among multiple drivers > > > > such as uas and the hub driver for purposes like this. (In fact, the > > > > hub driver already has its own dedicated workqueue.) > > > > > > That is a good idea. But does UAS need WQ_MEM_RECLAIM? > > > > These are good questions, and I don't have the answers. Perhaps Tejun > > or someone else on LKML can help. > > Any device which may host a filesystem or swap needs to use > WQ_MEM_RECLAIM workqueues on anything which may be used during normal > IOs including e.g. error handling which may be invoked. One > WQ_MEM_RECLAIM workqueue guarantees one level of concurrency for all > its tasks regardless of memory situation, so as long as there's no > interdependence between work items, the workqueue can be shared. Ouch. Alan, in that case anything doing a reset, suspend or resume needs to use WQ_MEM_RECLAIM, it looks to me. What do we do? Regards Oliver
Re: [IMX] [DRM]: suspend/resume support
Am Montag, den 17.06.2019, 19:19 +0530 schrieb Pintu Agarwal: > Currently, I am trying to understand what needs to be taken care > during suspend/resume. You need to take care of * wakeup sources * not requiring services of devices higher up in the tree. > With some reference, I figured out that hdmi power off/on needs to be > done during suspend/resume. That would make sense. First of all you need to understand that the generic model is, well, generic. Now this may look like a tautology, so let me explain. A generic model cannot tell you how to save power on a specific hardware. It exists to model dependencies among subsystems and to help you. The suspend() call is a notification which tells you that the rest of the system will not require your services until resume() is called(). That means that after resume() your driver must be functional again. And it means that between suspend() and resume() you cannot touch your device because what is above you in the tree need not be functional. > But after resume, system is hanging. > It seems like vblank events are not getting triggered after the resume. > May be irq remains disabled after resume, I need to figure out some > way to enable the all the irqs again. In your case it looks like parts of dw_hdmi_imx_bind() need to be redone in resume(). HTH Oliver
Re: KASAN: use-after-free Read in device_del
Am Montag, den 03.06.2019, 04:41 -0700 schrieb syzbot:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit:69bbe8c7 usb-fuzzer: main usb gadget fuzzer driver
> git tree: https://github.com/google/kasan.git usb-fuzzer
> console output: https://syzkaller.appspot.com/x/log.txt?x=1684d87ca0
> kernel config: https://syzkaller.appspot.com/x/.config?x=193d8457178b3229
> dashboard link: https://syzkaller.appspot.com/bug?extid=93f2f45b19519b289613
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
>
> Unfortunately, I don't have any reproducer for this crash yet.
Will this do?
Regards
Oliver
From 6867abc1701f18892d32e8aeaf644901e9bcbf82 Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Wed, 5 Jun 2019 13:49:21 +0200
Subject: [PATCH] usb: hso: initialize so that we can tear down in the error
case
Initualization must follow the sequence stuff is undone in case
we bail out. Thus the parent pointer must be set earlier.
Signed-off-by: Oliver Neukum
---
drivers/net/usb/hso.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 6a0ecddff310..4d9100fb9f6e 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -2653,6 +2653,9 @@ static struct hso_device *hso_create_bulk_serial_device(
BULK_URB_TX_SIZE))
goto exit;
+ /* and record this serial */
+ set_serial_by_index(serial->minor, serial);
+
serial->in_endp = hso_get_ep(interface, USB_ENDPOINT_XFER_BULK,
USB_DIR_IN);
if (!serial->in_endp) {
@@ -2669,9 +2672,6 @@ static struct hso_device *hso_create_bulk_serial_device(
serial->write_data = hso_std_serial_write_data;
- /* and record this serial */
- set_serial_by_index(serial->minor, serial);
-
/* setup the proc dirs and files if needed */
hso_log_port(hso_dev);
--
2.16.4
Re: [RFC PATCH] usb: host: xhci: allow __GFP_FS in dma allocation
Am Donnerstag, den 23.05.2019, 10:01 -0400 schrieb Alan Stern:
> On Wed, 22 May 2019, Oliver Neukum wrote:
>
> > On Mi, 2019-05-22 at 10:56 -0400, Alan Stern wrote:
> > > On Wed, 22 May 2019, Oliver Neukum wrote:
> > >
> > > > I agree with the problem, but I fail to see why this issue would be
> > > > specific to USB. Shouldn't this be done in the device core layer?
> > >
> > > Only for drivers that are on the block-device writeback path. The
> > > device core doesn't know which drivers these are.
> >
> > Neither does USB know. It is very hard to predict or even tell which
> > devices are block device drivers. I think we must assume that
> > any device may be affected.
>
> All right. Would you like to submit a patch?
Do you like this one?
Regards
Oliver
From 0dc9c7dfe994fc9c28a63ba283e4442c237f6989 Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Tue, 28 May 2019 11:43:02 +0200
Subject: [PATCH] base: force NOIO allocations during unplug
There is one overlooked situation under which a driver
must not do IO to allocate memory. You cannot do that
while disconnecting a device. A device being disconnected
is no longer functional in most cases, yet IO may fail
only when the handler runs.
Signed-off-by: Oliver Neukum
---
drivers/base/core.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index fd7511e04e62..a7f5f45bd761 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2229,6 +2229,7 @@ void device_del(struct device *dev)
struct device *parent = dev->parent;
struct kobject *glue_dir = NULL;
struct class_interface *class_intf;
+ unsigned int noio_flag;
/*
* Hold the device lock and set the "dead" flag to guarantee that
@@ -2256,6 +2257,7 @@ void device_del(struct device *dev)
device_remove_sys_dev_entry(dev);
device_remove_file(dev, _attr_dev);
}
+ noio_flag = memalloc_noio_save();
if (dev->class) {
device_remove_class_symlinks(dev);
@@ -2277,6 +2279,8 @@ void device_del(struct device *dev)
device_platform_notify(dev, KOBJ_REMOVE);
device_remove_properties(dev);
device_links_purge(dev);
+ memalloc_noio_restore(noio_flag);
+
if (dev->bus)
blocking_notifier_call_chain(>bus->p->bus_notifier,
--
2.16.4
Re: [RFC PATCH] usb: host: xhci: allow __GFP_FS in dma allocation
On So, 2019-05-19 at 22:56 -0700, Christoph Hellwig wrote: > Folks, you can't just pass arbitary GFP_ flags to dma allocation > routines, beause very often they are not just wrappers around > the page allocator. > > So no, you can't just fine grained control the flags, but the > existing code is just as buggy. > > Please switch to use memalloc_noio_save() instead. Thinking about this again, we have a problem. We introduced memalloc_noio_save() in 3.10 . Hence the code should have been correct in v4.14. Which means that either 6518202970c1 "(mm/cma: remove unsupported gfp_mask parameter from cma_alloc()" is buggy, or the original issue with a delay of 2 seconds still exist. Do we need to do something? Regards Oliver
Re: [RFC PATCH] usb: host: xhci: allow __GFP_FS in dma allocation
On Mi, 2019-05-22 at 10:56 -0400, Alan Stern wrote: > On Wed, 22 May 2019, Oliver Neukum wrote: > > > I agree with the problem, but I fail to see why this issue would be > > specific to USB. Shouldn't this be done in the device core layer? > > Only for drivers that are on the block-device writeback path. The > device core doesn't know which drivers these are. Neither does USB know. It is very hard to predict or even tell which devices are block device drivers. I think we must assume that any device may be affected. Regards Oliver
Re: [RFC PATCH] usb: host: xhci: allow __GFP_FS in dma allocation
On Di, 2019-05-21 at 10:00 -0400, Alan Stern wrote: > > Changing configurations amounts to much the same as disconnecting, > because both operations destroy all the existing interfaces. > > Disconnect can arise in two different ways. > > Physical hot-unplug: All I/O operations will fail. > > Rmmod or unbind: I/O operations will succeed. > > The second case is probably okay. The first we can do nothing about. > However, in either case we do need to make sure that memory allocations > do not require any writebacks. This suggests that we need to call > memalloc_noio_save() from within usb_unbind_interface(). I agree with the problem, but I fail to see why this issue would be specific to USB. Shouldn't this be done in the device core layer? Regards Oliver
Re: [RFC PATCH] usb: host: xhci: allow __GFP_FS in dma allocation
On Mo, 2019-05-20 at 10:16 -0400, Alan Stern wrote: > On Mon, 20 May 2019, Christoph Hellwig wrote: > > > GFP_KERNEL if you can block, GFP_ATOMIC if you can't for a good reason, > > that is the allocation is from irq context or under a spinlock. If you > > think you have a case where you think you don't want to block, but it > > is not because of the above reasons we need to have a chat about the > > details. > > What if the allocation requires the kernel to swap some old pages out > to the backing store, but the backing store is on the device that the > driver is managing? The swap can't take place until the current I/O > operation is complete (assuming the driver can handle only one I/O > operation at a time), and the current operation can't complete until > the old pages are swapped out. Result: deadlock. > > Isn't that the whole reason for using GFP_NOIO in the first place? Hi, lookig at this it seems to me that we are in danger of a deadlock - during reset - devices cannot do IO while being reset covered by the USB layer in usb_reset_device - resume & restore - devices cannot do IO while suspended covered by driver core in rpm_callback - disconnect - a disconnected device cannot do IO is this a theoretical case or should I do something to the driver core? How about changing configurations on USB? Regards Oliver
Re: [RFC PATCH] usb: host: xhci: allow __GFP_FS in dma allocation
On Mo, 2019-05-20 at 07:23 -0700, Christoph Hellwig wrote: > On Mon, May 20, 2019 at 10:16:57AM -0400, Alan Stern wrote: > > What if the allocation requires the kernel to swap some old pages out > > to the backing store, but the backing store is on the device that the > > driver is managing? The swap can't take place until the current I/O > > operation is complete (assuming the driver can handle only one I/O > > operation at a time), and the current operation can't complete until > > the old pages are swapped out. Result: deadlock. > > > > Isn't that the whole reason for using GFP_NOIO in the first place? > > It is, or rather was. As it has been incredibly painful to wire > up the gfp_t argument through some callstacks, most notably the > vmalloc allocator which is used by a lot of the DMA allocators on > non-coherent platforms, we now have the memalloc_noio_save and > memalloc_nofs_save functions that mark a thread as not beeing to > go into I/O / FS reclaim. So even if you use GFP_KERNEL you will > not dip into reclaim with those flags set on the thread. OK, but this leaves a question open. Will the GFP_NOIO actually hurt, if it is used after memalloc_noio_save()? Regards Oliver
Re: [RFC v2 3/5] clk: bcm2835: use firmware interface to update pllb
On Mo, 2019-05-20 at 12:47 +0200, Nicolas Saenz Julienne wrote:
> + * For more information on the firmware interface check:
> + * https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface
> + */
> +struct bcm2835_firmware_prop {
> + u32 id;
> + u32 val;
> + u32 disable_turbo;
> +} __packed;
Hi,
technically we are not in arch and those fields have a defined
endianness.
Regards
Oliver
Re: [RFC PATCH] usb: host: xhci: allow __GFP_FS in dma allocation
On So, 2019-05-19 at 22:56 -0700, Christoph Hellwig wrote:
> Folks, you can't just pass arbitary GFP_ flags to dma allocation
> routines, beause very often they are not just wrappers around
> the page allocator.
>
> So no, you can't just fine grained control the flags, but the
> existing code is just as buggy.
>
> Please switch to use memalloc_noio_save() instead.
>
Hi,
we actually do. It is just higher up in the calling path:
int usb_reset_device(struct usb_device *udev)
{
int ret;
int i;
unsigned int noio_flag;
struct usb_port *port_dev;
struct usb_host_config *config = udev->actconfig;
struct usb_hub *hub = usb_hub_to_struct_hub(udev->parent);
if (udev->state == USB_STATE_NOTATTACHED ||
udev->state == USB_STATE_SUSPENDED) {
dev_dbg(>dev, "device reset not allowed in state %d\n",
udev->state);
return -EINVAL;
}
if (!udev->parent) {
/* this requires hcd-specific logic; see ohci_restart() */
dev_dbg(>dev, "%s for root hub!\n", __func__);
return -EISDIR;
}
port_dev = hub->ports[udev->portnum - 1];
/*
* Don't allocate memory with GFP_KERNEL in current
* context to avoid possible deadlock if usb mass
* storage interface or usbnet interface(iSCSI case)
* is included in current configuration. The easist
* approach is to do it for every device reset,
* because the device 'memalloc_noio' flag may have
* not been set before reseting the usb device.
*/
noio_flag = memalloc_noio_save();
So, do we need to audit the mem_flags again?
What are we supposed to use? GFP_KERNEL?
Regards
Oliver
Re: [PATCH] usb/hcd: Send a uevent signaling that the host controller has died
On Mi, 2019-04-10 at 14:35 -0600, Raul E Rangel wrote: > This change will send a CHANGE event to udev with the DEAD environment > variable set when the HC dies. I chose this instead of any of the other > udev events because it's representing a state change in the host > controller. The only other event that might have fit was OFFLINE, but > that seems to be used for hot-removal. > > By notifying user space the appropriate policies can be applied. > e.g., > * Collect error logs. > * Notify the user that USB is no longer functional. > * Perform a graceful reboot. Could you please make sure this type of event is shared with other subsystems whose devices can "die"? It looks to me like SCSI offline should for example create the same event. This kind of thing needs to be documented. Regards Oliver
Re: [PATCH v3] HID: core: move Usage Page concatenation to Main item
On Di, 2019-03-26 at 21:03 +0100, Nicolas Saenz Julienne wrote: > --- a/drivers/hid/hid-core.c > +++ b/drivers/hid/hid-core.c > @@ -218,13 +218,14 @@ static unsigned hid_lookup_collection(struct hid_parser > *parser, unsigned type) > * Add a usage to the temporary parser table. > */ > > -static int hid_add_usage(struct hid_parser *parser, unsigned usage) > +static int hid_add_usage(struct hid_parser *parser, unsigned usage, __u8 > size) No need to use the __u8 style inside the kernel. u8 will do. Regards Oliver
Re: [PATCH] usb: uas: fix usb subsystem hang after power off hub port
On Fr, 2019-03-08 at 09:13 +, kento.a.kobaya...@sony.com wrote: > The usb_reset_and_verify_device included in usb_reset_device fails > with -ENODEV after power off hub port, and the -ENODEV error will > be reported to uas_eh_bus_reset_handler and upper layer, so it > doesn't need to do rebind if -ENODEV happens. Hi, no I am sorry, that is an assumption you just cannot make. Anything can trigger a reset. That being SCSI is the common case certainly, but not the only case. And in those cases we cannot depend on upper layers doing the right thing, if we just ignore an error. NACK Sorry Oliver
Re: [PATCH v2] USB: serial: cp210x: Fix GPIO in autosuspend
On So, 2019-02-17 at 18:59 +0100, Karoly Pados wrote: > Current GPIO code in cp210x fails to take USB autosuspend into account, > making it practically impossible to use GPIOs with autosuspend enabled > without user configuration. Fix this like for ftdi_sio in a previous patch. > Tested on a CP2102N. Hi, your patch is looking good to me, but I am afraid there are issues. How do the GPIO lines on the device interact with USB reset and system suspend? Regards Oliver
Re: [PATCH 2/2] RTL8153-BD is used in Dell DA300 type-C dongle. It should be added to the whitelist of devices to activate MAC address pass through.
On Mo, 2019-02-18 at 11:48 +0800, David Chen wrote:
> From: David Chen
>
> Per confirming with Realtek all devices containing RTL8153-BD should
> activate MAC pass through and there won't use pass through bit on efuse
> like in RTL8153-AD.
>
> Signed-off-by: David Chen
> ---
> drivers/net/usb/r8152.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index ada6baf8847a..86c8c64fbb0f 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -1179,7 +1179,7 @@ static int vendor_mac_passthru_addr_read(struct r8152
> *tp, struct sockaddr *sa)
> } else {
> /* test for RTL8153-BND and RTL8153-BD */
> ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_MISC_1);
> - if ((ocp_data & BND_MASK) == 0 && (ocp_data & BD_MASK)) {
> + if ((ocp_data & BND_MASK) == 0 && (ocp_data & BD_MASK) == 0) {
You are inverting the second half of the test. How can this possibly be
right? Had you dropped it, I would understand. But this? Are you sure?
Regards
Oliver
Re: [PATCH] drivers/usb/storage/sddr55.c: Remove duplicate header
On Fr, 2019-01-11 at 10:10 +0100, Greg KH wrote:
> On Thu, Jan 10, 2019 at 01:10:25PM +0530, Sabyasachi Gupta wrote:
> > Remove unusual_sddr55.h which is included more than once
> >
> > Signed-off-by: Sabyasachi Gupta
> > ---
> > drivers/usb/storage/sddr55.c | 1 -
> > 1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/usb/storage/sddr55.c b/drivers/usb/storage/sddr55.c
> > index b8527c5..96bf7ee 100644
> > --- a/drivers/usb/storage/sddr55.c
> > +++ b/drivers/usb/storage/sddr55.c
> > @@ -62,7 +62,6 @@ MODULE_DEVICE_TABLE(usb, sddr55_usb_ids);
> > }
> >
> > static struct us_unusual_dev sddr55_unusual_dev_list[] = {
> > -# include "unusual_sddr55.h"
> > { } /* Terminating entry */
> > };
>
> As Oliver said on the other patch, this breaks the code. Please do not
> blindly make changes without understanding what the code does.
Maybe this should be explained. The file that is included does have
a name ending in .h. But it is not simply declarations, for which
newer trendy languages would use a a statement like ¨import".
"unusual_sddr55.h" is a list of devices. This list is used in
multiple places. And it needs to be identical in all those places.
Hence it is included. These are true includes. There is nothing wrong
with that code.
Our complaint is not that your fix is wrong, but there is nothing wrong
with the code as is. Including this list is the good thing to do.
Your wish to clean up the kernel is appreciated. But please find a
place that actually needs to be cleaned up.
Regards
Oliver
Re: [PATCH] drivers/usb/storage/jumpshot.c: Remove duplicate header
On Do, 2019-01-10 at 13:03 +0530, Sabyasachi Gupta wrote:
> Remove unusual_jumpshot.h which is included more than once.
>
> Signed-off-by: Sabyasachi Gupta
> ---
> drivers/usb/storage/jumpshot.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/usb/storage/jumpshot.c b/drivers/usb/storage/jumpshot.c
> index 917f170..06e46c6 100644
> --- a/drivers/usb/storage/jumpshot.c
> +++ b/drivers/usb/storage/jumpshot.c
> @@ -84,7 +84,6 @@ MODULE_DEVICE_TABLE(usb, jumpshot_usb_ids);
> }
>
> static struct us_unusual_dev jumpshot_unusual_dev_list[] = {
> -#include "unusual_jumpshot.h"
> { } /* Terminating entry */
> };
>
NACK.
It is included twice because the same device needs to be specified
and is quirky. You are breaking the driver.
Regards
Oliver
Re: [PATCH v3] usb: hub: add retry routine after intr URB submit error
On Mo, 2019-01-07 at 13:59 -0500, Alan Stern wrote: > On Mon, 7 Jan 2019, Greg KH wrote: > > > > What ever happened to this patch? Is it still needed? Oliver and Alan, > > any objections about it anymore? > > I have just one very minor nit to pick (see below). Mostly I've been > waiting to hear from Oliver. Sorry, I have been away over Christmas. It is looking good to me. Regards Oliver
Re: [PATCH RFC 4/5] usb: cdc-acm: Enable serdev support
On Fr, 2019-01-04 at 12:21 +0100, Andreas Färber wrote: > Switch from tty_port_register_device() to tty_port_register_device_serdev() > and from tty_unregister_device() to tty_port_unregister_device(). > > On removal of a serdev driver sometimes count mismatch warnings were seen: > > ttyACM ttyACM0: tty_hangup: tty->count(1) != (#fd's(0) + #kopen's(0)) > ttyACM ttyACM0: tty_port_close_start: tty->count = 1 port count = 0 > > Note: The serdev drivers appear to probe asynchronously as soon as they > are registered. Should the USB quirks in probe be moved before registration? > No noticeable difference for the devices at hand. That is quite drastic a change. Johan, how complete in terms of features is serdev? Are you refering to CLEAR_HALT_CONDITIONS in terms of quirks? Regards Oliver
