Re: Etron EJ168A controller
Am Mittwoch, den 09.10.2019, 11:57 -0700 schrieb Kenneth:
> Hi,
>
> I was informed that there were bug fixes for the Etron EJ168A controller in
> kernel 5.2 and 5.3
>
> While I can read most USB sticks, if I connect an android phone to this port,
> applications hang trying to access the phone.
Hi,
you will need to provide log files. A simple report of this kind
cannot be acted upon as no developer can replicate it.
Regards
Oliver
Re: Possible bug with cdc_ether, triggers NETDEV WATCHDOG
Am Mittwoch, den 09.10.2019, 09:27 -0400 schrieb Adam Bennett: > On 10/9/19 4:53 AM, Bjørn Mork wrote: > > This warning means that the gadget doesn't accept the packets we send > > it. There isn't much the host can do about that, except dropping > > packets on the floor. Which is why the warning is this loud. > > > > Would a firewall on either the linux host or the Pi Zero cause the same > problem (and message)? If a firewall discards a packet, it will not reach the transmit queue. Hence the answer to that question is negative. If other OSes get this device to work, we would need to know what they are doing differently. Can you get a trace from them and compare it to ours? Regards Oliver
Re: [PATCH] USB: microtek: fix info-leak at probe
Am Donnerstag, den 03.10.2019, 09:09 +0200 schrieb Johan Hovold:
> Add missing bulk-in endpoint sanity check to prevent uninitialised stack
> data from being reported to the system log and used as endpoint
> addresses.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable
> Reported-by: syzbot+5630ca7c3b2be5c9d...@syzkaller.appspotmail.com
> Signed-off-by: Johan Hovold
Acked-by: Oliver Neukum
Re: [RFC PATCH 1/1] USB: myriad-ma24xx-vsc: Firmware loader driver for USB Myriad ma24xx
api-2450.mvcmd"
> +#define MA2480_FIRMWARE FW_DIR "mvncapi-2480.mvcmd"
> +
> +MODULE_FIRMWARE(MA2450_FIRMWARE);
> +MODULE_FIRMWARE(MA2480_FIRMWARE);
> +
> +enum {
> + MA2450FW = 0,
> + MA2480FW
> +};
> +
> +/* macros for struct usb_device_id */
> +#define MYRIAD_CHIP_VERSION(x) \
> + ((x)->driver_info & 0xf)
> +
> +#define MYRIAD_VID 0x03e7 /* Movidius Ltd, an Intel company */
> +#define MA2450_PID 0x2150 /* ma2450 VSC loopback device, without fw */
> +#define MA2485_PID 0x2485 /* ma2485 VSC loopback device, without fw */
> +
> +#define MYRIAD_BUF_LEN 512 /* max size of USB_SPEED_HIGH packet */
> +#define WRITE_TIMEOUT 2000 /* milliseconds */
> +
> +static const struct usb_device_id id_table[] = {
> + { USB_DEVICE(MYRIAD_VID, MA2450_PID), .driver_info = MA2450FW },
> + { USB_DEVICE(MYRIAD_VID, MA2485_PID), .driver_info = MA2480FW },
> + { },
> +};
> +MODULE_DEVICE_TABLE(usb, id_table);
> +
> +static int myriad_ma24xx_vsc_probe(struct usb_interface *intf,
> + const struct usb_device_id *id)
> +{
> + struct usb_device *dev = interface_to_usbdev(intf);
> + struct usb_host_interface *altsetting = intf->cur_altsetting;
> + struct usb_endpoint_descriptor *epd;
> + int out_ep, res, size;
> + const struct firmware *firmware = NULL;
> + const u8 *ptr;
> + int offset, ret = 0;
> + char *buf;
> + char *fw_family_name;
> +
> + usb_dbg(intf, "probe %s-%s", dev->product, dev->serial);
> +
> + /* Find the first bulk OUT endpoint and its packet size */
> + res = usb_find_bulk_out_endpoint(altsetting, &epd);
> + if (res) {
> + usb_err(intf, "no OUT endpoint found");
> + return res;
> + }
> +
> + out_ep = usb_endpoint_num(epd);
> + size = usb_endpoint_maxp(epd);
> +
> + /* Validate endpoint and size */
> + if (size <= 0) {
> + usb_err(intf, "invalid size (%d)", size);
> + return -ENODEV;
> + }
> +
> + if (size > MYRIAD_BUF_LEN) {
> + usb_dbg(intf, "size reduced %d -> %d\n", size, MYRIAD_BUF_LEN);
> + size = MYRIAD_BUF_LEN;
> + }
> +
> + buf = kmalloc(size, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + switch (MYRIAD_CHIP_VERSION(id)) {
> + case MA2450FW:
> + fw_family_name = MA2450_FIRMWARE;
> + break;
> + case MA2480FW:
> + fw_family_name = MA2480_FIRMWARE;
> + break;
> + default:
> + usb_err(intf, "unsupported myriad ma24xx firmware family\n");
> + return -ENODEV;
Memory leak
Regards
Oliver
Re: ttyACM and BREAK chars ?
Am Donnerstag, den 12.09.2019, 07:09 + schrieb Joakim Tjernlund: > On Wed, 2019-09-11 at 20:27 +0200, Oliver Neukum wrote: > > Am Mittwoch, den 11.09.2019, 14:34 + schrieb Joakim Tjernlund: > > > On Wed, 2019-09-11 at 16:22 +0200, Oliver Neukum wrote: > > > > CAUTION: This email originated from outside of the organization. Do not > > > > click links or open attachments unless you recognize the sender and > > > > know the content is safe. > > > > > > > > > > > > Am Mittwoch, den 11.09.2019, 12:39 + schrieb Joakim Tjernlund: > > > > > Every now and then my ttyACM0 hangs up or sends a BREAK char to my > > > > > device. > > > > > I am trying to make ttyACM ignore incoming(over USB) and not emit > > > > > any BREAK automatically using termios (IGN_BRK) but that does not > > > > > make a difference. > > > > > > > > > > Is BREAK handling unimpl. in ttyACM ? > > > > > > > > acm_send_break() implements it. > > > > > > Yes, I se that funktion but I don't see how one can ignore received BREAKs > > > If I set IGN_BRK on /dev/ttyACM0 I expect that every BREAK should just be > > > ignored > > > > Handling breaks looks a bit broken on CDC-ACM. > > Could you test the attached patch? > > > > Sure, I can test it but from looking at the patch it seems like ACM already > ignores > BREAKs(hardcoded) and with your patch you actually start reporting them. Well, what is not reported cannot really be ignored. AFAICT n_tty_receive_break() should solve the issue generically. > My problem is sudden disconnects I cannot explain but I think they are > connect to BREAKs > I have seen these errors in dmesg though, not sure if they help the diagnose: > [181780.167987] usb usb1-port6: disabled by hub (EMI?), re-enabling... The relevant fault happens likely just before that. > [181780.168208] cdc_acm 1-6.3:1.1: acm_ctrl_irq - usb_submit_urb failed: -19 > [181780.167996] usb 1-6: USB disconnect, device number 30 > [181780.176548] usb 1-6-port2: attempt power cycle > [181781.772847] usb 1-6.3: USB disconnect, device number 32 > [181781.773134] cdc_acm 1-6.3:1.1: failed to set dtr/rts Either your cabling is indeed crap, or something crashes your device. Regards Oliver
Re: ttyACM and BREAK chars ?
Am Mittwoch, den 11.09.2019, 14:34 + schrieb Joakim Tjernlund:
> On Wed, 2019-09-11 at 16:22 +0200, Oliver Neukum wrote:
> > CAUTION: This email originated from outside of the organization. Do not
> > click links or open attachments unless you recognize the sender and know
> > the content is safe.
> >
> >
> > Am Mittwoch, den 11.09.2019, 12:39 + schrieb Joakim Tjernlund:
> > > Every now and then my ttyACM0 hangs up or sends a BREAK char to my device.
> > > I am trying to make ttyACM ignore incoming(over USB) and not emit
> > > any BREAK automatically using termios (IGN_BRK) but that does not make a
> > > difference.
> > >
> > > Is BREAK handling unimpl. in ttyACM ?
> >
> > acm_send_break() implements it.
>
> Yes, I se that funktion but I don't see how one can ignore received BREAKs
> If I set IGN_BRK on /dev/ttyACM0 I expect that every BREAK should just be
> ignored
Handling breaks looks a bit broken on CDC-ACM.
Could you test the attached patch?
Regards
Oliver
From 74a16a0fdc056659b0543ec377b51fa231a423c2 Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Wed, 11 Sep 2019 20:17:38 +0200
Subject: [PATCH] cdc-acm: fix BREAK rx code path adding necessary calls
Counting break events is nice but we should actually report them to
the tty layer.
Fixes: 5a6a62bdb9257 ("cdc-acm: add TIOCMIWAIT")
Signed-off-by: Oliver Neukum
---
drivers/usb/class/cdc-acm.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 62f4fb9b362f..89d97d9763b0 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -312,8 +312,10 @@ static void acm_process_notification(struct acm *acm, unsigned char *buf)
acm->iocount.dsr++;
if (difference & ACM_CTRL_DCD)
acm->iocount.dcd++;
- if (newctrl & ACM_CTRL_BRK)
+ if (newctrl & ACM_CTRL_BRK) {
acm->iocount.brk++;
+ tty_insert_flip_char(&acm->port, 0, TTY_BREAK);
+ }
if (newctrl & ACM_CTRL_RI)
acm->iocount.rng++;
if (newctrl & ACM_CTRL_FRAMING)
--
2.16.4
Re: ttyACM and BREAK chars ?
Am Mittwoch, den 11.09.2019, 12:39 + schrieb Joakim Tjernlund: > Every now and then my ttyACM0 hangs up or sends a BREAK char to my device. > I am trying to make ttyACM ignore incoming(over USB) and not emit > any BREAK automatically using termios (IGN_BRK) but that does not make a > difference. > > Is BREAK handling unimpl. in ttyACM ? acm_send_break() implements it. But the support is optional in ACM devices. What is bmCapabilities in sysfs for your device? Regards Oliver
Re: Lacie Rugged USB3-FW does not work with UAS
Am Mittwoch, den 04.09.2019, 19:10 +0200 schrieb Julian Sikorski:
>
>
> Moreover, does this matter that the two Read Capacity errors only appear
> after the device is disconnected?
Hi,
yes it does. However, it didn't in the first log I looked at.
Could you check whether the command the failure happens on
is constant? That is, test a few times and look for differences.
Regards
Oliver
Re: Lacie Rugged USB3-FW does not work with UAS
Am Donnerstag, den 29.08.2019, 20:33 +0200 schrieb Julian Sikorski: Hi, this is a relief. If necessary we can blacklist the new device. Howevera, as that costs performance, I would appriciate if you take first try out an alternative approach. > [ 362.230833] usb 2-4: New USB device found, idVendor=059f, > idProduct=1061, bcdDevice= 0.01 > [ 362.230837] usb 2-4: New USB device strings: Mfr=2, Product=3, > SerialNumber=1 > [ 362.230839] usb 2-4: Product: Rugged USB3-FW > [ 362.230841] usb 2-4: Manufacturer: LaCie > [ 362.230842] usb 2-4: SerialNumber: 157f928920fa > [ 362.270100] scsi host12: uas > [ 362.270720] scsi 12:0:0:0: Direct-Access LaCieRugged FW USB3 > 051E PQ: 0 ANSI: 6 > [ 362.271472] sd 12:0:0:0: Attached scsi generic sg1 type 0 > [ 362.280344] sd 12:0:0:0: [sdb] 1953525168 512-byte logical blocks: > (1.00 TB/932 GiB) > [ 362.280422] sd 12:0:0:0: [sdb] Write Protect is off > [ 362.280423] sd 12:0:0:0: [sdb] Mode Sense: 43 00 00 00 > [ 362.280544] sd 12:0:0:0: [sdb] Write cache: enabled, read cache: > enabled, doesn't support DPO or FUA This means that at least the earliest commandos did get through. > [ 392.672691] sd 12:0:0:0: tag#29 uas_eh_abort_handler 0 uas-tag 1 > inflight: IN > [ 392.672697] sd 12:0:0:0: tag#29 CDB: Report supported operation codes > a3 0c 01 12 00 00 00 00 02 00 00 00 > [ 392.678304] scsi host12: uas_eh_device_reset_handler start > [ 392.800099] usb 2-4: reset SuperSpeed Gen 1 USB device number 3 using > xhci_hcd > [ 392.848154] scsi host12: uas_eh_device_reset_handler success > [ 422.875443] scsi host12: uas_eh_device_reset_handler start > [ 422.875650] sd 12:0:0:0: tag#16 uas_zap_pending 0 uas-tag 1 inflight: > [ 422.875654] sd 12:0:0:0: tag#16 CDB: Report supported operation codes > a3 0c 01 12 00 00 00 00 02 00 00 00 > [ 422.997556] usb 2-4: reset SuperSpeed Gen 1 USB device number 3 using > xhci_hcd > [ 423.046525] scsi host12: uas_eh_device_reset_handler success > [ 431.853505] usb 2-4: USB disconnect, device number 3 > [ 431.903459] sd 12:0:0:0: [sdb] Optimal transfer size 33553920 bytes > [ 432.064456] sd 12:0:0:0: [sdb] Read Capacity(16) failed: Result: > hostbyte=DID_ERROR driverbyte=DRIVER_OK Read Capacity(16) failed > [ 432.064459] sd 12:0:0:0: [sdb] Sense not available. > [ 432.184595] sd 12:0:0:0: [sdb] Read Capacity(10) failed: Result: > hostbyte=DID_ERROR driverbyte=DRIVER_OK Read Capacity(10) failed There is a chance that this device can deal only with Read Capacity(10) and crashes on Read Capacity(16). One difference between Usb-storage and UAS is the order in which the 10 and 16 versions are tried. The attached patches introduce a quirk to reverse the order for this particular device under UAS. Could you try them? Regards Oliver From 883355951a23d3c4b3c14ca0540972739ae6ffb2 Mon Sep 17 00:00:00 2001 From: Oliver Neukum Date: Mon, 2 Sep 2019 13:28:39 +0200 Subject: [PATCH 1/2] uas: honor flag to avoid CAPACITY16 Copy the support over from usb-storage Signed-off-by: Oliver Neukum --- drivers/usb/storage/uas.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c index 68b1cb0f84e5..a8bd5ff5a4b9 100644 --- a/drivers/usb/storage/uas.c +++ b/drivers/usb/storage/uas.c @@ -854,6 +854,10 @@ static int uas_slave_configure(struct scsi_device *sdev) sdev->wce_default_on = 1; } + /* Some devices cannot handle READ_CAPACITY_16 */ + if (devinfo->flags & US_FL_NO_READ_CAPACITY_16) + sdev->no_read_capacity_16 = 1; + /* * Some disks return the total number of blocks in response * to READ CAPACITY rather than the highest block number. -- 2.16.4 From 115389ff678cae7cb636ac7e520f06e5182cd353 Mon Sep 17 00:00:00 2001 From: Oliver Neukum Date: Mon, 2 Sep 2019 13:30:00 +0200 Subject: [PATCH 2/2] uas: quirk for LaCie Rugged USB 3 No. CAPACITY16 for these devices. Signed-off-by: Oliver Neukum --- drivers/usb/storage/unusual_devs.h | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/usb/storage/unusual_devs.h b/drivers/usb/storage/unusual_devs.h index ea0d27a94afe..643bba41291e 100644 --- a/drivers/usb/storage/unusual_devs.h +++ b/drivers/usb/storage/unusual_devs.h @@ -806,6 +806,12 @@ UNUSUAL_DEV( 0x059f, 0x0651, 0x, 0x, USB_SC_DEVICE, USB_PR_DEVICE, NULL, US_FL_NO_WP_DETECT ), +UNUSUAL_DEV( 0x059f, 0x103e, 0x0002, 0x0002, + "LaCie", + "Rugged USB 3", + USB_SC_DEVICE, USB_PR_DEVICE, NULL, + US_FL_NO_READ_CAPACITY_16 ), + /* * Submitted by Joel Bourquard * Some versions of this device need the SubClass and Protocol overrides -- 2.16.4
Re: [PATCH 3/3] net: cdc_ncm: Add ACPI MAC address pass through functionality
Am Freitag, den 30.08.2019, 19:38 + schrieb
charles.h...@dellteam.com:
> This change adds support to cdc_ncm for ACPI MAC address pass through
> functionality that also exists in the Realtek r8152 driver. This is in
> support of Dell's Universal Dock D6000, to give it the same feature
> capability as is currently available in Windows and advertized on Dell's
> product web site.
>
> Signed-off-by: Charles Hyde
> Cc: Mario Limonciello
> Cc: Oliver Neukum
> Cc: Rafael J. Wysocki
> Cc: Len Brown
> Cc: linux-usb@vger.kernel.org
> Cc: linux-a...@vger.kernel.org
> ---
> drivers/net/usb/cdc_ncm.c | 67 +--
> 1 file changed, 64 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> index 85093579612f..11a04dc2298d 100644
> --- a/drivers/net/usb/cdc_ncm.c
> +++ b/drivers/net/usb/cdc_ncm.c
> @@ -52,6 +52,7 @@
> #include
> #include
> #include
> +#include
>
> #if IS_ENABLED(CONFIG_USB_NET_CDC_MBIM)
> static bool prefer_mbim = true;
> @@ -984,11 +985,30 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct
> usb_interface *intf, u8 data_
> usb_set_intfdata(ctx->control, dev);
>
> if (ctx->ether_desc) {
> - temp = usbnet_get_ethernet_addr(dev,
> ctx->ether_desc->iMACAddress);
> + struct sockaddr sa;
> +
> + temp = cdc_ncm_get_ethernet_address(dev, ctx);
> if (temp) {
> dev_dbg(&intf->dev, "failed to get mac address\n");
> goto error2;
> }
> +
> + /* Check for a Dell Universal Dock D6000 before checking if
> + * ACPI supports MAC address pass through.
> + */
> + if (!strstr(dev->udev->product, "D6000"))
> + goto skip_acpi_mapt_in_bind;
> +
> + if (get_acpi_mac_passthru(sa.sa_data) != 0)
> + goto skip_acpi_mapt_in_bind;
> +
> + if (memcmp(dev->net->dev_addr, sa.sa_data, ETH_ALEN) == 0)
> + goto skip_acpi_mapt_in_bind;
> +
> + if (cdc_ncm_set_ethernet_address(dev, &sa) == 0)
> + memcpy(dev->net->dev_addr, sa.sa_data, ETH_ALEN);
> +
> +skip_acpi_mapt_in_bind:
> dev_info(&intf->dev, "MAC-Address: %pM\n", dev->net->dev_addr);
> }
>
> @@ -1716,6 +1736,47 @@ static void cdc_ncm_status(struct usbnet *dev, struct
> urb *urb)
> }
> }
>
> +static int cdc_ncm_resume(struct usb_interface *intf)
> +{
> + struct usbnet *dev = usb_get_intfdata(intf);
> + struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
> + int ret;
> +
> + ret = usbnet_resume(intf);
> + if (ret != 0)
> + goto error2;
> +
> + if (ctx->ether_desc) {
> + struct sockaddr sa;
> +
> + if (cdc_ncm_get_ethernet_address(dev, ctx)) {
> + dev_dbg(&intf->dev, "failed to get mac address\n");
> + goto error2;
> + }
> +
> + /* Check for a Dell Universal Dock D6000 before checking if
> + * ACPI supports MAC address pass through.
> + */
> + if (!strstr(dev->udev->product, "D6000"))
> + goto skip_acpi_mapt_in_resume;
This is too ugly. Use a flag for the need to restore the MAC.
And have you consider the case somebody triggers a reset through
usbfs? It looks to me like you should encapsulate the restoration
of the MAC and also use it in post_reset()
Regards
Oliver
Re: [PATCH 1/3] net: cdc_ncm: add get/set ethernet address functions
Am Freitag, den 30.08.2019, 19:37 + schrieb
charles.h...@dellteam.com:
> This patch adds support for pushing a MAC address out to USB based
> ethernet controllers driven by cdc_ncm. With this change, ifconfig can
> now set the device's MAC address. For example, the Dell Universal Dock
> D6000 is driven by cdc_ncm. The D6000 can now have its MAC address set
> by ifconfig, as it can be done in Windows. This was tested with a D6000
> using ifconfig on an x86 based chromebook, where iproute2 is not
> available.
> +/* Provide method to push MAC address to the USB device's ethernet
> controller.
> + */
> +int cdc_ncm_set_mac_addr(struct net_device *net, void *p)
> +{
> + struct usbnet *dev = netdev_priv(net);
> +
> + /* Try to push the MAC address out to the device. Ignore any errors,
> + * to be compatible with prior versions of this source.
> + */
> + cdc_ncm_set_ethernet_address(dev, (struct sockaddr *)p);
You are throwing away error reports.
Regards
Oliver
[PATCH] USB: cdc-wdm: fix race between write and disconnect due to flag abuse
In case of a disconnect an ongoing flush() has to be made fail.
Nevertheless we cannot be sure that any pending URB has already
finished, so although they will never succeed, they still must
not be touched.
The clean solution for this is to check for WDM_IN_USE
and WDM_DISCONNECTED in flush(). There is no point in ever
clearing WDM_IN_USE, as no further writes make sense.
The issue is as old as the driver.
Fixes: afba937e540c9 ("USB: CDC WDM driver")
Reported-by: syzbot+d232cca6ec42c2edb...@syzkaller.appspotmail.com
Signed-off-by: Oliver Neukum
---
drivers/usb/class/cdc-wdm.c | 16
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 1656f5155ab8..f9f7c8a5e091 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -588,10 +588,20 @@ static int wdm_flush(struct file *file, fl_owner_t id)
{
struct wdm_device *desc = file->private_data;
- wait_event(desc->wait, !test_bit(WDM_IN_USE, &desc->flags));
+ wait_event(desc->wait,
+ /*
+* needs both flags. We cannot do with one
+* because resetting it would cause a race
+* with write() yet we need to signal
+* a disconnect
+*/
+ !test_bit(WDM_IN_USE, &desc->flags) ||
+ test_bit(WDM_DISCONNECTING, &desc->flags));
/* cannot dereference desc->intf if WDM_DISCONNECTING */
- if (desc->werr < 0 && !test_bit(WDM_DISCONNECTING, &desc->flags))
+ if (test_bit(WDM_DISCONNECTING, &desc->flags))
+ return -ENODEV;
+ if (desc->werr < 0)
dev_err(&desc->intf->dev, "Error in flush path: %d\n",
desc->werr);
@@ -975,8 +985,6 @@ static void wdm_disconnect(struct usb_interface *intf)
spin_lock_irqsave(&desc->iuspin, flags);
set_bit(WDM_DISCONNECTING, &desc->flags);
set_bit(WDM_READ, &desc->flags);
- /* to terminate pending flushes */
- clear_bit(WDM_IN_USE, &desc->flags);
spin_unlock_irqrestore(&desc->iuspin, flags);
wake_up_all(&desc->wait);
mutex_lock(&desc->rlock);
--
2.16.4
Re: [RFC 1/3] net: cdc_ncm: add get/set ethernet address functions
Am Freitag, den 23.08.2019, 22:26 + schrieb
charles.h...@dellteam.com:
> This patch adds support for pushing a MAC address out to USB based
> ethernet controllers driven by cdc_ncm. With this change, ifconfig can
> now set the device's MAC address. For example, the Dell Universal Dock
> D6000 is driven by cdc_ncm. The D6000 can now have its MAC address set
> by ifconfig, as it can be done in Windows. This was tested with a D6000
> using ifconfig on an x86 based chromebook, where iproute2 is not
> available.
The code is good. But placing it into cdc_ncm means that cdc_ether
or other drivers cannot use it.
Regards
Oliver
Re: Lacie Rugged USB3-FW does not work with UAS
Am Freitag, den 23.08.2019, 16:21 +0200 schrieb Julian Sikorski: > > I did some further digging regarding whether this is a regression: the > quirk file on the laptop is from 15 July 2014. The machine is from ca. > May 2011. Looking through my earlier posts to linux-usb it appears that > the addition of the quirk is related to this thread: > > https://marc.info/?l=linux-usb&m=140537519907935&w=2 > > At the same time, back in 2011, I reported that the drive was working > after some fixes: > > https://marc.info/?l=linux-usb&m=132276407611433&w=2 Hi, this is alarming. Was this physically the same drive? I am asking because we have seen cases where two different devices were sold under the same name. Regards Oliver
Re: Lacie Rugged USB3-FW does not work with UAS
Am Freitag, den 23.08.2019, 15:31 +0200 schrieb Julian Sikorski:
> it appears that lacie rugged usb3-fw is not compatible with UAS.
> I have just connected my few years old Lacie Rugged USB3-FW to my new
> desktop PC to see if the backups I have been creating on the laptop can
> actually be restored.
Hi,
does that mean that we have a regression? How did you create those
backups?
Regards
Oliver
Re: WARNING in r871xu_dev_remove
Am Donnerstag, den 22.08.2019, 07:28 -0700 schrieb syzbot:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit:eea39f24 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=163ae01260
> kernel config: https://syzkaller.appspot.com/x/.config?x=d0c62209eedfd54e
> dashboard link: https://syzkaller.appspot.com/bug?extid=80899a8a8efe8968cde7
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1739cb0e60
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=154fcc2e60
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+80899a8a8efe8968c...@syzkaller.appspotmail.com
#syz test: https://github.com/google/kasan.git eea39f24
>From 4f21b5aabc448719aa612b9359d90a178cb485d8 Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Thu, 22 Aug 2019 16:37:33 +0200
Subject: [PATCH] rtl8712: fix race between firmware failing to load and
disconnect
We have to wait for the attempt to load the firmware to finish
before we evaluate the result.
Reported-by: syzbot+80899a8a8efe8968c...@syzkaller.appspotmail.com
Signed-off-by: Oliver Neukum
---
drivers/staging/rtl8712/hal_init.c | 3 ++-
drivers/staging/rtl8712/usb_intf.c | 8 ++--
2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/rtl8712/hal_init.c
b/drivers/staging/rtl8712/hal_init.c
index 40145c0338e4..42c0a3c947f1 100644
--- a/drivers/staging/rtl8712/hal_init.c
+++ b/drivers/staging/rtl8712/hal_init.c
@@ -33,7 +33,6 @@ static void rtl871x_load_fw_cb(const struct firmware
*firmware, void *context)
{
struct _adapter *adapter = context;
- complete(&adapter->rtl8712_fw_ready);
if (!firmware) {
struct usb_device *udev = adapter->dvobjpriv.pusbdev;
struct usb_interface *usb_intf = adapter->pusb_intf;
@@ -41,11 +40,13 @@ static void rtl871x_load_fw_cb(const struct firmware
*firmware, void *context)
dev_err(&udev->dev, "r8712u: Firmware request failed\n");
usb_put_dev(udev);
usb_set_intfdata(usb_intf, NULL);
+ complete(&adapter->rtl8712_fw_ready);
return;
}
adapter->fw = firmware;
/* firmware available - start netdev */
register_netdev(adapter->pnetdev);
+ complete(&adapter->rtl8712_fw_ready);
}
static const char firmware_file[] = "rtlwifi/rtl8712u.bin";
diff --git a/drivers/staging/rtl8712/usb_intf.c
b/drivers/staging/rtl8712/usb_intf.c
index d0daae0b8299..8d7b57073592 100644
--- a/drivers/staging/rtl8712/usb_intf.c
+++ b/drivers/staging/rtl8712/usb_intf.c
@@ -595,10 +595,13 @@ static void r871xu_dev_remove(struct usb_interface
*pusb_intf)
if (pnetdev) {
struct _adapter *padapter = netdev_priv(pnetdev);
- usb_set_intfdata(pusb_intf, NULL);
- release_firmware(padapter->fw);
/* never exit with a firmware callback pending */
wait_for_completion(&padapter->rtl8712_fw_ready);
+ pnetdev = usb_get_intfdata(pusb_intf);
+ usb_set_intfdata(pusb_intf, NULL);
+ if (!pnetdev)
+ goto raced_with_firmware_failure;
+ release_firmware(padapter->fw);
if (drvpriv.drv_registered)
padapter->surprise_removed = true;
unregister_netdev(pnetdev); /* will call netdev_close() */
@@ -609,6 +612,7 @@ static void r871xu_dev_remove(struct usb_interface
*pusb_intf)
r871x_dev_unload(padapter);
r8712_free_drv_sw(padapter);
+raced_with_firmware_failure:
/* decrease the reference count of the usb device structure
* when disconnect
*/
--
2.16.4
Re: WARNING in rollback_registered_many (2)
Am Mittwoch, den 07.08.2019, 16:03 +0200 schrieb Andrey Konovalov:
I may offer a preliminary analysis.
Regards
Oliver
> On Fri, Apr 12, 2019 at 1:32 PM Andrey Konovalov
> wrote:
> >
> > On Fri, Apr 12, 2019 at 1:29 AM syzbot
> > wrote:
> > >
> > > syzbot has found a reproducer for the following crash on:
> > >
> > > HEAD commit:9a33b369 usb-fuzzer: main usb gadget fuzzer driver
> > > git tree: https://github.com/google/kasan/tree/usb-fuzzer
> > > console output: https://syzkaller.appspot.com/x/log.txt?x=10d552b720
> > > kernel config: https://syzkaller.appspot.com/x/.config?x=23e37f59d94ddd15
> > > dashboard link:
> > > https://syzkaller.appspot.com/bug?extid=40918e4d826fb2ff9b96
> > > compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=17a4c1af20
> > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=121b274b20
> > >
> > > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > > Reported-by: syzbot+40918e4d826fb2ff9...@syzkaller.appspotmail.com
> > >
> > > usb 1-1: r8712u: MAC Address from efuse = 00:e0:4c:87:00:00
> > > usb 1-1: r8712u: Loading firmware from "rtlwifi/rtl8712u.bin"
> > > usb 1-1: USB disconnect, device number 2
Disconnect will run which leads to
static void r871xu_dev_remove(struct usb_interface *pusb_intf)
{
struct net_device *pnetdev = usb_get_intfdata(pusb_intf);
struct usb_device *udev = interface_to_usbdev(pusb_intf);
if (pnetdev) {
^^^ This is supposed to save us
struct _adapter *padapter = netdev_priv(pnetdev);
usb_set_intfdata(pusb_intf, NULL);
release_firmware(padapter->fw);
/* never exit with a firmware callback pending */
wait_for_completion(&padapter->rtl8712_fw_ready);
if (drvpriv.drv_registered)
padapter->surprise_removed = true;
unregister_netdev(pnetdev); /* will call netdev_close() */
So we will call unregister_netdev()
> > > usb 1-1: Direct firmware load for rtlwifi/rtl8712u.bin failed with error
> > > -2
> > > usb 1-1: r8712u: Firmware request failed
So we ran into the error handling of:
static void rtl871x_load_fw_cb(const struct firmware *firmware, void *context)
{
struct _adapter *adapter = context;
complete(&adapter->rtl8712_fw_ready);
if (!firmware) {
struct usb_device *udev = adapter->dvobjpriv.pusbdev;
struct usb_interface *usb_intf = adapter->pusb_intf;
dev_err(&udev->dev, "r8712u: Firmware request failed\n");
usb_put_dev(udev);
usb_set_intfdata(usb_intf, NULL);
^^^ This is supposed to save us from deregistering an unregistered device
but it comes too late. We have already called complete.
return;
}
adapter->fw = firmware;
/* firmware available - start netdev */
register_netdev(adapter->pnetdev);
register_netdev() is not called.
> > > Kernel panic - not syncing: panic_on_warn set ...
> > > CPU: 0 PID: 575 Comm: kworker/0:4 Not tainted 5.1.0-rc4-319354-g9a33b36 #3
> > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> > > Google 01/01/2011
> > > Workqueue: usb_hub_wq hub_event
> > > Call Trace:
> > > __dump_stack lib/dump_stack.c:77 [inline]
> > > dump_stack+0xe8/0x16e lib/dump_stack.c:113
> > > panic+0x29d/0x5f2 kernel/panic.c:214
> > > __warn.cold+0x20/0x48 kernel/panic.c:571
> > > report_bug+0x262/0x2a0 lib/bug.c:186
> > > fixup_bug arch/x86/kernel/traps.c:179 [inline]
> > > fixup_bug arch/x86/kernel/traps.c:174 [inline]
> > > do_error_trap+0x130/0x1f0 arch/x86/kernel/traps.c:272
> > > do_invalid_op+0x37/0x40 arch/x86/kernel/traps.c:291
> > > invalid_op+0x14/0x20 arch/x86/entry/entry_64.S:973
This kills us.
> > > RIP: 0010:rollback_registered_many+0x1f3/0xe70 net/core/dev.c:8152
> > > Code: 05 00 00 31 ff 44 89 fe e8 5a 15 f3 f4 45 84 ff 0f 85 49 ff ff ff e8
> > > 1c 14 f3 f4 0f 1f 44 00 00 e8 12 14 f3 f4 e8 0d 14 f3 f4 <0f> 0b 4c 89 e7
> > > e8 33 72 f2 f6 31 ff 41 89 c4 89 c6 e8 27 15 f3 f4
> > > RSP: 0018:88809d087698 EFLAGS: 00010293
> > > RAX: 88809d058000 RBX: 88809624 RCX: 8c7eb146
> > > RDX: RSI: 8c7eb163 RDI: 0001
> > > RBP: 88809d0877c8
Re: [PATCH] HID: quirks: Disable runtime suspend on Microsoft Corp. Basic Optical Mouse v2.0
Am Donnerstag, den 22.08.2019, 21:23 +0800 schrieb Kai-Heng Feng: > at 18:38, Oliver Neukum wrote: > > Well, sort of. The USB spec merely states how to enter and exit > > a suspended state and that device state must not be lost. > > It does not tell you what a suspended device must be able to do. > > But shouldn’t remote wakeup signaling wakes the device up and let it exit > suspend state? Yes. Have you tested using a button? If they indeed do not work, then the device lies about supporting remote wakeup. That would warrant a quirk, but for remote wakeup. > Or it’s okay to let the device be suspended when remote wakeup is needed > but broken? Again, the HID spec does not specify what should trigger a remote wakeup. Limiting this to mouse buttons but not movements is inconvinient, but not buggy. This is indeed what Windows does. The device is suspended when the screen saver switches on. That we do not do that is a deficiency of X. To use runtime PM regularly you need an .ini file > > In other words, if on your system it is on, you need to look > > at udev, not the kernel. > > So if a device is broken when “power/control” is flipped by user, we should > deal it at userspace? That doesn’t sound right to me. If it is broken, as in crashing we could talk about it. If it merely does not do what you want, then, yes, that is for user space to deal with. > > Well, no. Runtime PM is a trade off. You lose something if you use > > it. If it worked just as well as full power, you would never use > > full power, would you? > > I am not asking the suspended state to work as full power, but to prevent a > device enters suspend state because of broken remote wakeup. What then would be the difference between suspended and active? A small delay in data transfer? > > Whether the loss of functionality or performance is worth the energy > > savings is a policy decision. Hence it belongs into udev. > > Ideally the kernel would tell user space what will work in a > > suspended state. Unfortunately HID does not provide support for that. > > I really don’t think “loss of functionally” belongs to policy decision. But > that’s just my opinion. That is just what we do if, for example, you choose between the configs of a USB device or when you use authorization. > Maybe just calling usb_autopm_put_interface() in usbhid_close() to balance > the refcount? No, the refcount is good. If remote wakeup is totally broken, you need to introduce a quirk that will prevent the kernel from believing the device when it claims to support it. Regards Oliver
Re: [PATCH] HID: quirks: Disable runtime suspend on Microsoft Corp. Basic Optical Mouse v2.0
Am Donnerstag, den 22.08.2019, 18:04 +0800 schrieb Kai-Heng Feng: > Hi Oliver, > > at 17:45, Oliver Neukum wrote: > > > 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. > > Can you please point out which spec it is? Is it USB 2.0 spec? Well, sort of. The USB spec merely states how to enter and exit a suspended state and that device state must not be lost. It does not tell you what a suspended device must be able to do. > > And it behaves like most hardware. > > So seems like most hardware are broken. > Maybe a more appropriate solution is to disable RPM for all USB mice. That is a decision a distro certainly can make. However, the kernel does not, by default, call usb_enable_autosuspend() for HID devices for this very reason. It is enabled by default only for hubs, BT dongles and UVC cameras (and some minor devices) In other words, if on your system it is on, you need to look at udev, not the kernel. > > If you do not want runtime PM for such devices, do not switch > > it on. > > A device should work regardless of runtime PM status. Well, no. Runtime PM is a trade off. You lose something if you use it. If it worked just as well as full power, you would never use full power, would you? Whether the loss of functionality or performance is worth the energy savings is a policy decision. Hence it belongs into udev. Ideally the kernel would tell user space what will work in a suspended state. Unfortunately HID does not provide support for that. This is a deficiency of user space. The kernel has an ioctl() to let user space tell it, whether a device is fully needed. X does not use them. > > The refcounting needs to be done correctly. > > Will do. Well, I am afraid your patch breaks it and if you do not break it, the patch is reduced to nothing. > > > > This patch does something that udev should do and in a > > questionable manner. > > IMO if the device doesn’t support runtime suspend, then it needs to be > disabled in kernel but not workaround in userspace. You switch it on from user space. Of course the kernel default must be safe, as you said. It already is. 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: [RFC 1/4] Add usb_get_address and usb_set_address support
Am Mittwoch, den 21.08.2019, 23:35 + schrieb charles.h...@dellteam.com: > > > > > This is a VERY cdc-net-specific function. It is not a "generic" USB > > function at all. Why does it belong in the USB core? Shouldn't it live > > in the code that handles the other cdc-net-specific logic? > > > > thanks, > > > > greg k-h > > > Thank you for this feedback, Greg. I was not sure about adding this to > message.c, because of the USB_CDC_GET_NET_ADDRESS. I had found references to > SET_ADDRESS in the USB protocol at > https://wiki.osdev.org/Universal_Serial_Bus#USB_Protocol. If one wanted a > generic USB function for SET_ADDRESS, to be used for both sending a MAC > address and receiving one, how would you suggest this be implemented? This > is a legit question because I am curious. Your implementation was, except for missing error handling, usable. The problem is where you put it. CDC messages exist only for CDC devices. Now it is true that there is no generic CDC driver. Creating a module just for that would cost more memory than it saves in most cases. But MACs are confined to network devices. Hence the functionality can be put into usbnet. It should not be put into any individual driver, so that every network driver can use it without duplication. > Your feedback led to moving the functionality into cdc_ncm.c for today's > testing, and removing all changes from messages.c, usb.h, usbnet.c, and > usbnet.h. This may be where I end up long term, but I would like to learn if > there is a possible solution that could live in message.c and be callable > from other USB-to-Ethernet aware drivers. All those drivers use usbnet. Hence there it should be. Regards Oliver
Re: [RFC 0/4] Add support into cdc_ncm for MAC address pass through
Am Dienstag, den 20.08.2019, 22:15 + schrieb
charles.h...@dellteam.com:
> In recent testing of a Dell Universal Dock D6000, I found that MAC address
> pass through is not supported in the Linux drivers.
Hi,
thank you for writing this code. It adds cool functionality.
There are, however, a few design issues and minor flaws with it.
I will comment on the patches in the series to point them out.
Could you correct them?
Regards
Oliver
Re: [RFC 2/4] Allow cdc_ncm to set MAC address in hardware
Am Dienstag, den 20.08.2019, 22:21 + schrieb
charles.h...@dellteam.com:
> This patch adds support for pushing a MAC address out to USB based
> ethernet controllers driven by cdc_ncm. With this change, ifconfig can
> now set the device's MAC address. For example, the Dell Universal Dock
> D6000 is driven by cdc_ncm. The D6000 can now have its MAC address set
> by ifconfig, as it can be done in Windows. This was tested with a D6000
> using ifconfig.
On a design note, it looks like you broke S4 with the driver.
Suspend To Disk will cut power and hence reset the MAC to default.
You need to reset it to the user's setting in reset_resume().
Please add that to usbnet.
>
> Signed-off-by: Charles Hyde
> Cc: Mario Limonciello
> Cc: Oliver Neukum
> Cc: net...@vger.kernel.org
> Cc: linux-usb@vger.kernel.org
> ---
> drivers/net/usb/cdc_ncm.c | 20 +++-
> drivers/net/usb/usbnet.c | 37 -
> include/linux/usb/usbnet.h | 1 +
> 3 files changed, 48 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> index 50c05d0f44cb..f77c8672f972 100644
> --- a/drivers/net/usb/cdc_ncm.c
> +++ b/drivers/net/usb/cdc_ncm.c
> @@ -750,6 +750,24 @@ int cdc_ncm_change_mtu(struct net_device *net, int
> new_mtu)
> }
> EXPORT_SYMBOL_GPL(cdc_ncm_change_mtu);
>
> +/* Provide method to push MAC address to the USB device's ethernet
> controller.
> + */
> +int cdc_ncm_set_mac_addr(struct net_device *net, void *p)
> +{
> + struct usbnet *dev = netdev_priv(net);
> + struct sockaddr *addr = p;
> +
> + memcpy(dev->net->dev_addr, addr->sa_data, ETH_ALEN);
> + /*
> + * Try to push the MAC address out to the device. Ignore any errors,
> + * to be compatible with prior versions of this source.
> + */
> + usbnet_set_ethernet_addr(dev);
> +
> + return eth_mac_addr(net, p);
> +}
> +EXPORT_SYMBOL_GPL(cdc_ncm_set_mac_addr);
> +
> static const struct net_device_ops cdc_ncm_netdev_ops = {
> .ndo_open= usbnet_open,
> .ndo_stop= usbnet_stop,
> @@ -757,7 +775,7 @@ static const struct net_device_ops cdc_ncm_netdev_ops = {
> .ndo_tx_timeout = usbnet_tx_timeout,
> .ndo_get_stats64 = usbnet_get_stats64,
> .ndo_change_mtu = cdc_ncm_change_mtu,
> - .ndo_set_mac_address = eth_mac_addr,
> + .ndo_set_mac_address = cdc_ncm_set_mac_addr,
Why can't this fully go into usbnet?
> .ndo_validate_addr = eth_validate_addr,
> };
>
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index 72514c46b478..72bdac34b0ee 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -149,20 +149,39 @@ int usbnet_get_ethernet_addr(struct usbnet *dev, int
> iMACAddress)
> int tmp = -1, ret;
> unsigned char buf [13];
>
> - ret = usb_string(dev->udev, iMACAddress, buf, sizeof buf);
> - if (ret == 12)
> - tmp = hex2bin(dev->net->dev_addr, buf, 6);
> - if (tmp < 0) {
> - dev_dbg(&dev->udev->dev,
> - "bad MAC string %d fetch, %d\n", iMACAddress, tmp);
> - if (ret >= 0)
> - ret = -EINVAL;
> - return ret;
> + ret = usb_get_address(dev->udev, buf);
> + if (ret == 6)
If you mean ETH_ALEN, you should use it.
> + memcpy(dev->net->dev_addr, buf, 6);
> + else if (ret < 0) {
> + ret = usb_string(dev->udev, iMACAddress, buf, sizeof buf);
> + if (ret == 12)
> + tmp = hex2bin(dev->net->dev_addr, buf, 6);
> + if (tmp < 0) {
> + dev_dbg(&dev->udev->dev,
> + "bad MAC string %d fetch, %d\n", iMACAddress,
> + tmp);
> + if (ret >= 0)
> + ret = -EINVAL;
> + return ret;
Again, you cannot ignore the possibility of getting fewer or more than
6 bytes.
> + }
> }
> return 0;
> }
> EXPORT_SYMBOL_GPL(usbnet_get_ethernet_addr);
>
> +int usbnet_set_ethernet_addr(struct usbnet *dev)
> +{
> + int ret;
> +
> + ret = usb_set_address(dev->udev, dev->net->dev_addr);
> + if (ret < 0) {
> + dev_dbg(&dev->udev->dev,
> + "bad MAC address put, %d\n", ret);
> + }
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(usbnet_set_ethernet_addr);
What is the purpose of this wrapper?
Regards
Oliver
Re: [RFC 1/4] Add usb_get_address and usb_set_address support
Am Dienstag, den 20.08.2019, 22:18 + schrieb
charles.h...@dellteam.com:
> The core USB driver message.c is missing get/set address functionality
This should go into usbnet. The CDC parser is where it is because
it is needed for serial and network devices. As serial devices
do not have a MAC, this can go into usbnet.
> that stops ifconfig from being able to push MAC addresses out to USB
> based ethernet devices. Without this functionality, some USB devices
> stop responding to ethernet packets when using ifconfig to change MAC
> addresses. This has been tested with a Dell Universal Dock D6000.
>
> Signed-off-by: Charles Hyde
> Cc: Mario Limonciello
> Cc: Greg Kroah-Hartman
> Cc: linux-usb@vger.kernel.org
> ---
> drivers/usb/core/message.c | 59 ++
> include/linux/usb.h| 3 ++
> 2 files changed, 62 insertions(+)
>
> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> index 5adf489428aa..eea775234b09 100644
> --- a/drivers/usb/core/message.c
> +++ b/drivers/usb/core/message.c
> @@ -1085,6 +1085,65 @@ int usb_clear_halt(struct usb_device *dev, int pipe)
> }
> EXPORT_SYMBOL_GPL(usb_clear_halt);
>
> +/**
> + * usb_get_address -
> + * @dev: device whose endpoint is halted
Which endpoint?
> + * @mac: buffer for containing
> + * Context: !in_interrupt ()
> + *
> + * This will attempt to get the six byte MAC address from a USB device's
> + * ethernet controller using GET_NET_ADDRESS command.
> + *
> + * This call is synchronous, and may not be used in an interrupt context.
> + *
> + * Return: Zero on success, or else the status code returned by the
Well, I am afraid it will return 6 on success.
> + * underlying usb_control_msg() call.
> + */
> +int usb_get_address(struct usb_device *dev, unsigned char * mac)
> +{
> + int ret = -ENOMEM;
Initialization is unnecessary here.
> + unsigned char *tbuf = kmalloc(256, GFP_NOIO);
If you intentionally picked a safety margin of 42 times, this
is cool. Otherwise it is a litttle much.
> +
> + if (!tbuf)
> + return -ENOMEM;
> +
> + ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
> + USB_CDC_GET_NET_ADDRESS,
> + USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> + 0, USB_REQ_SET_ADDRESS, tbuf, 256,
> + USB_CTRL_GET_TIMEOUT);
> + if (ret == 6)
> + memcpy(mac, tbuf, 6);
You cannot ignore the case of devices sending more or less than 6
bytes.
Regards
Oliver
Re: KASAN: use-after-free Read in iowarrior_disconnect
Am Dienstag, den 20.08.2019, 16:38 +0200 schrieb Oliver Neukum: > Am Dienstag, den 20.08.2019, 10:18 -0400 schrieb Alan Stern: > > On Mon, 19 Aug 2019, Oliver Neukum wrote: > > > > > Am Montag, den 19.08.2019, 07:48 -0700 schrieb syzbot: > > > > Hello, > > > > > > > > syzbot found the following crash on: > > > > > > > > HEAD commit:d0847550 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=139be30260 > > > > kernel config: > > > > https://syzkaller.appspot.com/x/.config?x=dbc9c80cc095da19 > > > > dashboard link: > > > > https://syzkaller.appspot.com/bug?extid=cfe6d93e0abab9a0de05 > > > > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > > > > syz repro: > > > > https://syzkaller.appspot.com/x/repro.syz?x=12fe6b0260 > > > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1548189c60 > > > > > > > > IMPORTANT: if you fix the bug, please add the following tag to the > > > > commit: > > > > Reported-by: syzbot+cfe6d93e0abab9a0d...@syzkaller.appspotmail.com > > > > > > > > > > #syz test: https://github.com/google/kasan.git d0847550 > > > > There's no need for us to work at cross purposes on this. We can go > > with your approach. > > > > However, the code is more complicated than your patch accounts for. > > The wait can finish in several different ways: > > > > (1) The control URB succeeds and the interrupt URB gets an > > acknowledgment. > > > > (2) The control URB completes with an error. > > > > (3) The wait times out. > > > > (4) A disconnect occurs. > > I absolutely agree. There is something quite wrong in this driver. > Unfortunately this is likely exploitable by a malicious gadget, > so just ignoring this is a bad option. I will need to go through the > logic. Or do you want to have a shot at it? > > The patch was really only for testing. I wanted to know whether > I was hitting this very issue. This driver will need more surgery. PS: Referring to yurex Regards Oliver
Re: KASAN: use-after-free Read in iowarrior_disconnect
Am Dienstag, den 20.08.2019, 10:18 -0400 schrieb Alan Stern: > On Mon, 19 Aug 2019, Oliver Neukum wrote: > > > Am Montag, den 19.08.2019, 07:48 -0700 schrieb syzbot: > > > Hello, > > > > > > syzbot found the following crash on: > > > > > > HEAD commit:d0847550 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=139be30260 > > > kernel config: https://syzkaller.appspot.com/x/.config?x=dbc9c80cc095da19 > > > dashboard link: > > > https://syzkaller.appspot.com/bug?extid=cfe6d93e0abab9a0de05 > > > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=12fe6b0260 > > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1548189c60 > > > > > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > > > Reported-by: syzbot+cfe6d93e0abab9a0d...@syzkaller.appspotmail.com > > > > > > > #syz test: https://github.com/google/kasan.git d0847550 > > There's no need for us to work at cross purposes on this. We can go > with your approach. > > However, the code is more complicated than your patch accounts for. > The wait can finish in several different ways: > > (1) The control URB succeeds and the interrupt URB gets an > acknowledgment. > > (2) The control URB completes with an error. > > (3) The wait times out. > > (4) A disconnect occurs. I absolutely agree. There is something quite wrong in this driver. Unfortunately this is likely exploitable by a malicious gadget, so just ignoring this is a bad option. I will need to go through the logic. Or do you want to have a shot at it? The patch was really only for testing. I wanted to know whether I was hitting this very issue. This driver will need more surgery. Regards Oliver
Re: Duplicated code in hiddev_open()
Am Montag, den 19.08.2019, 10:17 -0400 schrieb Alan Stern:
> On Mon, 19 Aug 2019, Oliver Neukum wrote:
>
> > Am Freitag, den 16.08.2019, 13:10 -0400 schrieb Alan Stern:
> > > Oliver and Jiri:
> > >
> > > Why is there duplicated code in
> > > drivers/hid/usbhid/hiddev.c:hiddev_open()?
> > >
> > > Line 267:
> > > /*
> > >* no need for locking because the USB major number
> > >* is shared which usbcore guards against disconnect
> > >*/
> > > if (list->hiddev->exist) {
> > > if (!list->hiddev->open++) {
> > > res = hid_hw_open(hiddev->hid);
> > > if (res < 0)
> > > goto bail;
> > > }
> > > } else {
> > > res = -ENODEV;
> > > goto bail;
> > > }
> > >
> > > Line 286:
> > > mutex_lock(&hiddev->existancelock);
> > > if (!list->hiddev->open++)
> > > if (list->hiddev->exist) {
> > > struct hid_device *hid = hiddev->hid;
> > > res = hid_hw_power(hid, PM_HINT_FULLON);
> > > if (res < 0)
> > > goto bail_unlock;
> > > res = hid_hw_open(hid);
> > > if (res < 0)
> > > goto bail_normal_power;
> > > }
> > > mutex_unlock(&hiddev->existancelock);
> > >
> > > The second part can never execute, because the first part ensures that
> > > list->hiddev->open > 0 by the time the second part runs.
> > >
> > > Even more disturbing, why is one of these code sections protected by a
> > > mutex and the other not?
> >
> > I suppose the comment I made back then:
> >
> > 079034073faf9 drivers/hid/usbhid/hiddev.c (Oliver Neukum
> > 2008-12-16 10:55:15 +0100 268)* no need for locking because the USB
> > major number
> > 079034073faf9 drivers/hid/usbhid/hiddev.c (Oliver Neukum
> > 2008-12-16 10:55:15 +0100 269)* is shared which usbcore guards against
> > disconnect
> >
> > has ceased to be true, but the section was not removed, as the check
> > for existance was duplicated.
> >
> > > Note: The second section was added in commit 0361a28d3f9a ("HID:
> > > autosuspend support for USB HID") over ten years ago!
> >
> > Yes and I remember how frustrating keyboards were in testing, but
> > no further details.
>
> Indeed. But more importantly for now, how should this be fixed? This
> may be the culprit in some of the syzbot bug reports (those involving
> hiddev).
I doubt it. This looks like it would cause a resource leak, not the
other way round. But I'd say all operations need to be done under lock.
Regards
oliver
Re: WARNING in kmem_cache_alloc_trace
Am Dienstag, den 20.08.2019, 06:40 -0700 schrieb syzbot:
> Hello,
>
> syzbot has tested the proposed patch but the reproducer still triggered
> crash:
> WARNING in yurex_write/usb_submit_urb
It looks to me like we have two issues here. The driver is simply not
ready to deal with concurrent writers. Is that one of the test cases?
Regards
Oliver
Re: WARNING in wdm_write/usb_submit_urb
Am Montag, den 19.08.2019, 17:50 -0700 schrieb syzbot:
> syzbot has found a reproducer for the following crash on:
Hi Bjørn,
taking you into CC as you are affected.
V4:
> HEAD commit:e06ce4da 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=14a8c0b660
> kernel config: https://syzkaller.appspot.com/x/.config?x=d0c62209eedfd54e
> dashboard link: https://syzkaller.appspot.com/bug?extid=d232cca6ec42c2edb3fc
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=12b6dfba60
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=15f63a4c60
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+d232cca6ec42c2edb...@syzkaller.appspotmail.com
>
> [ cut here ]
> URB 5fab893a submitted while active
> WARNING: CPU: 1 PID: 1788 at drivers/usb/core/urb.c:362
> usb_submit_urb+0x10c1/0x13b0 drivers/usb/core/urb.c:362
> Kernel panic - not syncing: panic_on_warn set ...
> CPU: 1 PID: 1788 Comm: syz-executor522 Not tainted 5.3.0-rc5+ #27
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0xca/0x13e lib/dump_stack.c:113
> panic+0x2a3/0x6da kernel/panic.c:219
> __warn.cold+0x20/0x4a kernel/panic.c:576
> report_bug+0x262/0x2a0 lib/bug.c:186
> fixup_bug arch/x86/kernel/traps.c:179 [inline]
> fixup_bug arch/x86/kernel/traps.c:174 [inline]
> do_error_trap+0x12b/0x1e0 arch/x86/kernel/traps.c:272
> do_invalid_op+0x32/0x40 arch/x86/kernel/traps.c:291
> invalid_op+0x23/0x30 arch/x86/entry/entry_64.S:1028
> RIP: 0010:usb_submit_urb+0x10c1/0x13b0 drivers/usb/core/urb.c:362
> Code: 89 de e8 82 bc ef fd 84 db 0f 85 42 f6 ff ff e8 45 bb ef fd 4c 89 fe
> 48 c7 c7 80 68 18 86 c6 05 27 30 3a 04 01 e8 34 a1 c5 fd <0f> 0b e9 20 f6
> ff ff c7 44 24 14 01 00 00 00 e9 d7 f6 ff ff 41 bd
> RSP: 0018:8881d036fc98 EFLAGS: 00010286
> RAX: RBX: RCX:
> RDX: RSI: 81288cfd RDI: ed103a06df85
> RBP: 8881cfce56a0 R08: 8881d1ce4800 R09: ed103b663ee7
> R10: ed103b663ee6 R11: 8881db31f737 R12: 11103a06dfa7
> R13: fff0 R14: 8881cfce5688 R15: 8881d8106d00
> wdm_write+0x828/0xd87 drivers/usb/class/cdc-wdm.c:423
> __vfs_write+0x76/0x100 fs/read_write.c:494
> vfs_write+0x262/0x5c0 fs/read_write.c:558
> ksys_write+0x127/0x250 fs/read_write.c:611
> do_syscall_64+0xb7/0x580 arch/x86/entry/common.c:296
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
> RIP: 0033:0x447029
> Code: e8 ec e7 ff ff 48 83 c4 18 c3 0f 1f 80 00 00 00 00 48 89 f8 48 89 f7
> 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
> ff 0f 83 3b 07 fc ff c3 66 2e 0f 1f 84 00 00 00 00
> RSP: 002b:7f1e9e0a4da8 EFLAGS: 0246 ORIG_RAX: 0001
> RAX: ffda RBX: 006dcc28 RCX: 00447029
> RDX: RSI: RDI: 0004
> RBP: 006dcc20 R08: R09:
> R10: R11: 0246 R12: 006dcc2c
> R13: 2000 R14: 004af170 R15: 03e8
#syz test: https://github.com/google/kasan.git e06ce4da
>From 8973b1216f931f4c7b82b02186caee9dcae16d24 Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Tue, 20 Aug 2019 12:08:19 +0200
Subject: [PATCH] USB: cdc-wdm: fix race between write and disconnect due to
flag abuse
In case of a disconnect an ongoing flush() has to be made fail.
Nevertheless we cannot be sure that any pending URB has already
finished, so although they will never succeed, they still must
not be touched.
The clean solution for this is to check for WDM_IN_USE
and WDM_DISCONNECTED in flush(). There is no point in ever
clearing WDM_IN_USE, as no further writes make sense.
The issue is as old as the driver.
Fixes: afba937e540c9 ("USB: CDC WDM driver")
Reported-by: syzbot+d232cca6ec42c2edb...@syzkaller.appspotmail.com
Signed-off-by: Oliver Neukum
---
drivers/usb/class/cdc-wdm.c | 16
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 1656f5155ab8..f9f7c8a5e091 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -588,10 +588,20 @@ static int wdm_flush(struct file *file, fl_owner_t id)
{
struct wdm_device *desc = file->private_data;
- wait_event(desc->wait, !test_bit(WDM_IN_USE, &desc->flags));
+ wait_event(desc->wai
Re: WARNING in wdm_write/usb_submit_urb
Am Dienstag, den 20.08.2019, 15:13 +0200 schrieb Bjørn Mork : > Oliver Neukum writes: > > > + wait_event(desc->wait, > > + /* > > +* needs both flags. We cannot do with one > > +* because resetting it would cause a race > > +* with write() yet we need to signal > > +* a disconnect > > +*/ > > + !test_bit(WDM_IN_USE, &desc->flags) && > > + test_bit(WDM_DISCONNECTING, &desc->flags)); > > I'm confused now... Won't this condition always be false? > Should be > > wait_event(desc->wait, > !test_bit(WDM_IN_USE, &desc->flags) || > test_bit(WDM_DISCONNECTING, &desc->flags)); > > > instead, or? ... You are right. Regards Oliver
Re: WARNING in kmem_cache_alloc_trace
Am Montag, den 19.08.2019, 18:59 +0200 schrieb Andrey Konovalov:
> On Mon, Aug 19, 2019 at 6:18 PM syzbot
> wrote:
> >
> > Hello,
> >
> > syzbot found the following crash on:
> >
> > HEAD commit:d0847550 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=16947fce60
> > kernel config: https://syzkaller.appspot.com/x/.config?x=dbc9c80cc095da19
> > dashboard link: https://syzkaller.appspot.com/bug?extid=0e7b6b6001ca8ed655f6
> > compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1141c5ba60
> > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=11ed91d260
> >
> > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > Reported-by: syzbot+0e7b6b6001ca8ed65...@syzkaller.appspotmail.com
> >
#syz test: https://github.com/google/kasan.git d0847550
>From 8d100dc018a0c1ba9c25dc5a222527ea4748f4c7 Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Tue, 20 Aug 2019 15:04:00 +0200
Subject: [PATCH] USB: yurex: fix failure to wait for control message
Using usb_submit_urb() after prepare_to_wait() won't work, because
it may reset the task state to TASK_RUNNING. Replacing it with
a completion.
Signed-off-by: Oliver Neukum
---
drivers/usb/misc/yurex.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/misc/yurex.c b/drivers/usb/misc/yurex.c
index 6715a128e6c8..64b2bfe7fb83 100644
--- a/drivers/usb/misc/yurex.c
+++ b/drivers/usb/misc/yurex.c
@@ -62,6 +62,7 @@ struct usb_yurex {
struct mutexio_mutex;
struct fasync_struct*async_queue;
wait_queue_head_t waitq;
+ struct completion cntl_cpl;
spinlock_t lock;
__s64 bbu;/* BBU from device */
@@ -80,7 +81,7 @@ static void yurex_control_callback(struct urb *urb)
if (status) {
dev_err(&urb->dev->dev, "%s - control failed: %d\n",
__func__, status);
- wake_up_interruptible(&dev->waitq);
+ complete(&dev->cntl_cpl);
return;
}
/* on success, sender woken up by CMD_ACK int in, or timeout */
@@ -202,6 +203,7 @@ static int yurex_probe(struct usb_interface *interface,
const struct usb_device_
mutex_init(&dev->io_mutex);
spin_lock_init(&dev->lock);
init_waitqueue_head(&dev->waitq);
+ init_completion(&dev->cntl_cpl);
dev->udev = usb_get_dev(interface_to_usbdev(interface));
dev->interface = interface;
@@ -322,6 +324,7 @@ static void yurex_disconnect(struct usb_interface
*interface)
/* wakeup waiters */
kill_fasync(&dev->async_queue, SIGIO, POLL_IN);
wake_up_interruptible(&dev->waitq);
+ complete(&dev->cntl_cpl);
/* decrement our usage count */
kref_put(&dev->kref, yurex_delete);
@@ -485,13 +488,10 @@ static ssize_t yurex_write(struct file *file, const char
__user *user_buffer,
}
/* send the data as the control msg */
- prepare_to_wait(&dev->waitq, &wait, TASK_INTERRUPTIBLE);
dev_dbg(&dev->interface->dev, "%s - submit %c\n", __func__,
dev->cntl_buffer[0]);
retval = usb_submit_urb(dev->cntl_urb, GFP_KERNEL);
- if (retval >= 0)
- timeout = schedule_timeout(YUREX_WRITE_TIMEOUT);
- finish_wait(&dev->waitq, &wait);
+ timeout = wait_for_completion_interruptible_timeout(&dev->cntl_cpl,
YUREX_WRITE_TIMEOUT);
mutex_unlock(&dev->io_mutex);
--
2.16.4
Re: WARNING in kmem_cache_alloc_trace
Am Montag, den 19.08.2019, 18:59 +0200 schrieb Andrey Konovalov:
> On Mon, Aug 19, 2019 at 6:18 PM syzbot
> wrote:
> >
> > Hello,
> >
> > syzbot found the following crash on:
> >
> > HEAD commit:d0847550 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=16947fce60
> > kernel config: https://syzkaller.appspot.com/x/.config?x=dbc9c80cc095da19
> > dashboard link: https://syzkaller.appspot.com/bug?extid=0e7b6b6001ca8ed655f6
> > compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1141c5ba60
> > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=11ed91d260
> >
> > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > Reported-by: syzbot+0e7b6b6001ca8ed65...@syzkaller.appspotmail.com
> >
#syz test: https://github.com/google/kasan.git d0847550
>From eeb920819e1d98e631fb78fe849649dc8dd6eb1b Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Tue, 20 Aug 2019 15:04:00 +0200
Subject: [PATCH] USB: yurex: fix failure to wait for control message
Using usb_submit_urb() after prepare_to_wait() won't work, because
it may reset the task state to TASK_RUNNING. Replacing it with
a completion.
Signed-off-by: Oliver Neukum
---
drivers/usb/misc/yurex.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/misc/yurex.c b/drivers/usb/misc/yurex.c
index 6715a128e6c8..519bb53993aa 100644
--- a/drivers/usb/misc/yurex.c
+++ b/drivers/usb/misc/yurex.c
@@ -62,6 +62,7 @@ struct usb_yurex {
struct mutexio_mutex;
struct fasync_struct*async_queue;
wait_queue_head_t waitq;
+ struct completion cntl_cpl;
spinlock_t lock;
__s64 bbu;/* BBU from device */
@@ -80,7 +81,7 @@ static void yurex_control_callback(struct urb *urb)
if (status) {
dev_err(&urb->dev->dev, "%s - control failed: %d\n",
__func__, status);
- wake_up_interruptible(&dev->waitq);
+ complete(&dev->cntl_cpl);
return;
}
/* on success, sender woken up by CMD_ACK int in, or timeout */
@@ -202,6 +203,7 @@ static int yurex_probe(struct usb_interface *interface,
const struct usb_device_
mutex_init(&dev->io_mutex);
spin_lock_init(&dev->lock);
init_waitqueue_head(&dev->waitq);
+ init_completion(&dev->cntl_cpl);
dev->udev = usb_get_dev(interface_to_usbdev(interface));
dev->interface = interface;
@@ -322,6 +324,7 @@ static void yurex_disconnect(struct usb_interface
*interface)
/* wakeup waiters */
kill_fasync(&dev->async_queue, SIGIO, POLL_IN);
wake_up_interruptible(&dev->waitq);
+ complete(&dev->cntl_cpl);
/* decrement our usage count */
kref_put(&dev->kref, yurex_delete);
@@ -485,13 +488,10 @@ static ssize_t yurex_write(struct file *file, const char
__user *user_buffer,
}
/* send the data as the control msg */
- prepare_to_wait(&dev->waitq, &wait, TASK_INTERRUPTIBLE);
dev_dbg(&dev->interface->dev, "%s - submit %c\n", __func__,
dev->cntl_buffer[0]);
retval = usb_submit_urb(dev->cntl_urb, GFP_KERNEL);
- if (retval >= 0)
- timeout = schedule_timeout(YUREX_WRITE_TIMEOUT);
- finish_wait(&dev->waitq, &wait);
+ timeout = wait_for_completion_interruptible__timeout(&dev->cntl_cpl,
YUREX_WRITE_TIMEOUT);
mutex_unlock(&dev->io_mutex);
--
2.16.4
Re: WARNING in wdm_write/usb_submit_urb
Am Montag, den 19.08.2019, 17:50 -0700 schrieb syzbot:
> syzbot has found a reproducer for the following crash on:
Hi Bjørn,
taking you into CC as you are affected.
V3: changes you suggested
> HEAD commit:e06ce4da 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=14a8c0b660
> kernel config: https://syzkaller.appspot.com/x/.config?x=d0c62209eedfd54e
> dashboard link: https://syzkaller.appspot.com/bug?extid=d232cca6ec42c2edb3fc
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=12b6dfba60
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=15f63a4c60
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+d232cca6ec42c2edb...@syzkaller.appspotmail.com
>
> [ cut here ]
> URB 5fab893a submitted while active
> WARNING: CPU: 1 PID: 1788 at drivers/usb/core/urb.c:362
> usb_submit_urb+0x10c1/0x13b0 drivers/usb/core/urb.c:362
> Kernel panic - not syncing: panic_on_warn set ...
> CPU: 1 PID: 1788 Comm: syz-executor522 Not tainted 5.3.0-rc5+ #27
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0xca/0x13e lib/dump_stack.c:113
> panic+0x2a3/0x6da kernel/panic.c:219
> __warn.cold+0x20/0x4a kernel/panic.c:576
> report_bug+0x262/0x2a0 lib/bug.c:186
> fixup_bug arch/x86/kernel/traps.c:179 [inline]
> fixup_bug arch/x86/kernel/traps.c:174 [inline]
> do_error_trap+0x12b/0x1e0 arch/x86/kernel/traps.c:272
> do_invalid_op+0x32/0x40 arch/x86/kernel/traps.c:291
> invalid_op+0x23/0x30 arch/x86/entry/entry_64.S:1028
> RIP: 0010:usb_submit_urb+0x10c1/0x13b0 drivers/usb/core/urb.c:362
> Code: 89 de e8 82 bc ef fd 84 db 0f 85 42 f6 ff ff e8 45 bb ef fd 4c 89 fe
> 48 c7 c7 80 68 18 86 c6 05 27 30 3a 04 01 e8 34 a1 c5 fd <0f> 0b e9 20 f6
> ff ff c7 44 24 14 01 00 00 00 e9 d7 f6 ff ff 41 bd
> RSP: 0018:8881d036fc98 EFLAGS: 00010286
> RAX: RBX: RCX:
> RDX: RSI: 81288cfd RDI: ed103a06df85
> RBP: 8881cfce56a0 R08: 8881d1ce4800 R09: ed103b663ee7
> R10: ed103b663ee6 R11: 8881db31f737 R12: 11103a06dfa7
> R13: fff0 R14: 8881cfce5688 R15: 8881d8106d00
> wdm_write+0x828/0xd87 drivers/usb/class/cdc-wdm.c:423
> __vfs_write+0x76/0x100 fs/read_write.c:494
> vfs_write+0x262/0x5c0 fs/read_write.c:558
> ksys_write+0x127/0x250 fs/read_write.c:611
> do_syscall_64+0xb7/0x580 arch/x86/entry/common.c:296
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
> RIP: 0033:0x447029
> Code: e8 ec e7 ff ff 48 83 c4 18 c3 0f 1f 80 00 00 00 00 48 89 f8 48 89 f7
> 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
> ff 0f 83 3b 07 fc ff c3 66 2e 0f 1f 84 00 00 00 00
> RSP: 002b:7f1e9e0a4da8 EFLAGS: 0246 ORIG_RAX: 0001
> RAX: ffda RBX: 006dcc28 RCX: 00447029
> RDX: RSI: RDI: 0004
> RBP: 006dcc20 R08: R09:
> R10: R11: 0246 R12: 006dcc2c
> R13: 2000 R14: 004af170 R15: 03e8
#syz test: https://github.com/google/kasan.git e06ce4da
>From a867eff99e3d4099c8e810c4ed2695f3ecdcab41 Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Tue, 20 Aug 2019 12:08:19 +0200
Subject: [PATCH] USB: cdc-wdm: fix race between write and disconnect due to
flag abuse
In case of a disconnect an ongoing flush() has to be made fail.
Nevertheless we cannot be sure that any pending URB has already
finished, so although they will never succeed, they still must
not be touched.
The clean solution for this is to check for WDM_IN_USE
and WDM_DISCONNECTED in flush(). There is no point in ever
clearing WDM_IN_USE, as no further writes make sense.
The issue is as old as the driver.
Fixes: afba937e540c9 ("USB: CDC WDM driver")
Reported-by: syzbot+d232cca6ec42c2edb...@syzkaller.appspotmail.com
Signed-off-by: Oliver Neukum
---
drivers/usb/class/cdc-wdm.c | 16
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 1656f5155ab8..f2cb5d401790 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -588,10 +588,20 @@ static int wdm_flush(struct file *file, fl_owner_t id)
{
struct wdm_device *desc = file->private_data;
- wait_event(desc->wait, !test_bit(WDM_IN_USE, &a
Re: WARNING in wdm_write/usb_submit_urb
Am Dienstag, den 20.08.2019, 12:44 +0200 schrieb Bjørn Mork :
> Oliver Neukum writes:
>
> > diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
> > index 1656f5155ab8..a341081a5f47 100644
> > --- a/drivers/usb/class/cdc-wdm.c
> > +++ b/drivers/usb/class/cdc-wdm.c
> > @@ -588,14 +588,24 @@ static int wdm_flush(struct file *file, fl_owner_t id)
> > {
> > struct wdm_device *desc = file->private_data;
> >
> > - wait_event(desc->wait, !test_bit(WDM_IN_USE, &desc->flags));
> > + wait_event(desc->wait,
> > + /*
> > +* needs both flags. We cannot do with one
> > +* because resetting it would cause a race
> > +* with write() yet we need to signal
> > +* a disconnect
> > +*/
> > + !test_bit(WDM_IN_USE, &desc->flags) &&
> > + !test_bit(WDM_DISCONNECTING, &desc->flags));
>
>
> Makes sense. But isn't the WDM_DISCONNECTING test inverted?
You are right. I am making V3.
> > /* cannot dereference desc->intf if WDM_DISCONNECTING */
> > if (desc->werr < 0 && !test_bit(WDM_DISCONNECTING, &desc->flags))
> > dev_err(&desc->intf->dev, "Error in flush path: %d\n",
> > desc->werr);
> >
> > - return usb_translate_errors(desc->werr);
> > + return test_bit(WDM_DISCONNECTING, &desc->flags) ?
> > + -ENODEV :
> > + usb_translate_errors(desc->werr);
> > }
>
> Minor detail, but there's an awful lot of test_bit(WDM_DISCONNECTING)
> here. How about
>
> if (test_bit(WDM_DISCONNECTING, &desc->flags))
> return -ENODEV;
> if (desc->werr < 0)
> dev_err(&desc->intf->dev, "Error in flush path: %d\n", desc->werr);
> return usb_translate_errors(desc->werr);
Much better.
Regards
Oliver
Re: WARNING in wdm_write/usb_submit_urb
Am Montag, den 19.08.2019, 17:50 -0700 schrieb syzbot:
> syzbot has found a reproducer for the following crash on:
Hi Bjørn,
taking you into CC as you are affected.
V2: fix logic bug
Regards
Oliver
> HEAD commit:e06ce4da 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=14a8c0b660
> kernel config: https://syzkaller.appspot.com/x/.config?x=d0c62209eedfd54e
> dashboard link: https://syzkaller.appspot.com/bug?extid=d232cca6ec42c2edb3fc
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=12b6dfba60
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=15f63a4c60
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+d232cca6ec42c2edb...@syzkaller.appspotmail.com
>
> [ cut here ]
> URB 5fab893a submitted while active
> WARNING: CPU: 1 PID: 1788 at drivers/usb/core/urb.c:362
> usb_submit_urb+0x10c1/0x13b0 drivers/usb/core/urb.c:362
> Kernel panic - not syncing: panic_on_warn set ...
> CPU: 1 PID: 1788 Comm: syz-executor522 Not tainted 5.3.0-rc5+ #27
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0xca/0x13e lib/dump_stack.c:113
> panic+0x2a3/0x6da kernel/panic.c:219
> __warn.cold+0x20/0x4a kernel/panic.c:576
> report_bug+0x262/0x2a0 lib/bug.c:186
> fixup_bug arch/x86/kernel/traps.c:179 [inline]
> fixup_bug arch/x86/kernel/traps.c:174 [inline]
> do_error_trap+0x12b/0x1e0 arch/x86/kernel/traps.c:272
> do_invalid_op+0x32/0x40 arch/x86/kernel/traps.c:291
> invalid_op+0x23/0x30 arch/x86/entry/entry_64.S:1028
> RIP: 0010:usb_submit_urb+0x10c1/0x13b0 drivers/usb/core/urb.c:362
> Code: 89 de e8 82 bc ef fd 84 db 0f 85 42 f6 ff ff e8 45 bb ef fd 4c 89 fe
> 48 c7 c7 80 68 18 86 c6 05 27 30 3a 04 01 e8 34 a1 c5 fd <0f> 0b e9 20 f6
> ff ff c7 44 24 14 01 00 00 00 e9 d7 f6 ff ff 41 bd
> RSP: 0018:8881d036fc98 EFLAGS: 00010286
> RAX: RBX: RCX:
> RDX: RSI: 81288cfd RDI: ed103a06df85
> RBP: 8881cfce56a0 R08: 8881d1ce4800 R09: ed103b663ee7
> R10: ed103b663ee6 R11: 8881db31f737 R12: 11103a06dfa7
> R13: fff0 R14: 8881cfce5688 R15: 8881d8106d00
> wdm_write+0x828/0xd87 drivers/usb/class/cdc-wdm.c:423
> __vfs_write+0x76/0x100 fs/read_write.c:494
> vfs_write+0x262/0x5c0 fs/read_write.c:558
> ksys_write+0x127/0x250 fs/read_write.c:611
> do_syscall_64+0xb7/0x580 arch/x86/entry/common.c:296
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
> RIP: 0033:0x447029
> Code: e8 ec e7 ff ff 48 83 c4 18 c3 0f 1f 80 00 00 00 00 48 89 f8 48 89 f7
> 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
> ff 0f 83 3b 07 fc ff c3 66 2e 0f 1f 84 00 00 00 00
> RSP: 002b:7f1e9e0a4da8 EFLAGS: 0246 ORIG_RAX: 0001
> RAX: ffda RBX: 006dcc28 RCX: 00447029
> RDX: RSI: RDI: 0004
> RBP: 006dcc20 R08: R09:
> R10: R11: 0246 R12: 006dcc2c
> R13: 2000 R14: 004af170 R15: 03e8
#syz test: https://github.com/google/kasan.git e06ce4da
>From 8d0c7a38b1a4883e7bdab76b06a9e564fc466506 Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Tue, 20 Aug 2019 12:08:19 +0200
Subject: [PATCH] USB: cdc-wdm: fix race between write and disconnect due to
flag abuse
In case of a disconnect an ongoing flush() has to be made fail.
Nevertheless we cannot be sure that any pending URB has already
finished, so although they will never succeed, they still must
not be touched.
The clean solution for this is to check for WDM_IN_USE
and WDM_DISCONNECTED in flush(). There is no point in ever
clearing WDM_IN_USE, as no further writes make sense.
The issue is as old as the driver.
Fixes: afba937e540c9 ("USB: CDC WDM driver")
Reported-by: syzbot+d232cca6ec42c2edb...@syzkaller.appspotmail.com
Signed-off-by: Oliver Neukum
---
drivers/usb/class/cdc-wdm.c | 16
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 1656f5155ab8..2b341f3a155f 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -588,14 +588,24 @@ static int wdm_flush(struct file *file, fl_owner_t id)
{
struct wdm_device *desc = file->private_data;
- wait_event(desc->wait, !test_bit(WDM_IN_US
Re: WARNING in wdm_write/usb_submit_urb
Am Montag, den 19.08.2019, 17:50 -0700 schrieb syzbot:
> syzbot has found a reproducer for the following crash on:
Hi Bjørn,
taking you into CC as you are affected.
> HEAD commit:e06ce4da 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=14a8c0b660
> kernel config: https://syzkaller.appspot.com/x/.config?x=d0c62209eedfd54e
> dashboard link: https://syzkaller.appspot.com/bug?extid=d232cca6ec42c2edb3fc
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=12b6dfba60
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=15f63a4c60
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+d232cca6ec42c2edb...@syzkaller.appspotmail.com
>
> [ cut here ]
> URB 5fab893a submitted while active
> WARNING: CPU: 1 PID: 1788 at drivers/usb/core/urb.c:362
> usb_submit_urb+0x10c1/0x13b0 drivers/usb/core/urb.c:362
> Kernel panic - not syncing: panic_on_warn set ...
> CPU: 1 PID: 1788 Comm: syz-executor522 Not tainted 5.3.0-rc5+ #27
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0xca/0x13e lib/dump_stack.c:113
> panic+0x2a3/0x6da kernel/panic.c:219
> __warn.cold+0x20/0x4a kernel/panic.c:576
> report_bug+0x262/0x2a0 lib/bug.c:186
> fixup_bug arch/x86/kernel/traps.c:179 [inline]
> fixup_bug arch/x86/kernel/traps.c:174 [inline]
> do_error_trap+0x12b/0x1e0 arch/x86/kernel/traps.c:272
> do_invalid_op+0x32/0x40 arch/x86/kernel/traps.c:291
> invalid_op+0x23/0x30 arch/x86/entry/entry_64.S:1028
> RIP: 0010:usb_submit_urb+0x10c1/0x13b0 drivers/usb/core/urb.c:362
> Code: 89 de e8 82 bc ef fd 84 db 0f 85 42 f6 ff ff e8 45 bb ef fd 4c 89 fe
> 48 c7 c7 80 68 18 86 c6 05 27 30 3a 04 01 e8 34 a1 c5 fd <0f> 0b e9 20 f6
> ff ff c7 44 24 14 01 00 00 00 e9 d7 f6 ff ff 41 bd
> RSP: 0018:8881d036fc98 EFLAGS: 00010286
> RAX: RBX: RCX:
> RDX: RSI: 81288cfd RDI: ed103a06df85
> RBP: 8881cfce56a0 R08: 8881d1ce4800 R09: ed103b663ee7
> R10: ed103b663ee6 R11: 8881db31f737 R12: 11103a06dfa7
> R13: fff0 R14: 8881cfce5688 R15: 8881d8106d00
> wdm_write+0x828/0xd87 drivers/usb/class/cdc-wdm.c:423
> __vfs_write+0x76/0x100 fs/read_write.c:494
> vfs_write+0x262/0x5c0 fs/read_write.c:558
> ksys_write+0x127/0x250 fs/read_write.c:611
> do_syscall_64+0xb7/0x580 arch/x86/entry/common.c:296
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
> RIP: 0033:0x447029
> Code: e8 ec e7 ff ff 48 83 c4 18 c3 0f 1f 80 00 00 00 00 48 89 f8 48 89 f7
> 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
> ff 0f 83 3b 07 fc ff c3 66 2e 0f 1f 84 00 00 00 00
> RSP: 002b:7f1e9e0a4da8 EFLAGS: 0246 ORIG_RAX: 0001
> RAX: ffda RBX: 006dcc28 RCX: 00447029
> RDX: RSI: RDI: 0004
> RBP: 006dcc20 R08: R09:
> R10: R11: 0246 R12: 006dcc2c
> R13: 2000 R14: 004af170 R15: 03e8
#syz test: https://github.com/google/kasan.git e06ce4da
>From 38dda97aa4820a60c680374edbab0323cfcb0193 Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Tue, 20 Aug 2019 12:08:19 +0200
Subject: [PATCH] USB: cdc-wdm: fix race between write and disconnect due to
flag abuse
In case of a disconnect an ongoing flush() has to be made fail.
Nevertheless we cannot be sure that any pending URB has already
finished, so although they will never succeed, they still must
not be touched.
The clean solution for this is to check for WDM_IN_USE
and WDM_DISCONNECTED in flush(). There is no point in ever
clearing WDM_IN_USE, as no further writes make sense.
The issue is as old as the driver.
Fixes: afba937e540c9 ("USB: CDC WDM driver")
Reported-by: syzbot+d232cca6ec42c2edb...@syzkaller.appspotmail.com
Signed-off-by: Oliver Neukum
---
drivers/usb/class/cdc-wdm.c | 16
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 1656f5155ab8..a341081a5f47 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -588,14 +588,24 @@ static int wdm_flush(struct file *file, fl_owner_t id)
{
struct wdm_device *desc = file->private_data;
- wait_event(desc->wait, !test_bit(WDM_IN_USE, &desc->flags));
+ wait_event(desc->wait,
[PATCH] usbtmc: more sanity checking for packet size
A malicious device can make the driver divide ny zero with a nonsense maximum packet size. V2: return a sensible error code SIgned-off-by: Oliver Neukum --- drivers/usb/class/usbtmc.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index 4942122b2346..36858ddd8d9b 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -2362,8 +2362,11 @@ static int usbtmc_probe(struct usb_interface *intf, goto err_put; } + retcode = -EINVAL; data->bulk_in = bulk_in->bEndpointAddress; data->wMaxPacketSize = usb_endpoint_maxp(bulk_in); + if (!data->wMaxPacketSize) + goto err_put; dev_dbg(&intf->dev, "Found bulk in endpoint at %u\n", data->bulk_in); data->bulk_out = bulk_out->bEndpointAddress; -- 2.16.4
Re: divide error in usbtmc_generic_read
Am Montag, den 19.08.2019, 17:40 +0200 schrieb Andrey Konovalov: > > This implies that we can differentiate between different crashes. We > can differentiate between different manifestations of crashes, but > those can be caused by the same bug. I think we can remove the word > "still" though, so the words will be: "syzbot has tested the proposed > patch, but the reproducer triggered a crash". That is exactly my point. There are three cases: - no crash - clearly the same crash - something Yet syzbot has two answers only. We are killing information needlessly. Regards Oliver
Re: KASAN: use-after-free Read in iowarrior_disconnect
Am Montag, den 19.08.2019, 07:48 -0700 schrieb syzbot:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit:d0847550 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=139be30260
> kernel config: https://syzkaller.appspot.com/x/.config?x=dbc9c80cc095da19
> dashboard link: https://syzkaller.appspot.com/bug?extid=cfe6d93e0abab9a0de05
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=12fe6b0260
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1548189c60
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+cfe6d93e0abab9a0d...@syzkaller.appspotmail.com
>
#syz test: https://github.com/google/kasan.git d0847550
From 43c4270a424052dcb168a0fea5a9ad89778eadc7 Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Mon, 19 Aug 2019 17:22:53 +0200
Subject: [PATCH] Revert "usb: iowarrior: fix deadlock on disconnect"
This reverts commit aa40cfb4d2f134322a782b18a687d04300f50f60.
---
drivers/usb/misc/iowarrior.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/misc/iowarrior.c b/drivers/usb/misc/iowarrior.c
index f5bed9f29e56..ba05dd80a020 100644
--- a/drivers/usb/misc/iowarrior.c
+++ b/drivers/usb/misc/iowarrior.c
@@ -866,20 +866,19 @@ static void iowarrior_disconnect(struct usb_interface *interface)
dev = usb_get_intfdata(interface);
mutex_lock(&iowarrior_open_disc_lock);
usb_set_intfdata(interface, NULL);
- /* prevent device read, write and ioctl */
- dev->present = 0;
minor = dev->minor;
- mutex_unlock(&iowarrior_open_disc_lock);
- /* give back our minor - this will call close() locks need to be dropped at this point*/
+ /* give back our minor */
usb_deregister_dev(interface, &iowarrior_class);
mutex_lock(&dev->mutex);
/* prevent device read, write and ioctl */
+ dev->present = 0;
mutex_unlock(&dev->mutex);
+ mutex_unlock(&iowarrior_open_disc_lock);
if (dev->opened) {
/* There is a process that holds a filedescriptor to the device ,
--
2.16.4
Re: divide error in usbtmc_generic_read
Am Montag, den 19.08.2019, 15:18 +0200 schrieb Andrey Konovalov: > On Mon, Aug 19, 2019 at 3:09 PM Oliver Neukum wrote: > > > > Am Montag, den 19.08.2019, 14:43 +0200 schrieb Andrey Konovalov: > > > On Mon, Aug 19, 2019 at 2:37 PM Oliver Neukum wrote: > > > > The original error was a divide by zero. The first fix fixed that > > > > but still another error showed up. If I propose a fix there are > > > > other possibilities besides it working. > > > > > > > > I could have no effect on the original bug or my fix breaks > > > > something else and KASAN is making no difference between > > > > those cases. > > > > > > I think you mean syzbot here and not KASAN. Do I understand correctly, > > > that you're saying that the original report was > > > > Yes, sorry syzbot. > > > > > divide-by-zero, but > > > when you requested to test the patch the reproducer triggered a > > > use-after-free, and syzbot didn't treat the patch you provided as a > > > correct fix? > > > > No, obviously there is still a bug. What I would like syzbot to have > > would be a third category: inconclusive. > > Seeing another bug instead may also mean the first bug struck > > before the second could ever happen. We just lack data to tell. > > OK, I see. The exact words that syzbot uses in this case are "syzbot > has tested the proposed patch but the reproducer still triggered > crash". What would you like to see instead? > "syzbot has tested the proposed patch but the reproducer triggered another crash" would make it clearer. Regards Oliver
Re: divide error in usbtmc_generic_read
Am Montag, den 19.08.2019, 14:43 +0200 schrieb Andrey Konovalov: > On Mon, Aug 19, 2019 at 2:37 PM Oliver Neukum wrote: > > The original error was a divide by zero. The first fix fixed that > > but still another error showed up. If I propose a fix there are > > other possibilities besides it working. > > > > I could have no effect on the original bug or my fix breaks > > something else and KASAN is making no difference between > > those cases. > > I think you mean syzbot here and not KASAN. Do I understand correctly, > that you're saying that the original report was Yes, sorry syzbot. > divide-by-zero, but > when you requested to test the patch the reproducer triggered a > use-after-free, and syzbot didn't treat the patch you provided as a > correct fix? No, obviously there is still a bug. What I would like syzbot to have would be a third category: inconclusive. Seeing another bug instead may also mean the first bug struck before the second could ever happen. We just lack data to tell. Regards Oliver
Re: divide error in usbtmc_generic_read
Am Montag, den 19.08.2019, 14:17 +0200 schrieb Andrey Konovalov: > On Thu, Aug 15, 2019 at 3:31 PM Oliver Neukum wrote: > > > > Am Mittwoch, den 14.08.2019, 06:38 -0700 schrieb syzbot: > > > syzbot has tested the proposed patch but the reproducer still triggered > > > crash: > > > KASAN: use-after-free Read in usbtmc_disconnect > > > > I am afraid that is a difficiency in KASAN that should be fixed. > > Is the class of the error compared if I leave in more of the > > original bug report? Actually the ID is still there, so it really > > should return an inconclusive in these cases. > > I don't get this, what kind of deficiency do you mean? The original error was a divide by zero. The first fix fixed that but still another error showed up. If I propose a fix there are other possibilities besides it working. I could have no effect on the original bug or my fix breaks something else and KASAN is making no difference between those cases. Regards Oliver
Re: AW: AW: Strange behaviour of D-Link DUB-1312 USB 3.0 Adapters
xhci_hcd :00:15.0: Transfer error for slot 3 ep 5 on > endpoint > [87800.398172] xhci_hcd :00:15.0: Cleaning up stalled endpoint ring > [87800.398175] xhci_hcd :00:15.0: Finding endpoint context > [87800.398179] xhci_hcd :00:15.0: Cycle state = 0x1 > [87800.398181] xhci_hcd :00:15.0: New dequeue segment = 8d9330b29900 > (virtual) > [87800.398184] xhci_hcd :00:15.0: New dequeue pointer = 0x174213410 (DMA) > [87800.398186] xhci_hcd :00:15.0: Queueing new dequeue state > [87800.398189] xhci_hcd :00:15.0: Set TR Deq Ptr cmd, new deq seg = > 8d9330b29900 (0x174213000 dma), new deq ptr = 8d92b4213410 > (0x174213410 dma), new cycle = 1 > [87800.398192] xhci_hcd :00:15.0: // Ding dong! > [87800.398197] xhci_hcd :00:15.0: Giveback URB 8d92b4374c00, len = 0, > expected = 74, status = -71 > [87800.398209] xhci_hcd :00:15.0: Ignoring reset ep completion code of 1 > [87800.398217] xhci_hcd :00:15.0: Successful Set TR Deq Ptr cmd, deq = > @174213410 > [87800.401654] xhci_hcd :00:15.0: Transfer error for slot 3 ep 5 on > endpoint This points at a low level XHCI thing. Time to get Mathias involved. Regards Oliver
Re: [PATCH] USB: rio500: Fix lockdep violation
Am Donnerstag, den 15.08.2019, 14:48 +0200 schrieb Greg KH:
> On Thu, Aug 08, 2019 at 02:23:00PM -0400, Alan Stern wrote:
> > On Thu, 8 Aug 2019, Greg KH wrote:
> >
> > > On Thu, Aug 08, 2019 at 01:34:08PM -0400, Alan Stern wrote:
> > > > The syzbot fuzzer found a lockdep violation in the rio500 driver:
> > > >
> > > > ==
> > > > WARNING: possible circular locking dependency detected
> > > > 5.3.0-rc2+ #23 Not tainted
> > > > --
> > > > syz-executor.2/20386 is trying to acquire lock:
> > > > 772249c6 (rio500_mutex){+.+.}, at: open_rio+0x16/0xc0
> > > > drivers/usb/misc/rio500.c:64
> > > >
> > > > but task is already holding lock:
> > > > d3e8f4b9 (minor_rwsem){}, at: usb_open+0x23/0x270
> > > > drivers/usb/core/file.c:39
> > > >
> > > > which lock already depends on the new lock.
> > > >
> > > > The problem is that the driver's open_rio() routine is called while
> > > > the usbcore's minor_rwsem is locked for reading, and it acquires the
> > > > rio500_mutex; whereas conversely, probe_rio() and disconnect_rio()
> > > > first acquire the rio500_mutex and then call usb_register_dev() or
> > > > usb_deregister_dev(), which lock minor_rwsem for writing.
> > > >
> > > > The correct ordering of acquisition should be: minor_rwsem first, then
> > > > rio500_mutex (since the locking in open_rio() cannot be changed).
> > > > Thus, the probe and disconnect routines should avoid holding
> > > > rio500_mutex while doing their registration and deregistration.
> > > >
> > > > This patch adjusts the code in those two routines to do just that. It
> > > > also relies on the fact that the probe and disconnect routines are
> > > > protected by the device mutex, so the initial test of rio->present
> > > > needs no extra locking.
> > > >
> > > > Reported-by: syzbot+7bbcbe9c9ff0cd495...@syzkaller.appspotmail.com
> > > > Signed-off-by: Alan Stern
> > > > Fixes: d710734b0677 ("USB: rio500: simplify locking")
> > > > CC: Oliver Neukum
> > > > CC:
> > > >
> > > > ---
> > > >
> > > > This patch is different from the one I posted earlier. I realized that
> > > > we don't want to register the device's char file until after the
> > > > buffers have been allocated.
> > >
> > > Should I revert Oliver's patch?
> >
> > Sorry, I should have explained more clearly: This goes on top of
> > Oliver's patch. In fact, Oliver's patch is the one listed in the
> > Fixes: tag.
> >
> > You do not need to apply Oliver's reversion. Assuming he agrees that
> > this patch is correct, of course.
>
> Ok, I applied the revert, and that's in 5.3-rc4. So of course this does
> not apply :)
>
> Shoudl I revert the revert and then apply this? I will if I can get an
> ack from Oliver...
Acked-by: Oliver Neukum
Re: dwc2 / Raspberry Pi - hardware bug for small transfers results in memory corruption
Am Freitag, den 16.08.2019, 23:18 +0100 schrieb Jonathan Bell: > On Thu, Aug 15, 2019 at 3:52 PM Oliver Neukum wrote: > > That is an accident waiting to happen. Please make a patch using > > a bounce buffer allocated with knalloc() in > > drivers/media/usb/uvc/uvc_ctrl.c:uvc_ctrl_populate_cache() and friends. > > A patch to uvcvideo will not fix the underlying bug with the host > controller hardware. Absolutely correct. > There are hundreds of device drivers of varying > vintages that potentially react badly to having a rogue host > controller DMA engine writing more bytes than were reported by the > controller's interrupt status register. Then these drivers are likely to be buggy. Not guaranteed to, it is possible to write a driver which is correct and still would react badly to that, but it is difficult. > So my original two questions still need answering: > 1) Does the symptom seen with v4l2-ctl exist on other platforms using > dwc2 (which implies that this is not a bug specific to Raspberry Pi) > 2) How do we harden upstream dwc2 against a broken controller DMA? Unknown and very hard to find out, because you are almost always in a situation where you have a full cache line, which is larger than 4 bytes. You must flush all cache lines your buffer is part of. You must not touch them until DMA is complete. That is easiest to achieve if you just kmalloc() each buffer separately. Using two parts of a buffer for subsequent DMA is within the rules, but not worth the trouble. Using a bounce buffer in the dwc2 driver is likely not worth the trouble, as you wouldn't get away with a single buffer and dynamic allocation would suck (it would have to be atomic). Regards Oliver
Re: Duplicated code in hiddev_open()
Am Freitag, den 16.08.2019, 13:10 -0400 schrieb Alan Stern:
> Oliver and Jiri:
>
> Why is there duplicated code in
> drivers/hid/usbhid/hiddev.c:hiddev_open()?
>
> Line 267:
> /*
>* no need for locking because the USB major number
>* is shared which usbcore guards against disconnect
>*/
> if (list->hiddev->exist) {
> if (!list->hiddev->open++) {
> res = hid_hw_open(hiddev->hid);
> if (res < 0)
> goto bail;
> }
> } else {
> res = -ENODEV;
> goto bail;
> }
>
> Line 286:
> mutex_lock(&hiddev->existancelock);
> if (!list->hiddev->open++)
> if (list->hiddev->exist) {
> struct hid_device *hid = hiddev->hid;
> res = hid_hw_power(hid, PM_HINT_FULLON);
> if (res < 0)
> goto bail_unlock;
> res = hid_hw_open(hid);
> if (res < 0)
> goto bail_normal_power;
> }
> mutex_unlock(&hiddev->existancelock);
>
> The second part can never execute, because the first part ensures that
> list->hiddev->open > 0 by the time the second part runs.
>
> Even more disturbing, why is one of these code sections protected by a
> mutex and the other not?
I suppose the comment I made back then:
079034073faf9 drivers/hid/usbhid/hiddev.c (Oliver Neukum
2008-12-16 10:55:15 +0100 268)* no need for locking because the USB major
number
079034073faf9 drivers/hid/usbhid/hiddev.c (Oliver Neukum
2008-12-16 10:55:15 +0100 269)* is shared which usbcore guards against
disconnect
has ceased to be true, but the section was not removed, as the check
for existance was duplicated.
> Note: The second section was added in commit 0361a28d3f9a ("HID:
> autosuspend support for USB HID") over ten years ago!
Yes and I remember how frustrating keyboards were in testing, but
no further details.
Regards
Oliver
Re: dwc2 / Raspberry Pi - hardware bug for small transfers results in memory corruption
Am Donnerstag, den 15.08.2019, 12:41 +0100 schrieb Jonathan Bell: > On Thu, Aug 15, 2019 at 11:55 AM Oliver Neukum wrote: > > > > Am Mittwoch, den 14.08.2019, 16:59 +0100 schrieb Jonathan Bell: > > > As reported by one of our users here: > > > https://github.com/raspberrypi/linux/issues/3148 > > > > > > There is a bug when the dwc2 core receives USB data packets that are > > > between 1 and 4 bytes in length - 4 bytes are always written to memory > > > where the non-packet bytes are garbage. > > > > Hi, > > > > in which function does that happen? If your buffer cannot handle 4 > > bytes I cannot see how it copes with teh DMA rules. > > > > In drivers/media/usb/uvc/uvc_ctrl.c:uvc_ctrl_populate_cache() and friends. OK, I see. > The UVC driver passes in offsets into a struct uvc_control as the > "buffer" that usb_control_msg() fills. Not quite that bad. It passes a pointer into the middle of a buffer used at different offsets for the transfer. This is technically allowed as long as you never touch the buffer while a transfer is ongoing. That is an accident waiting to happen. Please make a patch using a bounce buffer allocated with knalloc() in drivers/media/usb/uvc/uvc_ctrl.c:uvc_ctrl_populate_cache() and friends. Regards Oliver
Re: divide error in usbtmc_generic_read
Am Mittwoch, den 14.08.2019, 04:38 -0700 schrieb syzbot: > Hello, > > syzbot found the following crash on: > > HEAD commit:d0847550 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=16295d4a60 > kernel config: https://syzkaller.appspot.com/x/.config?x=dbc9c80cc095da19 > dashboard link: https://syzkaller.appspot.com/bug?extid=55b0304b360654a7537b > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1288a31c60 > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=15282e8660 > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > Reported-by: syzbot+55b0304b360654a75...@syzkaller.appspotmail.com #syz test: https://github.com/google/kasan.git d0847550 >From df64f5cd2e6a9b43c75bb7e3276b8805a225db75 Mon Sep 17 00:00:00 2001 From: Oliver Neukum Date: Wed, 14 Aug 2019 15:21:41 +0200 Subject: [PATCH] usbtmc: more sanity checking for packet size A malicious device can make the driver divide ny zero with a nonsense maximum packet size. V2: return a sensible error code SIgned-off-by: Oliver Neukum --- drivers/usb/class/usbtmc.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index 4942122b2346..36858ddd8d9b 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -2362,8 +2362,11 @@ static int usbtmc_probe(struct usb_interface *intf, goto err_put; } + retcode = -EINVAL; data->bulk_in = bulk_in->bEndpointAddress; data->wMaxPacketSize = usb_endpoint_maxp(bulk_in); + if (!data->wMaxPacketSize) + goto err_put; dev_dbg(&intf->dev, "Found bulk in endpoint at %u\n", data->bulk_in); data->bulk_out = bulk_out->bEndpointAddress; -- 2.16.4
Re: divide error in usbtmc_generic_read
Am Mittwoch, den 14.08.2019, 06:38 -0700 schrieb syzbot:
> syzbot has tested the proposed patch but the reproducer still triggered
> crash:
> KASAN: use-after-free Read in usbtmc_disconnect
I am afraid that is a difficiency in KASAN that should be fixed.
Is the class of the error compared if I leave in more of the
original bug report? Actually the ID is still there, so it really
should return an inconclusive in these cases.
Regards
Oliver
Re: dwc2 / Raspberry Pi - hardware bug for small transfers results in memory corruption
Am Mittwoch, den 14.08.2019, 16:59 +0100 schrieb Jonathan Bell: > As reported by one of our users here: > https://github.com/raspberrypi/linux/issues/3148 > > There is a bug when the dwc2 core receives USB data packets that are > between 1 and 4 bytes in length - 4 bytes are always written to memory > where the non-packet bytes are garbage. Hi, in which function does that happen? If your buffer cannot handle 4 bytes I cannot see how it copes with teh DMA rules. Regards Oliver
Re: [PATCH] net: usbnet: fix a memory leak bug
Am Mittwoch, den 14.08.2019, 12:41 -0500 schrieb Wenwen Wang: > In usbnet_start_xmit(), 'urb->sg' is allocated through kmalloc_array() by > invoking build_dma_sg(). Later on, if 'CONFIG_PM' is defined and the if > branch is taken, the execution will go to the label 'deferred'. However, > 'urb->sg' is not deallocated on this execution path, leading to a memory > leak bug. Just to make this clear: > Signed-off-by: Wenwen Wang NACK For the reason Jack explained. Deferral is not a failure. Regards Oliver
Re: divide error in usbtmc_generic_read
Am Mittwoch, den 14.08.2019, 04:38 -0700 schrieb syzbot: > Hello, > > syzbot found the following crash on: > > HEAD commit:d0847550 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=16295d4a60 > kernel config: https://syzkaller.appspot.com/x/.config?x=dbc9c80cc095da19 > dashboard link: https://syzkaller.appspot.com/bug?extid=55b0304b360654a7537b > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1288a31c60 > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=15282e8660 > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > Reported-by: syzbot+55b0304b360654a75...@syzkaller.appspotmail.com #syz test: https://github.com/google/kasan.git d0847550 >From 5fa900e86e57d92a0b23a1a60ff4f4f13e997a93 Mon Sep 17 00:00:00 2001 From: Oliver Neukum Date: Wed, 14 Aug 2019 15:21:41 +0200 Subject: [PATCH] usbtmc: more sanity checking for packet size A malicious device can make the driver divide ny zero with a nonsense maximum packet size. SIgned-off-by: Oliver Neukum --- drivers/usb/class/usbtmc.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index 4942122b2346..11042ca56818 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -2364,6 +2364,8 @@ static int usbtmc_probe(struct usb_interface *intf, data->bulk_in = bulk_in->bEndpointAddress; data->wMaxPacketSize = usb_endpoint_maxp(bulk_in); + if (!data->wMaxPacketSize) + goto err_put; dev_dbg(&intf->dev, "Found bulk in endpoint at %u\n", data->bulk_in); data->bulk_out = bulk_out->bEndpointAddress; -- 2.16.4
Re: AW: Strange behaviour of D-Link DUB-1312 USB 3.0 Adapters
Am Mittwoch, den 14.08.2019, 08:56 + schrieb Schmid, Carsten : > > This is on a lower layer than ax88179. This comes from xhci_hcd. > > Is this a regression? > > > > I don't think its a regression. It would be better to know than to assume. > Is there something i can do to force an error message to be seen > when the ETH2USB adapter stalls? You can activate dynamic debugging for the xhci_hcd module Remember that no data to transfer is not an error as such. > My current assumption is that the signal quality of the USB port is at a > corner case, and therefore some "better" Adapters work, some "bad ones" > don't. But as there is no error message seen in the dmesg, i am somehow lost. Two things you can do: 1. Generate a usbmon trace (it will be gigantic though) 2. Activate dynamic debugging for the xhci_hcd module Regards Oliver
Re: KMSAN: uninit-value in smsc75xx_bind
Am Dienstag, den 13.08.2019, 17:08 +0200 schrieb Andrey Konovalov:
> On Tue, Aug 13, 2019 at 2:43 PM Oliver Neukum wrote:
> >
> >
> > 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?
>
> AFAICS, the uninitialized use of buf that KMSAN points out is in the
> "if (buf & PMT_CTL_DEV_RDY)" statement in smsc75xx_wait_ready(). Does
> __smsc75xx_read_reg/usb_control_msg() always initialize buf? Can it
> just initialize the first few bytes for example?
>
Hi,
you are unfortunately right and this is not the only driver vulnerable
in this way. I am going through them.
Regards
Oliver
Re: Strange behaviour of D-Link DUB-1312 USB 3.0 Adapters
Am Mittwoch, den 14.08.2019, 08:17 + schrieb Schmid, Carsten : > [Resend - had mailer errors ] > > Hi Florian, > > today i have seen a strange behaviour of two D-Link DUB-1312 adapters (same > Revision A1). > Plugging them into the same port (!) on my device one of them is recognized > as SuperSpeed, the other as high speed ??? > (working on 4.14.129 LTS) > > From dmesg, the "faulty" one: > [ 530.585871] usb 1-2: new high-speed USB device number 4 using xhci_hcd > <<<<<<<<< HUH XHCI is not like EHCI. It needs no companion controller, as it serves all speeds. > I had a look at the driver code of ax88179, but that one didn't change much > in the past up to v5.2. > Nothing that explains what i can see here. This is on a lower layer than ax88179. This comes from xhci_hcd. Is this a regression? Regards Oliver
Re: KASAN: use-after-free Read in device_release_driver_internal
Am Dienstag, den 13.08.2019, 14:42 +0200 schrieb Andrey Konovalov:
> >
[..]
> On Thu, Aug 8, 2019 at 4:00 PM Alan Stern wrote:
> > Ah, that looks right, thank you. The patch worked correctly -- good
> > work Oliver!
>
> Great! Just a reminder to submit the fix :)
I did last week:
https://patchwork.kernel.org/patch/11084261/
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
[PATCH] USB: CDC: fix sanity checks in CDC union parser
A few checks checked for the size of the pointer to a structure
instead of the structure itself. Copy & paste issue presumably.
Fixes: e4c6fb7794982 ("usbnet: move the CDC parser into USB core")
Reported-by: syzbot+45a53506b65321c1f...@syzkaller.appspotmail.com
Signed-off-by: Oliver Neukum
---
drivers/usb/core/message.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index e844bb7b5676..5adf489428aa 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -2218,14 +2218,14 @@ int cdc_parse_cdc_header(struct usb_cdc_parsed_header
*hdr,
(struct usb_cdc_dmm_desc *)buffer;
break;
case USB_CDC_MDLM_TYPE:
- if (elength < sizeof(struct usb_cdc_mdlm_desc *))
+ if (elength < sizeof(struct usb_cdc_mdlm_desc))
goto next_desc;
if (desc)
return -EINVAL;
desc = (struct usb_cdc_mdlm_desc *)buffer;
break;
case USB_CDC_MDLM_DETAIL_TYPE:
- if (elength < sizeof(struct usb_cdc_mdlm_detail_desc *))
+ if (elength < sizeof(struct usb_cdc_mdlm_detail_desc))
goto next_desc;
if (detail)
return -EINVAL;
--
2.16.4
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: slab-out-of-bounds Read in hidraw_ioctl
Am Sonntag, den 11.08.2019, 13:46 -0700 schrieb syzbot:
> syzbot has found a reproducer for the following crash on:
>
> HEAD commit:e96407b4 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=150426ba60
> kernel config: https://syzkaller.appspot.com/x/.config?x=cfa2c18fb6a8068e
> dashboard link: https://syzkaller.appspot.com/bug?extid=5a6c4ec678a0c6ee84ba
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=12725c0260
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=162163c260
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+5a6c4ec678a0c6ee8...@syzkaller.appspotmail.com
#syz test: https://github.com/google/kasan.git e96407b4
>From 700a7cc270f06c6e9881f49e36a7722d16ee37db Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Thu, 8 Aug 2019 15:08:48 +0200
Subject: [PATCH] HID: use strnlen to not walk through kernel memory
If a device sets no phy name we must limit the range
for looking for the end of the string lest we touch
areas of memory we should not touch.
Reported-by: syzbot+5a6c4ec678a0c6ee8...@syzkaller.appspotmail.com
Signed-off-by: Oliver Neukum
---
drivers/hid/hidraw.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
index 006bd6f4f653..c7dcff6222b5 100644
--- a/drivers/hid/hidraw.c
+++ b/drivers/hid/hidraw.c
@@ -443,9 +443,7 @@ static long hidraw_ioctl(struct file *file, unsigned int
cmd,
}
if (_IOC_NR(cmd) == _IOC_NR(HIDIOCGRAWPHYS(0)))
{
- int len = strlen(hid->phys) + 1;
- if (len > _IOC_SIZE(cmd))
- len = _IOC_SIZE(cmd);
+ int len = strnlen(hid->phys,
_IOC_SIZE(cmd)) + 1;
ret = copy_to_user(user_arg, hid->phys,
len) ?
-EFAULT : len;
break;
--
2.16.4
Re: [PATCH] xhci: wait CNR when doing xhci resume
Am Montag, den 12.08.2019, 15:24 +0800 schrieb Rick Tseng:
> From: Rick
>
> NVIDIA 3.1 xHCI card would lose power when moving power state into D3Cold.
> Thus we need to wait CNR bit to clear when xhci resmue as xhci init.
Should any controller have CNR set? Why is this specific to a vendor?
Regards
Oliver
Re: KASAN: use-after-free Read in usbhid_power
Am Donnerstag, den 08.08.2019, 20:54 +0200 schrieb Andrey Konovalov:
> On Thu, Jul 25, 2019 at 5:09 PM Alan Stern wrote:
> >
> > On Thu, 25 Jul 2019, Oliver Neukum wrote:
> >
> > > Am Mittwoch, den 24.07.2019, 17:02 -0400 schrieb Alan Stern:
> > > > On Wed, 24 Jul 2019, Oliver Neukum wrote:
> > > >
> > > > > drivers/hid/usbhid/hid-core.c | 13 +
> > > > > 1 file changed, 13 insertions(+)
> > > > >
> > > > > diff --git a/drivers/hid/usbhid/hid-core.c
> > > > > b/drivers/hid/usbhid/hid-core.c
> > > > > index c7bc9db5b192..98b996ecf4d3 100644
> > > > > --- a/drivers/hid/usbhid/hid-core.c
> > > > > +++ b/drivers/hid/usbhid/hid-core.c
> > > > > @@ -1229,6 +1229,17 @@ static int usbhid_power(struct hid_device
> > > > > *hid, int lvl)
> > > > > struct usbhid_device *usbhid = hid->driver_data;
> > > > > int r = 0;
> > > > >
> > > > > + spin_lock_irq(&usbhid->lock);
> > > > > + if (test_bit(HID_DISCONNECTED, &usbhid->iofl)) {
> > > > > + r = -ENODEV;
> > > > > + spin_unlock_irq(&usbhid->lock);
> > > > > + goto bail_out;
> > > > > + } else {
> > > > > + /* protect against disconnect */
> > > > > + usb_get_dev(interface_to_usbdev(usbhid->intf));
> > > > > + spin_unlock_irq(&usbhid->lock);
> > > > > + }
> > > > > +
> > > > > switch (lvl) {
> > > > > case PM_HINT_FULLON:
> > > > > r = usb_autopm_get_interface(usbhid->intf);
> > > > > @@ -1238,7 +1249,9 @@ static int usbhid_power(struct hid_device *hid,
> > > > > int lvl)
> > > > > usb_autopm_put_interface(usbhid->intf);
> > > > > break;
> > > > > }
> > > > > + usb_put_dev(interface_to_usbdev(usbhid->intf));
> > > > >
> > > > > +bail_out:
> > > > > return r;
> > > > > }
> > > >
> > > > Isn't this treating the symptom instead of the cause?
> > >
> > > Sort of. Holding a reference for the whole time would have merit,
> > > but I doubt it is strictly necessary.
> >
> > Just to be crystal clear, I was talking about a device reference --
> > usb_{get,put}_dev or usb_{get,put}_intf -- not a runtime PM reference.
> >
> > (Incidentally, your patch could be simplified by using usb_get_intf
> > instead of usb_get_dev.)
> >
> > > > Shouldn't the hid_device hold a reference to usbhid->intf throughout
> > > > its lifetime? That way this sort of problem wouldn't arise in any
> > > > routine, not just usbhid_power().
> > >
> > > Unfortunately the semantics would still be wrong without the check
> > > in corner cases. In case disconnect() is called without a physical
> > > unplug, we must not touch the power state.
> > > I am indeed afraid that in that case my putative fix is still racy.
> > > But I don't to just introduce a mutex just for this. Any ideas?
> >
> > That's a separate issue. USB drivers -- indeed, all drivers -- are
> > required to balance their runtime PM gets and puts (although in the
> > case of a physical disconnection it doesn't matter). Are you asking
> > about the best way to do this?
> >
> > Normally a driver's release or disconnect routine will stop all
> > asynchronous accesses to the device (interrupt handlers, work queues,
> > URBs, and so on). At that point the only remaining runtime PM activity
> > will be whatever the routine itself does. So it can see if any extra
> > runtime PM gets or puts are needed, and do whatever is necessary.
> >
> > Does that answer your question? I can't tell for sure...
> >
> > Note: I did not try to track down the reason for the invalid access
> > reported by syzbot. It looked like a simple use-after-free, which
> > would normally be fixed by taking the appropriate reference. Which is
> > what your patch does, except that it holds the reference only for a
> > short time instead of over the entire lifetime of the private data
> > structure (the usbhid structure), which is what normally happens.
>
> This report looks like very similar to these two:
>
> https://syzkaller.appspot.com/bug?extid=b156665cf4d1b5e00c76
> https://syzkaller.appspot.com/bug?extid=3cbe5cd105d2ad56a1df
Yes, they stem from the same root cause most likely.
> Maybe we should mark those two as duplicates.
>
> Hillf suggested a fix on one of them, but it looks different from what
> you propose:
>
> https://groups.google.com/d/msg/syzkaller-bugs/xW7LvKfpyn0/SpKbs5ZLEAAJ
The fix may indeed be necessary, but it is incomplete. It does
not help keeping the PM counters consistent.
Regards
Oliver
[PATCH] usb: cdc-acm: make sure a refcount is taken early enough
destroy() will decrement the refcount on the interface, so that
it needs to be taken so early that it never undercounts.
Fixes: 7fb57a019f94e ("USB: cdc-acm: Fix potential deadlock (lockdep warning)")
Reported-and-tested-by: syzbot+1b2449b7b5dc240d1...@syzkaller.appspotmail.com
Signed-off-by: Oliver Neukum
---
drivers/usb/class/cdc-acm.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 183b41753c98..62f4fb9b362f 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -1301,10 +1301,6 @@ static int acm_probe(struct usb_interface *intf,
tty_port_init(&acm->port);
acm->port.ops = &acm_port_ops;
- minor = acm_alloc_minor(acm);
- if (minor < 0)
- goto alloc_fail1;
-
ctrlsize = usb_endpoint_maxp(epctrl);
readsize = usb_endpoint_maxp(epread) *
(quirks == SINGLE_RX_URB ? 1 : 2);
@@ -1312,6 +1308,13 @@ static int acm_probe(struct usb_interface *intf,
acm->writesize = usb_endpoint_maxp(epwrite) * 20;
acm->control = control_interface;
acm->data = data_interface;
+
+ usb_get_intf(acm->control); /* undone in destruct() */
+
+ minor = acm_alloc_minor(acm);
+ if (minor < 0)
+ goto alloc_fail1;
+
acm->minor = minor;
acm->dev = usb_dev;
if (h.usb_cdc_acm_descriptor)
@@ -1458,7 +1461,6 @@ static int acm_probe(struct usb_interface *intf,
usb_driver_claim_interface(&acm_driver, data_interface, acm);
usb_set_intfdata(data_interface, acm);
- usb_get_intf(control_interface);
tty_dev = tty_port_register_device(&acm->port, acm_tty_driver, minor,
&control_interface->dev);
if (IS_ERR(tty_dev)) {
--
2.16.4
Re: usb zero copy dma handling
Am Donnerstag, den 08.08.2019, 10:59 +0100 schrieb Russell King - ARM
Linux admin:
> On Thu, Aug 08, 2019 at 10:58:11AM +0200, Greg KH wrote:
> > But the main issue here is what exactly is this "fixing"? What is wrong
> > with the existing code that non-x86 systems have such a problem with?
> > Shouldn't all of these dma issues be handled by the platform with the
> > remap_pfn_range() call itself?
>
> remap_pfn_range() takes a PFN. virt_to_phys() converts a kernel *direct
> mapped* virtual address to a physical address. That much is fine.
>
> The question is - what is usbm->mem? If that is anything other than an
> address returned by kmalloc() or from the normal page allocator, then
> virt_to_phys() will return garbage.
>
> In other words, if it comes from dma_alloc_coherent(), vmalloc() or
> ioremap(), using virt_to_phys() on it results in garbage.
It comes from usb_alloc_coherent() -> hcd_buffer_alloc() ->
hcd_buffer_alloc()
That function is a bit complicated. so I rather quote than explain:
if (hcd->localmem_pool)
return gen_pool_dma_alloc(hcd->localmem_pool, size, dma)
/* some USB hosts just use PIO */
if (!IS_ENABLED(CONFIG_HAS_DMA) ||
!is_device_dma_capable(bus->sysdev)) {
*dma = ~(dma_addr_t) 0;
return kmalloc(size, mem_flags);
}
for (i = 0; i < HCD_BUFFER_POOLS; i++) {
if (size <= pool_max[i])
return dma_pool_alloc(hcd->pool[i], mem_flags, dma);
}
return dma_alloc_coherent(hcd->self.sysdev, size, dma, mem_flags);
Regards
Oliver
[PATCH] Revert "USB: rio500: simplify locking"
This reverts commit d710734b06770814de2bfa2819420fb5df7f3a81.
This simplification causes a deadlock.
Reported-by: syzbot+7bbcbe9c9ff0cd495...@syzkaller.appspotmail.com
Signed-off-by: Oliver Neukum
---
drivers/usb/misc/rio500.c | 43 +++
1 file changed, 27 insertions(+), 16 deletions(-)
diff --git a/drivers/usb/misc/rio500.c b/drivers/usb/misc/rio500.c
index 27e9c78a791e..a32d61a79ab8 100644
--- a/drivers/usb/misc/rio500.c
+++ b/drivers/usb/misc/rio500.c
@@ -51,6 +51,7 @@ struct rio_usb_data {
char *obuf, *ibuf; /* transfer buffers */
char bulk_in_ep, bulk_out_ep; /* Endpoint assignments */
wait_queue_head_t wait_q; /* for timeouts */
+ struct mutex lock; /* general race avoidance */
};
static DEFINE_MUTEX(rio500_mutex);
@@ -62,8 +63,10 @@ static int open_rio(struct inode *inode, struct file *file)
/* against disconnect() */
mutex_lock(&rio500_mutex);
+ mutex_lock(&(rio->lock));
if (rio->isopen || !rio->present) {
+ mutex_unlock(&(rio->lock));
mutex_unlock(&rio500_mutex);
return -EBUSY;
}
@@ -71,6 +74,7 @@ static int open_rio(struct inode *inode, struct file *file)
init_waitqueue_head(&rio->wait_q);
+ mutex_unlock(&(rio->lock));
dev_info(&rio->rio_dev->dev, "Rio opened.\n");
mutex_unlock(&rio500_mutex);
@@ -84,6 +88,7 @@ static int close_rio(struct inode *inode, struct file *file)
/* against disconnect() */
mutex_lock(&rio500_mutex);
+ mutex_lock(&(rio->lock));
rio->isopen = 0;
if (!rio->present) {
@@ -95,6 +100,7 @@ static int close_rio(struct inode *inode, struct file *file)
} else {
dev_info(&rio->rio_dev->dev, "Rio closed.\n");
}
+ mutex_unlock(&(rio->lock));
mutex_unlock(&rio500_mutex);
return 0;
}
@@ -109,7 +115,7 @@ static long ioctl_rio(struct file *file, unsigned int cmd,
unsigned long arg)
int retries;
int retval=0;
- mutex_lock(&rio500_mutex);
+ mutex_lock(&(rio->lock));
/* Sanity check to make sure rio is connected, powered, etc */
if (rio->present == 0 || rio->rio_dev == NULL) {
retval = -ENODEV;
@@ -253,7 +259,7 @@ static long ioctl_rio(struct file *file, unsigned int cmd,
unsigned long arg)
err_out:
- mutex_unlock(&rio500_mutex);
+ mutex_unlock(&(rio->lock));
return retval;
}
@@ -273,12 +279,12 @@ write_rio(struct file *file, const char __user *buffer,
int errn = 0;
int intr;
- intr = mutex_lock_interruptible(&rio500_mutex);
+ intr = mutex_lock_interruptible(&(rio->lock));
if (intr)
return -EINTR;
/* Sanity check to make sure rio is connected, powered, etc */
if (rio->present == 0 || rio->rio_dev == NULL) {
- mutex_unlock(&rio500_mutex);
+ mutex_unlock(&(rio->lock));
return -ENODEV;
}
@@ -301,7 +307,7 @@ write_rio(struct file *file, const char __user *buffer,
goto error;
}
if (signal_pending(current)) {
- mutex_unlock(&rio500_mutex);
+ mutex_unlock(&(rio->lock));
return bytes_written ? bytes_written : -EINTR;
}
@@ -339,12 +345,12 @@ write_rio(struct file *file, const char __user *buffer,
buffer += copy_size;
} while (count > 0);
- mutex_unlock(&rio500_mutex);
+ mutex_unlock(&(rio->lock));
return bytes_written ? bytes_written : -EIO;
error:
- mutex_unlock(&rio500_mutex);
+ mutex_unlock(&(rio->lock));
return errn;
}
@@ -361,12 +367,12 @@ read_rio(struct file *file, char __user *buffer, size_t
count, loff_t * ppos)
char *ibuf;
int intr;
- intr = mutex_lock_interruptible(&rio500_mutex);
+ intr = mutex_lock_interruptible(&(rio->lock));
if (intr)
return -EINTR;
/* Sanity check to make sure rio is connected, powered, etc */
if (rio->present == 0 || rio->rio_dev == NULL) {
- mutex_unlock(&rio500_mutex);
+ mutex_unlock(&(rio->lock));
return -ENODEV;
}
@@ -377,11 +383,11 @@ read_rio(struct file *file, char __user *buffer, size_t
count, loff_t * ppos)
while (count > 0) {
if (signal_pending(current)) {
- mutex_unlock(&rio500_mutex);
+ mutex_un
[PATCH] usb: iowarrior: fix deadlock on disconnect
We have to drop the mutex before we close() upon disconnect()
as close() needs the lock. This is safe to do by dropping the
mutex as intfdata is already set to NULL, so open() will fail.
Fixes: 03f36e885fc26 ("USB: open disconnect race in iowarrior")
Reported-by: syzbot+a64a382964bf6c71a...@syzkaller.appspotmail.com
Signed-off-by: Oliver Neukum
---
drivers/usb/misc/iowarrior.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/misc/iowarrior.c b/drivers/usb/misc/iowarrior.c
index ba05dd80a020..f5bed9f29e56 100644
--- a/drivers/usb/misc/iowarrior.c
+++ b/drivers/usb/misc/iowarrior.c
@@ -866,19 +866,20 @@ static void iowarrior_disconnect(struct usb_interface
*interface)
dev = usb_get_intfdata(interface);
mutex_lock(&iowarrior_open_disc_lock);
usb_set_intfdata(interface, NULL);
+ /* prevent device read, write and ioctl */
+ dev->present = 0;
minor = dev->minor;
+ mutex_unlock(&iowarrior_open_disc_lock);
+ /* give back our minor - this will call close() locks need to be
dropped at this point*/
- /* give back our minor */
usb_deregister_dev(interface, &iowarrior_class);
mutex_lock(&dev->mutex);
/* prevent device read, write and ioctl */
- dev->present = 0;
mutex_unlock(&dev->mutex);
- mutex_unlock(&iowarrior_open_disc_lock);
if (dev->opened) {
/* There is a process that holds a filedescriptor to the device
,
--
2.16.4
Re: WARNING in zd_mac_clear
Am Mittwoch, den 07.08.2019, 16:07 +0200 schrieb Andrey Konovalov: > On Fri, Apr 12, 2019 at 1:46 PM syzbot > wrote: > > > > Hello, > > > > syzbot found the following crash on: > > > > HEAD commit:9a33b369 usb-fuzzer: main usb gadget fuzzer driver > > git tree: https://github.com/google/kasan/tree/usb-fuzzer > > console output: https://syzkaller.appspot.com/x/log.txt?x=101a06dd20 > > kernel config: https://syzkaller.appspot.com/x/.config?x=23e37f59d94ddd15 > > dashboard link: https://syzkaller.appspot.com/bug?extid=74c65761783d66a9c97c > > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1170c22d20 > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1496adbb20 > > > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > > Reported-by: syzbot+74c65761783d66a9c...@syzkaller.appspotmail.com > > > > usb 1-1: New USB device strings: Mfr=0, Product=0, SerialNumber=0 > > usb 1-1: config 0 descriptor?? > > usb 1-1: reset low-speed USB device number 2 using dummy_hcd > > usb 1-1: read over firmware interface failed: -71 > > usb 1-1: reset low-speed USB device number 2 using dummy_hcd > > WARNING: CPU: 1 PID: 21 at drivers/net/wireless/zydas/zd1211rw/zd_mac.c:238 > > zd_mac_clear+0xb0/0xe0 drivers/net/wireless/zydas/zd1211rw/zd_mac.c:238 > > Kernel panic - not syncing: panic_on_warn set ... > > CPU: 1 PID: 21 Comm: kworker/1:1 Not tainted 5.1.0-rc4-319354-g9a33b36 #3 > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS > > Google 01/01/2011 > > Workqueue: usb_hub_wq hub_event > > Call Trace: > > __dump_stack lib/dump_stack.c:77 [inline] > > dump_stack+0xe8/0x16e lib/dump_stack.c:113 > > panic+0x29d/0x5f2 kernel/panic.c:214 > > __warn.cold+0x20/0x48 kernel/panic.c:571 > > report_bug+0x262/0x2a0 lib/bug.c:186 > > fixup_bug arch/x86/kernel/traps.c:179 [inline] > > fixup_bug arch/x86/kernel/traps.c:174 [inline] > > do_error_trap+0x130/0x1f0 arch/x86/kernel/traps.c:272 > > do_invalid_op+0x37/0x40 arch/x86/kernel/traps.c:291 > > invalid_op+0x14/0x20 arch/x86/entry/entry_64.S:973 > > RIP: 0010:zd_mac_clear+0xb0/0xe0 > > drivers/net/wireless/zydas/zd1211rw/zd_mac.c:238 > > Code: e8 85 d0 60 f8 48 8d bb f8 2b 00 00 be ff ff ff ff e8 54 5a 46 f8 31 > > ff 89 c3 89 c6 e8 d9 d1 60 f8 85 db 75 d4 e8 60 d0 60 f8 <0f> 0b 5b 5d e9 > > 57 d0 60 f8 48 c7 c7 58 05 cb 93 e8 fb e0 97 f8 eb > > RSP: 0018:8880a85c7310 EFLAGS: 00010293 > > RAX: 8880a84de200 RBX: RCX: 8910f507 > > RDX: RSI: 8910f510 RDI: 0005 > > RBP: 0001 R08: 8880a84de200 R09: ed1012f83a0b > > R10: ed1012f83a0a R11: 888097c1d057 R12: ffb9 > > R13: 888097c18b20 R14: 888099456630 R15: 8f979398 > > probe+0x259/0x590 drivers/net/wireless/zydas/zd1211rw/zd_usb.c:1421 > > usb_probe_interface+0x31d/0x820 drivers/usb/core/driver.c:361 > > really_probe+0x2da/0xb10 drivers/base/dd.c:509 > > driver_probe_device+0x21d/0x350 drivers/base/dd.c:671 > > __device_attach_driver+0x1d8/0x290 drivers/base/dd.c:778 > > bus_for_each_drv+0x163/0x1e0 drivers/base/bus.c:454 > > __device_attach+0x223/0x3a0 drivers/base/dd.c:844 > > bus_probe_device+0x1f1/0x2a0 drivers/base/bus.c:514 > > device_add+0xad2/0x16e0 drivers/base/core.c:2106 > > usb_set_configuration+0xdf7/0x1740 drivers/usb/core/message.c:2021 > > generic_probe+0xa2/0xda drivers/usb/core/generic.c:210 > > usb_probe_device+0xc0/0x150 drivers/usb/core/driver.c:266 > > really_probe+0x2da/0xb10 drivers/base/dd.c:509 > > driver_probe_device+0x21d/0x350 drivers/base/dd.c:671 > > __device_attach_driver+0x1d8/0x290 drivers/base/dd.c:778 > > bus_for_each_drv+0x163/0x1e0 drivers/base/bus.c:454 > > __device_attach+0x223/0x3a0 drivers/base/dd.c:844 > > bus_probe_device+0x1f1/0x2a0 drivers/base/bus.c:514 > > device_add+0xad2/0x16e0 drivers/base/core.c:2106 > > usb_new_device.cold+0x537/0xccf drivers/usb/core/hub.c:2534 > > hub_port_connect drivers/usb/core/hub.c:5089 [inline] > > hub_port_connect_change drivers/usb/core/hub.c:5204 [inline] > > port_event drivers/usb/core/hub.c:5350 [inline] > > hub_event+0x138e/0x3b00 drivers/usb/core/hub.c:5432 > > process_one_work+0x90f/0x1580 kernel/workqueue.c:2269 > > worker_thread+0x9b/0xe20 kernel/workqueue.c:2415 > > kthread+0x313/0x420 kernel/kthread.c:253 > > ret_from_fork
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: possible deadlock in open_rio
Am Dienstag, den 06.08.2019, 15:13 -0400 schrieb Alan Stern: > On Thu, 1 Aug 2019, syzbot wrote: > > > Hello, > > > > syzbot found the following crash on: > > > > HEAD commit:7f7867ff 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=136b6aec60 > > kernel config: https://syzkaller.appspot.com/x/.config?x=792eb47789f57810 > > dashboard link: https://syzkaller.appspot.com/bug?extid=7bbcbe9c9ff0cd49592a > > 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+7bbcbe9c9ff0cd495...@syzkaller.appspotmail.com > > > > == > > WARNING: possible circular locking dependency detected > > 5.3.0-rc2+ #23 Not tainted > > -- > > Andrey: > > This should be completely reproducible, since it's a simple ABBA > locking violation. Maybe just introducing a time delay (to avoid races > and give the open() call time to run) between the gadget creation and > gadget removal would be enough to do it. Hi, technically yes. However in practical terms the straight revert I sent out yesterday should fix it. Regards Oliver
Re: [PATCH] usbfs: Add ioctls for runtime power management
Am Dienstag, den 06.08.2019, 15:46 -0400 schrieb Alan Stern:
> +static int proc_wait_for_resume(struct usb_dev_state *ps)
> +{
> + int ret;
> +
> + usb_unlock_device(ps->dev);
> + ret = wait_event_interruptible(ps->wait_for_resume,
> + READ_ONCE(ps->not_yet_resumed) == 0);
> + usb_lock_device(ps->dev);
> +
> + if (ret != 0)
> + return ret;
> + return proc_forbid_suspend(ps);
One nitpick, this seems to return raw -ERESTARTSYS in the interrupt
case. Should it?
Regards
Oliver
Re: KASAN: use-after-free Read in device_release_driver_internal
Am Dienstag, den 06.08.2019, 10:19 -0400 schrieb Alan Stern:
> In any case, I don't know if this missing "get" would cause the
> problem, but it might well.
Hi,
upon further thought, this should be automated. Checking for
refcount leaks is KASAN's job. In particular, refcounts
should not
* decrease in probe()
* increase in disconnect()
* change in case probe() fails
Regards
Oliver
Re: KASAN: use-after-free Read in device_release_driver_internal
Am Dienstag, den 06.08.2019, 10:19 -0400 schrieb Alan Stern:
> On Tue, 6 Aug 2019, 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.
>
> Only one interface (numbered 234!).
Yes. cdc-acm went into the look_for_collapsed_interface code path.
But I cannot tell whether it proceeded to made_compressed_probe
(Yes, I know the code makes extensive use of "goto")
> > If you agree. I am attaching a putative fix.
>
> Your patch adds a line saying:
>
> > + usb_get_intf(acm->control); /* undone in destroy() */
>
> but I don't see any destroy() function in that source file. Did you
> mean acm_port_destruct()?
Yes, sorry
> In any case, I don't know if this missing "get" would cause the
> problem, but it might well.
Then let's wait for the result.
Regards
Oliver
Re: possible deadlock in usb_deregister_dev
Am Montag, den 05.08.2019, 04:58 -0700 schrieb syzbot:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit:e96407b4 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=13b5bc8a60
> kernel config: https://syzkaller.appspot.com/x/.config?x=792eb47789f57810
> dashboard link: https://syzkaller.appspot.com/bug?extid=a64a382964bf6c71a9c0
> 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+a64a382964bf6c71a...@syzkaller.appspotmail.com
#syz test: https://github.com/google/kasan.git e96407b4
>From 973e82b3f583113e4d7fe5cd2f918a16022c4e38 Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Tue, 6 Aug 2019 16:17:54 +0200
Subject: [PATCH] usb: iowarrior: fix deadlock on disconnect
We have to drop the mutex before we close() upon disconnect()
as close() needs the lock. This is safe to do by dropping the
mutex as intfdata is already set to NULL, so open() will fail.
Fixes: 03f36e885fc26 ("USB: open disconnect race in iowarrior")
Reported-by: syzbot+a64a382964bf6c71a...@syzkaller.appspotmail.com
Signed-off-by: Oliver Neukum
---
drivers/usb/misc/iowarrior.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/misc/iowarrior.c b/drivers/usb/misc/iowarrior.c
index ba05dd80a020..f5bed9f29e56 100644
--- a/drivers/usb/misc/iowarrior.c
+++ b/drivers/usb/misc/iowarrior.c
@@ -866,19 +866,20 @@ static void iowarrior_disconnect(struct usb_interface
*interface)
dev = usb_get_intfdata(interface);
mutex_lock(&iowarrior_open_disc_lock);
usb_set_intfdata(interface, NULL);
+ /* prevent device read, write and ioctl */
+ dev->present = 0;
minor = dev->minor;
+ mutex_unlock(&iowarrior_open_disc_lock);
+ /* give back our minor - this will call close() locks need to be
dropped at this point*/
- /* give back our minor */
usb_deregister_dev(interface, &iowarrior_class);
mutex_lock(&dev->mutex);
/* prevent device read, write and ioctl */
- dev->present = 0;
mutex_unlock(&dev->mutex);
- mutex_unlock(&iowarrior_open_disc_lock);
if (dev->opened) {
/* There is a process that holds a filedescriptor to the device
,
--
2.16.4
Re: WARNING in __iforce_usb_xmit/usb_submit_urb
Am Montag, den 05.08.2019, 04:58 -0700 schrieb syzbot: > Hello, > > syzbot found the following crash on: > > HEAD commit:e96407b4 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=10809e0c60 > kernel config: https://syzkaller.appspot.com/x/.config?x=cfa2c18fb6a8068e > dashboard link: https://syzkaller.appspot.com/bug?extid=5efc10c005014d061a74 > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=15e40b1a60 > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=174a69d860 > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > Reported-by: syzbot+5efc10c005014d061...@syzkaller.appspotmail.com #syz test: https://github.com/google/kasan.git e96407b4 >From 06be579ae09483c7c723067f4e5bf938ad302bda Mon Sep 17 00:00:00 2001 From: Oliver Neukum Date: Tue, 6 Aug 2019 15:33:35 +0200 Subject: [PATCH] iforce: add sanity checks The endpoint type should also be checked before a device is accepted. Reported-by: syzbot+5efc10c005014d061...@syzkaller.appspotmail.com Signed-off-by: Oliver Neukum --- drivers/input/joystick/iforce/iforce-usb.c | 5 + 1 file changed, 5 insertions(+) diff --git a/drivers/input/joystick/iforce/iforce-usb.c b/drivers/input/joystick/iforce/iforce-usb.c index 29abfeeef9a5..a481a226166c 100644 --- a/drivers/input/joystick/iforce/iforce-usb.c +++ b/drivers/input/joystick/iforce/iforce-usb.c @@ -203,6 +203,11 @@ static int iforce_usb_probe(struct usb_interface *intf, epirq = &interface->endpoint[0].desc; epout = &interface->endpoint[1].desc; + if (!usb_endpoint_is_int_in(epirq)) + return -ENODEV; + if (!usb_endpoint_is_int_out(epout)) + return -ENODEV; + iforce_usb = kzalloc(sizeof(*iforce_usb), GFP_KERNEL); if (!iforce_usb) goto fail; -- 2.16.4
Re: KMSAN: kernel-usb-infoleak in pcan_usb_pro_send_req
Am Dienstag, den 30.07.2019, 02:38 -0700 schrieb syzbot: > Hello, > > syzbot found the following crash on: > > HEAD commit:41550654 [UPSTREAM] KVM: x86: degrade WARN to pr_warn_rate.. > git tree: kmsan > console output: https://syzkaller.appspot.com/x/log.txt?x=13e95183a0 > kernel config: https://syzkaller.appspot.com/x/.config?x=40511ad0c5945201 > dashboard link: https://syzkaller.appspot.com/bug?extid=513e4d0985298538bf9b > compiler: clang version 9.0.0 (/home/glider/llvm/clang > 80fee25776c2fb61e74c1ecb1a523375c2500b69) > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=17eafa1ba0 > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=17b87983a0 > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > Reported-by: syzbot+513e4d0985298538b...@syzkaller.appspotmail.com #syz test: https://github.com/google/kmsan.git 41550654 >From 6de76fa3df8aedc7a76dc0ecdea8308e38d4dccc Mon Sep 17 00:00:00 2001 From: Oliver Neukum Date: Tue, 6 Aug 2019 14:41:52 +0200 Subject: [PATCH] pcan_usb_fd: zero out the common command buffer Lest we leak kernel memory to a device we better zero out buffers. Signed-off-by: Oliver Neukum --- drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c index 34761c3a6286..47cc1ff5b88e 100644 --- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c @@ -841,7 +841,7 @@ static int pcan_usb_fd_init(struct peak_usb_device *dev) goto err_out; /* allocate command buffer once for all for the interface */ - pdev->cmd_buffer_addr = kmalloc(PCAN_UFD_CMD_BUFFER_SIZE, + pdev->cmd_buffer_addr = kzalloc(PCAN_UFD_CMD_BUFFER_SIZE, GFP_KERNEL); if (!pdev->cmd_buffer_addr) goto err_out_1;
Re: KMSAN: kernel-usb-infoleak in pcan_usb_pro_send_req
Am Dienstag, den 30.07.2019, 02:38 -0700 schrieb syzbot: > Hello, > > syzbot found the following crash on: > > HEAD commit:41550654 [UPSTREAM] KVM: x86: degrade WARN to pr_warn_rate.. > git tree: kmsan > console output: https://syzkaller.appspot.com/x/log.txt?x=13e95183a0 > kernel config: https://syzkaller.appspot.com/x/.config?x=40511ad0c5945201 > dashboard link: https://syzkaller.appspot.com/bug?extid=513e4d0985298538bf9b > compiler: clang version 9.0.0 (/home/glider/llvm/clang > 80fee25776c2fb61e74c1ecb1a523375c2500b69) > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=17eafa1ba0 > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=17b87983a0 > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > Reported-by: syzbot+513e4d0985298538b...@syzkaller.appspotmail.com #syz test: https://github.com/google/kasan.git 41550654 >From 6de76fa3df8aedc7a76dc0ecdea8308e38d4dccc Mon Sep 17 00:00:00 2001 From: Oliver Neukum Date: Tue, 6 Aug 2019 14:41:52 +0200 Subject: [PATCH] pcan_usb_fd: zero out the common command buffer Lest we leak kernel memory to a device we better zero out buffers. Signed-off-by: Oliver Neukum --- drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c index 34761c3a6286..47cc1ff5b88e 100644 --- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c @@ -841,7 +841,7 @@ static int pcan_usb_fd_init(struct peak_usb_device *dev) goto err_out; /* allocate command buffer once for all for the interface */ - pdev->cmd_buffer_addr = kmalloc(PCAN_UFD_CMD_BUFFER_SIZE, + pdev->cmd_buffer_addr = kzalloc(PCAN_UFD_CMD_BUFFER_SIZE, GFP_KERNEL); if (!pdev->cmd_buffer_addr) goto err_out_1; -- 2.16.4
Re: KASAN: use-after-free Read in device_release_driver_internal
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.
Regards
Oliver
From 6b31904e6cf75f89441e308b9e428a1de7728fd8 Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Tue, 6 Aug 2019 14:34:27 +0200
Subject: [PATCH] usb: cdc-acm: make sure a refcount is taken early enough
destroy() will decrement the refcount on the interface, so that
it needs to be taken so early that it never undercounts.
Signed-off-by: Oliver Neukum
---
drivers/usb/class/cdc-acm.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 183b41753c98..28e3de775ada 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -1301,10 +1301,6 @@ static int acm_probe(struct usb_interface *intf,
tty_port_init(&acm->port);
acm->port.ops = &acm_port_ops;
- minor = acm_alloc_minor(acm);
- if (minor < 0)
- goto alloc_fail1;
-
ctrlsize = usb_endpoint_maxp(epctrl);
readsize = usb_endpoint_maxp(epread) *
(quirks == SINGLE_RX_URB ? 1 : 2);
@@ -1312,6 +1308,13 @@ static int acm_probe(struct usb_interface *intf,
acm->writesize = usb_endpoint_maxp(epwrite) * 20;
acm->control = control_interface;
acm->data = data_interface;
+
+ usb_get_intf(acm->control); /* undone in destroy() */
+
+ minor = acm_alloc_minor(acm);
+ if (minor < 0)
+ goto alloc_fail1;
+
acm->minor = minor;
acm->dev = usb_dev;
if (h.usb_cdc_acm_descriptor)
@@ -1458,7 +1461,6 @@ static int acm_probe(struct usb_interface *intf,
usb_driver_claim_interface(&acm_driver, data_interface, acm);
usb_set_intfdata(data_interface, acm);
- usb_get_intf(control_interface);
tty_dev = tty_port_register_device(&acm->port, acm_tty_driver, minor,
&control_interface->dev);
if (IS_ERR(tty_dev)) {
--
2.16.4
Re: KASAN: slab-out-of-bounds Write in lg4ff_init
Am Montag, den 05.08.2019, 05:38 -0700 schrieb syzbot: > Hello, > > syzbot found the following crash on: > > HEAD commit:e96407b4 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=144c21dc60 > kernel config: https://syzkaller.appspot.com/x/.config?x=cfa2c18fb6a8068e > dashboard link: https://syzkaller.appspot.com/bug?extid=94e2b9e9c7d1dd332345 > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=169e854260 > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=10ec826260 > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > Reported-by: syzbot+94e2b9e9c7d1dd332...@syzkaller.appspotmail.com > > usb 1-1: config 0 interface 0 altsetting 0 has 1 endpoint descriptor, > different from the interface descriptor's value: 9 > usb 1-1: New USB device found, idVendor=046d, idProduct=c298, bcdDevice= > 0.00 > usb 1-1: New USB device strings: Mfr=0, Product=0, SerialNumber=0 > usb 1-1: config 0 descriptor?? > logitech 0003:046D:C298.0001: unknown main item tag 0x0 > logitech 0003:046D:C298.0001: unknown main item tag 0x0 > logitech 0003:046D:C298.0001: hidraw0: USB HID v0.00 Device [HID 046d:c298] > on usb-dummy_hcd.0-1/input0 > BUG: KASAN: slab-out-of-bounds in set_bit > include/asm-generic/bitops-instrumented.h:28 [inline] #syz test: https://github.com/google/kasan.git e96407b4 >From 90b712f3e9b9a45996eb0dfe5f489a4502c9f843 Mon Sep 17 00:00:00 2001 From: Oliver Neukum Date: Mon, 5 Aug 2019 16:14:47 +0200 Subject: [PATCH] hid-lg4ff: sanity check for offsets of FF effects Malicious devices could provide huge offsets which would lead to setting bits in random kernel memory. Adding a sanity check. Reported-by: syzbot+94e2b9e9c7d1dd332...@syzkaller.appspotmail.com Signed-off-by: Oliver Neukum --- drivers/hid/hid-lg4ff.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/hid/hid-lg4ff.c b/drivers/hid/hid-lg4ff.c index cefba038520c..9e63da793a0d 100644 --- a/drivers/hid/hid-lg4ff.c +++ b/drivers/hid/hid-lg4ff.c @@ -1327,8 +1327,12 @@ int lg4ff_init(struct hid_device *hid) } /* Set supported force feedback capabilities */ + error = -ENODEV; for (j = 0; lg4ff_devices[i].ff_effects[j] >= 0; j++) - set_bit(lg4ff_devices[i].ff_effects[j], dev->ffbit); + if (lg4ff_devices[i].ff_effects[j] < FF_CNT) + set_bit(lg4ff_devices[i].ff_effects[j], dev->ffbit); + else + goto err_init; error = input_ff_create_memless(dev, NULL, lg4ff_play); -- 2.16.4
Re: KASAN: slab-out-of-bounds Write in lg4ff_init
Am Montag, den 05.08.2019, 16:53 +0200 schrieb Andrey Konovalov: > On Mon, Aug 5, 2019 at 4:34 PM Oliver Neukum wrote: > > > > Am Montag, den 05.08.2019, 05:38 -0700 schrieb syzbot: > > > Hello, > > > > > > syzbot found the following crash on: > > > > > > HEAD commit:e96407b4 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=144c21dc60 > > > kernel config: https://syzkaller.appspot.com/x/.config?x=cfa2c18fb6a8068e > > > dashboard link: > > > https://syzkaller.appspot.com/bug?extid=94e2b9e9c7d1dd332345 > > > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=169e854260 > > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=10ec826260 > > > > > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > > > Reported-by: syzbot+94e2b9e9c7d1dd332...@syzkaller.appspotmail.com > > > > > > usb 1-1: config 0 interface 0 altsetting 0 has 1 endpoint descriptor, > > > different from the interface descriptor's value: 9 > > > usb 1-1: New USB device found, idVendor=046d, idProduct=c298, bcdDevice= > > > 0.00 > > > usb 1-1: New USB device strings: Mfr=0, Product=0, SerialNumber=0 > > > usb 1-1: config 0 descriptor?? > > > logitech 0003:046D:C298.0001: unknown main item tag 0x0 > > > logitech 0003:046D:C298.0001: unknown main item tag 0x0 > > > logitech 0003:046D:C298.0001: hidraw0: USB HID v0.00 Device [HID > > > 046d:c298] > > > on usb-dummy_hcd.0-1/input0 > > > BUG: KASAN: slab-out-of-bounds in set_bit > > > include/asm-generic/bitops-instrumented.h:28 [inline] > > > > #syz test: https://github.com/google/kasan.git e96407b4 > > > > From 7e7f8ce9108b69613f8bb4ff2f95c258e22c3228 Mon Sep 17 00:00:00 2001 > > From: Oliver Neukum > > Date: Mon, 5 Aug 2019 16:14:47 +0200 > > Subject: [PATCH] hid-lg4ff: sanity check for offsets of FF effects > > > > Malicious devices could provide huge offsets which would lead > > to setting bits in random kernel memory. Adding a sanity check. > > > > Reported-by: syzbot+94e2b9e9c7d1dd332...@syzkaller.appspotmail.com > > Signed-off-by: Oliver Neukum > > --- > > drivers/hid/hid-lg4ff.c | 6 +- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/hid/hid-lg4ff.c b/drivers/hid/hid-lg4ff.c > > index cefba038520c..f9572750d889 100644 > > --- a/drivers/hid/hid-lg4ff.c > > +++ b/drivers/hid/hid-lg4ff.c > > @@ -1327,8 +1327,12 @@ int lg4ff_init(struct hid_device *hid) > > } > > > > /* Set supported force feedback capabilities */ > > + error = -ENODEV; > > for (j = 0; lg4ff_devices[i].ff_effects[j] >= 0; j++) > > - set_bit(lg4ff_devices[i].ff_effects[j], dev->ffbit); > > + if (lg4ff_devices[i].ff_effects[j] <= 15) > > Can't ff_effects have one of the FF_CONSTANT, FF_PERIODIC, etc. > values? Those are 0x50, 0x51, ... Or maybe I'm just misunderstanding > something. Are those ff_effects provided by the device? You are correct. It is a bitmap. Next patch is in the works. Reagrds Oliver
Re: KASAN: slab-out-of-bounds Write in lg4ff_init
Am Montag, den 05.08.2019, 05:38 -0700 schrieb syzbot: > Hello, > > syzbot found the following crash on: > > HEAD commit:e96407b4 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=144c21dc60 > kernel config: https://syzkaller.appspot.com/x/.config?x=cfa2c18fb6a8068e > dashboard link: https://syzkaller.appspot.com/bug?extid=94e2b9e9c7d1dd332345 > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=169e854260 > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=10ec826260 > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > Reported-by: syzbot+94e2b9e9c7d1dd332...@syzkaller.appspotmail.com > > usb 1-1: config 0 interface 0 altsetting 0 has 1 endpoint descriptor, > different from the interface descriptor's value: 9 > usb 1-1: New USB device found, idVendor=046d, idProduct=c298, bcdDevice= > 0.00 > usb 1-1: New USB device strings: Mfr=0, Product=0, SerialNumber=0 > usb 1-1: config 0 descriptor?? > logitech 0003:046D:C298.0001: unknown main item tag 0x0 > logitech 0003:046D:C298.0001: unknown main item tag 0x0 > logitech 0003:046D:C298.0001: hidraw0: USB HID v0.00 Device [HID 046d:c298] > on usb-dummy_hcd.0-1/input0 > BUG: KASAN: slab-out-of-bounds in set_bit > include/asm-generic/bitops-instrumented.h:28 [inline] #syz test: https://github.com/google/kasan.git e96407b4 >From 7e7f8ce9108b69613f8bb4ff2f95c258e22c3228 Mon Sep 17 00:00:00 2001 From: Oliver Neukum Date: Mon, 5 Aug 2019 16:14:47 +0200 Subject: [PATCH] hid-lg4ff: sanity check for offsets of FF effects Malicious devices could provide huge offsets which would lead to setting bits in random kernel memory. Adding a sanity check. Reported-by: syzbot+94e2b9e9c7d1dd332...@syzkaller.appspotmail.com Signed-off-by: Oliver Neukum --- drivers/hid/hid-lg4ff.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/hid/hid-lg4ff.c b/drivers/hid/hid-lg4ff.c index cefba038520c..f9572750d889 100644 --- a/drivers/hid/hid-lg4ff.c +++ b/drivers/hid/hid-lg4ff.c @@ -1327,8 +1327,12 @@ int lg4ff_init(struct hid_device *hid) } /* Set supported force feedback capabilities */ + error = -ENODEV; for (j = 0; lg4ff_devices[i].ff_effects[j] >= 0; j++) - set_bit(lg4ff_devices[i].ff_effects[j], dev->ffbit); + if (lg4ff_devices[i].ff_effects[j] <= 15) + set_bit(lg4ff_devices[i].ff_effects[j], dev->ffbit); + else + goto err_init; error = input_ff_create_memless(dev, NULL, lg4ff_play); -- 2.16.4
Re: KASAN: use-after-free Read in usb_free_coherent
Am Donnerstag, den 01.08.2019, 11:52 +0100 schrieb Suzuki K Poulose:
>
> Looks like the yurex_delete() drops the ref count on the dev->udev
> way early in the function and then later tries to see if there
> are any other buffers associated with it worth releasing. So,
> I am guessing moving the usb_put_dev(), after we have finished
> all that might solve the issue ?
Hi,
the fix looks good to me.
Regards
Oliver
Re: WARNING in usbhid_raw_request/usb_submit_urb
Am Dienstag, den 30.07.2019, 16:12 +0200 schrieb Andrey Konovalov: > On Tue, Jul 30, 2019 at 4:10 PM Alan Stern wrote: > > > > On Mon, 29 Jul 2019, syzbot wrote: > > > > > Hello, > > > > > > syzbot found the following crash on: > > > > > > HEAD commit:6a3599ce 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=12386cb460 > > > kernel config: https://syzkaller.appspot.com/x/.config?x=700ca426ab83faae > > > dashboard link: > > > https://syzkaller.appspot.com/bug?extid=a7a6b9c609b9457c62c6 > > > 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+a7a6b9c609b9457c6...@syzkaller.appspotmail.com > > > > > > [ cut here ] > > > usb 2-1: BOGUS urb xfer, pipe 2 != type 2 > > > WARNING: CPU: 0 PID: 3730 at drivers/usb/core/urb.c:477 > > > usb_submit_urb+0x1188/0x13b0 drivers/usb/core/urb.c:477 > > > Kernel panic - not syncing: panic_on_warn set ... > > > CPU: 0 PID: 3730 Comm: syz-executor.1 Not tainted 5.2.0-rc6+ #15 > > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS > > > Google 01/01/2011 > > > Call Trace: > > > __dump_stack lib/dump_stack.c:77 [inline] > > > dump_stack+0xca/0x13e lib/dump_stack.c:113 > > > panic+0x292/0x6c9 kernel/panic.c:219 > > > __warn.cold+0x20/0x4b kernel/panic.c:576 > > > report_bug+0x262/0x2a0 lib/bug.c:186 > > > fixup_bug arch/x86/kernel/traps.c:179 [inline] > > > fixup_bug arch/x86/kernel/traps.c:174 [inline] > > > do_error_trap+0x12b/0x1e0 arch/x86/kernel/traps.c:272 > > > do_invalid_op+0x32/0x40 arch/x86/kernel/traps.c:291 > > > invalid_op+0x14/0x20 arch/x86/entry/entry_64.S:986 > > > RIP: 0010:usb_submit_urb+0x1188/0x13b0 drivers/usb/core/urb.c:477 > > > Code: 4d 85 ed 74 2c e8 f8 d3 f4 fd 4c 89 f7 e8 a0 51 1c ff 41 89 d8 44 89 > > > e1 4c 89 ea 48 89 c6 48 c7 c7 00 0e f7 85 e8 83 98 ca fd <0f> 0b e9 20 f4 > > > ff ff e8 cc d3 f4 fd 4c 89 f2 48 b8 00 00 00 00 00 > > > RSP: 0018:8881d4f479d0 EFLAGS: 00010282 > > > RAX: RBX: 0002 RCX: > > > RDX: 5dfa RSI: 8127ef3d RDI: ed103a9e8f2c > > > RBP: R08: 8881af663000 R09: > > > R10: R11: R12: 0002 > > > R13: 8881d462ed38 R14: 8881d18f9a20 R15: 8881d80e1c00 > > > usb_start_wait_urb+0x108/0x2b0 drivers/usb/core/message.c:57 > > > usb_internal_control_msg drivers/usb/core/message.c:101 [inline] > > > usb_control_msg+0x31c/0x4a0 drivers/usb/core/message.c:152 > > > usbhid_set_raw_report drivers/hid/usbhid/hid-core.c:917 [inline] > > > usbhid_raw_request+0x21f/0x640 drivers/hid/usbhid/hid-core.c:1265 > > > hid_hw_raw_request include/linux/hid.h:1079 [inline] > > > hidraw_send_report+0x296/0x500 drivers/hid/hidraw.c:151 > > > hidraw_ioctl+0x5b4/0xaf0 drivers/hid/hidraw.c:421 > > > vfs_ioctl fs/ioctl.c:46 [inline] > > > file_ioctl fs/ioctl.c:509 [inline] > > > do_vfs_ioctl+0xcda/0x12e0 fs/ioctl.c:696 > > > ksys_ioctl+0x9b/0xc0 fs/ioctl.c:713 > > > __do_sys_ioctl fs/ioctl.c:720 [inline] > > > __se_sys_ioctl fs/ioctl.c:718 [inline] > > > __x64_sys_ioctl+0x6f/0xb0 fs/ioctl.c:718 > > > do_syscall_64+0xb7/0x560 arch/x86/entry/common.c:301 > > > entry_SYSCALL_64_after_hwframe+0x49/0xbe > > > > This is very strange. It looks like the kernel is complaining that 2 > > != 2. > > > > A more likely explanation is a race in the usbhid driver. If > > usbhid_set_raw_report() gets called _after_ usbhid has been unbound > > from the device and while the endpoint is being destroyed, we could get > > something like this. > > > > Perhaps one of Oliver's patches will also fix this. > > Since there's no reproducer this is quite likely some kind of race. We > can close this bug once Oliver's patches are applied, and if it gets > triggered again syzbot will rereport it. AFAICT my patch right now introduces another race. This will require a fresh look. Regards Oliver
Re: WARNING in usbtouch_open
Am Montag, den 29.07.2019, 09:38 -0700 schrieb syzbot:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit:7f7867ff 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=1503f4ec60
> kernel config: https://syzkaller.appspot.com/x/.config?x=792eb47789f57810
> dashboard link: https://syzkaller.appspot.com/bug?extid=199ea16c7f26418b4365
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=173e444260
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=115482b260
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+199ea16c7f26418b4...@syzkaller.appspotmail.com
#syz test: https://github.com/google/kasan.git usb-fuzzer
>From 29b755588bd353d0e10ae384c2c551dffa1b3e7b Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Tue, 30 Jul 2019 12:00:27 +0200
Subject: [PATCH] usbtouchscreen: add proper initialization
Mutexes shall be initialized before they are used.
Fixes: 12e510dbc57b2 ("Input: usbtouchscreen - fix deadlock in autosuspend")
Reported-by: syzbot+199ea16c7f26418b4...@syzkaller.appspotmail.com
Signed-off-by: Oliver Neukum
---
drivers/input/touchscreen/usbtouchscreen.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/input/touchscreen/usbtouchscreen.c
b/drivers/input/touchscreen/usbtouchscreen.c
index a2cec6cacf57..caacf211f51b 100644
--- a/drivers/input/touchscreen/usbtouchscreen.c
+++ b/drivers/input/touchscreen/usbtouchscreen.c
@@ -1658,6 +1658,7 @@ static int usbtouch_probe(struct usb_interface *intf,
input_dev = input_allocate_device();
if (!usbtouch || !input_dev)
goto out_free;
+ mutex_init(&usbtouch->pm_mutex);
type = &usbtouch_dev_info[id->driver_info];
usbtouch->type = type;
--
2.16.4
Re: general protection fault in flexcop_usb_probe
Am Montag, den 29.07.2019, 18:54 +0200 schrieb Andrey Konovalov: Hi, > Thanks a lot for fixing all of these USB bugs! I fear the day we get serious about MA USB. All these issues will turn into security issues. > The usb-fuzzer branch is working again, so it should be possible to > use it for testing. But, I've actually just realized, that the proper > way to test fixes for USB bugs is to use the exact commit hash that is > provided in each bug report (the kernel interface for emulating USB > device is not stable yet, and has significantly changed at least > once). I've updated syzbot documentation to reflect this. Where is taht documentation? > Let's try to retest this one with the right kernel commit id: > > #syz test: https://github.com/google/kasan.git 9a33b369 Retesting. Regards Oliver
KASAN reporting: general protection fault in flexcop_usb_probe
Reacting to this:
Title: general protection fault in flexcop_usb_probe
Last occurred: 0 days ago
Reported: 102 days ago
Branches: Mainline (with usb-fuzzer patches)
Dashboard link: https://syzkaller.appspot.com/bug?id=c0203bd72037d0
7493f4b7562411e4f5f4553a8f
Original thread:https://lkml.kernel.org/lkml/10fe260586
536...@google.com/T/#u
This bug has a C reproducer.
No one replied to the original thread for this bug.
This looks like a bug in a media USB driver.
If you fix this bug, please add the following tag to the commit:
Reported-by: syzbot+d93dff37e6a89431c...@syzkaller.appspotmail.com
#syz test: https://github.com/google/kasan.git 9a33b369
>From 5a34ecc6c75479a9f245a867e1ce37e6e28f58f8 Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Mon, 29 Jul 2019 16:21:11 +0200
Subject: [PATCH] b2c2-flexcop-usb: add sanity checking
The driver needs an isochronous endpoint to be present. It will
oops in its absence. Add checking for it.
Reported-by: syzbot+d93dff37e6a89431c...@syzkaller.appspotmail.com
Signed-off-by: Oliver Neukum
---
drivers/media/usb/b2c2/flexcop-usb.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/media/usb/b2c2/flexcop-usb.c
b/drivers/media/usb/b2c2/flexcop-usb.c
index 1826ff825c2e..1a801dc286f8 100644
--- a/drivers/media/usb/b2c2/flexcop-usb.c
+++ b/drivers/media/usb/b2c2/flexcop-usb.c
@@ -538,6 +538,9 @@ static int flexcop_usb_probe(struct usb_interface *intf,
struct flexcop_device *fc = NULL;
int ret;
+ if (intf->cur_altsetting->desc.bNumEndpoints < 1)
+ return -ENODEV;
+
if ((fc = flexcop_device_kmalloc(sizeof(struct flexcop_usb))) == NULL) {
err("out of memory\n");
return -ENOMEM;
KASAN reporting: general protection fault in flexcop_usb_probe
Reacting to this:
Title: general protection fault in flexcop_usb_probe
Last occurred: 0 days ago
Reported: 102 days ago
Branches: Mainline (with usb-fuzzer patches)
Dashboard link: https://syzkaller.appspot.com/bug?id=c0203bd72037d0
7493f4b7562411e4f5f4553a8f
Original thread:https://lkml.kernel.org/lkml/10fe260586
536...@google.com/T/#u
This bug has a C reproducer.
No one replied to the original thread for this bug.
This looks like a bug in a media USB driver.
If you fix this bug, please add the following tag to the commit:
Reported-by: syzbot+d93dff37e6a89431c...@syzkaller.appspotmail.com
#syz test: https://github.com/google/kasan.git usb-fuzzer-usb-testing-2019.07.11
>From 5a34ecc6c75479a9f245a867e1ce37e6e28f58f8 Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Mon, 29 Jul 2019 16:21:11 +0200
Subject: [PATCH] b2c2-flexcop-usb: add sanity checking
The driver needs an isochronous endpoint to be present. It will
oops in its absence. Add checking for it.
Reported-by: syzbot+d93dff37e6a89431c...@syzkaller.appspotmail.com
Signed-off-by: Oliver Neukum
---
drivers/media/usb/b2c2/flexcop-usb.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/media/usb/b2c2/flexcop-usb.c
b/drivers/media/usb/b2c2/flexcop-usb.c
index 1826ff825c2e..1a801dc286f8 100644
--- a/drivers/media/usb/b2c2/flexcop-usb.c
+++ b/drivers/media/usb/b2c2/flexcop-usb.c
@@ -538,6 +538,9 @@ static int flexcop_usb_probe(struct usb_interface *intf,
struct flexcop_device *fc = NULL;
int ret;
+ if (intf->cur_altsetting->desc.bNumEndpoints < 1)
+ return -ENODEV;
+
if ((fc = flexcop_device_kmalloc(sizeof(struct flexcop_usb))) == NULL) {
err("out of memory\n");
return -ENOMEM;
--
2.16.4
KASAN reporting bug in ath6kl
Hi, I am looking at this report: Title: general protection fault in ath6kl_usb_alloc_urb_from_pipe Last occurred: 0 days ago Reported: 102 days ago Branches: Mainline (with usb-fuzzer patches) Dashboard link: https://syzkaller.appspot.com/bug?id=cd8b9cfe50a0bf 36ee19eda2d7e2e06843dfbeaf Original thread:https://lkml.kernel.org/lkml/8e82510586 561...@google.com/T/#u This bug has a C reproducer. No one replied to the original thread for this bug. This looks like a bug in a net/wireless USB driver. If you fix this bug, please add the following tag to the commit: Reported-by: syzbot+ead4037ec793e025e...@syzkaller.appspotmail.com -- It looks like a bug in static int ath6kl_usb_setup_pipe_resources(struct ath6kl_usb *ar_usb) to me, which happily does nothing if the device has no endpoints. THis needs sanity checking, but it looks like the driver really uses 8 endpoints. Can you confirm that all 8 of them are indeed needed? Regards Oliver
Re: WARNING in iguanair_probe/usb_submit_urb
Am Freitag, den 26.07.2019, 05:28 -0700 schrieb syzbot:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit:6a3599ce 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=164ab1f060
> kernel config: https://syzkaller.appspot.com/x/.config?x=700ca426ab83faae
> dashboard link: https://syzkaller.appspot.com/bug?extid=01a77b82edaa374068e1
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=143d797860
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=134623f460
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+01a77b82edaa37406...@syzkaller.appspotmail.com
#syz test: https://github.com/google/kasan.git usb-fuzzer-usb-testing-2019.07.11
>From 0b0a7f7e980973e0c0d17f1dfe2bd7742492bfcc Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Mon, 29 Jul 2019 11:49:00 +0200
Subject: [PATCH] iguanair: add sanity checks
The driver needs to check the endpoint types, too, as opposed
to the number of endpoints. This also requires moving the check earlier.
Reported-by: syzbot+01a77b82edaa37406...@syzkaller.appspotmail.com
Signed-off-by: Oliver Neukum
---
drivers/media/rc/iguanair.c | 15 +++
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/media/rc/iguanair.c b/drivers/media/rc/iguanair.c
index ea05e125016a..663083a6b399 100644
--- a/drivers/media/rc/iguanair.c
+++ b/drivers/media/rc/iguanair.c
@@ -413,6 +413,10 @@ static int iguanair_probe(struct usb_interface *intf,
int ret, pipein, pipeout;
struct usb_host_interface *idesc;
+ idesc = intf->altsetting;
+ if (idesc->desc.bNumEndpoints < 2)
+ return -ENODEV;
+
ir = kzalloc(sizeof(*ir), GFP_KERNEL);
rc = rc_allocate_device(RC_DRIVER_IR_RAW);
if (!ir || !rc) {
@@ -427,18 +431,13 @@ static int iguanair_probe(struct usb_interface *intf,
ir->urb_in = usb_alloc_urb(0, GFP_KERNEL);
ir->urb_out = usb_alloc_urb(0, GFP_KERNEL);
- if (!ir->buf_in || !ir->packet || !ir->urb_in || !ir->urb_out) {
+ if (!ir->buf_in || !ir->packet || !ir->urb_in || !ir->urb_out ||
+ !usb_endpoint_is_int_in(&idesc->endpoint[0].desc) ||
+ !usb_endpoint_is_int_out(&idesc->endpoint[1].desc)) {
ret = -ENOMEM;
goto out;
}
- idesc = intf->altsetting;
-
- if (idesc->desc.bNumEndpoints < 2) {
- ret = -ENODEV;
- goto out;
- }
-
ir->rc = rc;
ir->dev = &intf->dev;
ir->udev = udev;
Re: WARNING in iguanair_probe/usb_submit_urb
Am Freitag, den 26.07.2019, 05:28 -0700 schrieb syzbot:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit:6a3599ce 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=164ab1f060
> kernel config: https://syzkaller.appspot.com/x/.config?x=700ca426ab83faae
> dashboard link: https://syzkaller.appspot.com/bug?extid=01a77b82edaa374068e1
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=143d797860
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=134623f460
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+01a77b82edaa37406...@syzkaller.appspotmail.com
#syz test: https://github.com/google/kasan.git
usb-fuzzer-usb-testing-2019.07.11]
>From 0b0a7f7e980973e0c0d17f1dfe2bd7742492bfcc Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Mon, 29 Jul 2019 11:49:00 +0200
Subject: [PATCH] iguanair: add sanity checks
The driver needs to check the endpoint types, too, as opposed
to the number of endpoints. This also requires moving the check earlier.
Reported-by: syzbot+01a77b82edaa37406...@syzkaller.appspotmail.com
Signed-off-by: Oliver Neukum
---
drivers/media/rc/iguanair.c | 15 +++
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/media/rc/iguanair.c b/drivers/media/rc/iguanair.c
index ea05e125016a..663083a6b399 100644
--- a/drivers/media/rc/iguanair.c
+++ b/drivers/media/rc/iguanair.c
@@ -413,6 +413,10 @@ static int iguanair_probe(struct usb_interface *intf,
int ret, pipein, pipeout;
struct usb_host_interface *idesc;
+ idesc = intf->altsetting;
+ if (idesc->desc.bNumEndpoints < 2)
+ return -ENODEV;
+
ir = kzalloc(sizeof(*ir), GFP_KERNEL);
rc = rc_allocate_device(RC_DRIVER_IR_RAW);
if (!ir || !rc) {
@@ -427,18 +431,13 @@ static int iguanair_probe(struct usb_interface *intf,
ir->urb_in = usb_alloc_urb(0, GFP_KERNEL);
ir->urb_out = usb_alloc_urb(0, GFP_KERNEL);
- if (!ir->buf_in || !ir->packet || !ir->urb_in || !ir->urb_out) {
+ if (!ir->buf_in || !ir->packet || !ir->urb_in || !ir->urb_out ||
+ !usb_endpoint_is_int_in(&idesc->endpoint[0].desc) ||
+ !usb_endpoint_is_int_out(&idesc->endpoint[1].desc)) {
ret = -ENOMEM;
goto out;
}
- idesc = intf->altsetting;
-
- if (idesc->desc.bNumEndpoints < 2) {
- ret = -ENODEV;
- goto out;
- }
-
ir->rc = rc;
ir->dev = &intf->dev;
ir->udev = udev;
--
2.16.4
Re: [PATCH] usb: storage: sddr55: Fix a possible null-pointer dereference in sddr55_transport()
Am Montag, den 29.07.2019, 18:05 +0800 schrieb Jia-Ju Bai: Hi, > In sddr55_transport(), there is an if statement on line 836 to check > whether info->lba_to_pba is NULL: > if (info->lba_to_pba == NULL || ...) > > When info->lba_to_pba is NULL, it is used on line 948: > pba = info->lba_to_pba[lba]; > > Thus, a possible null-pointer dereference may occur. Yes, in practice READ_CAPACITY will always be called and set up the correct translation table, but you can probably exploit this. > To fix this bug, info->lba_to_pba is checked before being used. > > This bug is found by a static analysis tool STCheck written by us. > > Signed-off-by: Jia-Ju Bai > --- > drivers/usb/storage/sddr55.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/storage/sddr55.c b/drivers/usb/storage/sddr55.c > index b8527c55335b..50afc39aa21d 100644 > --- a/drivers/usb/storage/sddr55.c > +++ b/drivers/usb/storage/sddr55.c > @@ -945,7 +945,8 @@ static int sddr55_transport(struct scsi_cmnd *srb, struct > us_data *us) > return USB_STOR_TRANSPORT_FAILED; > } > > - pba = info->lba_to_pba[lba]; > + if (info->lba_to_pba) > + pba = info->lba_to_pba[lba]; If you use that fix, pba will be uninitialized when used. It should be something like: pba = info->lba_to_pba ? info->lba_to_pba[lba] : 0; Regards Oliver
Re: KASAN: use-after-free Read in usbhid_power
Am Dienstag, den 23.07.2019, 05:48 -0700 schrieb syzbot:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit:6a3599ce 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=11b13e7860
> kernel config: https://syzkaller.appspot.com/x/.config?x=700ca426ab83faae
> dashboard link: https://syzkaller.appspot.com/bug?extid=ef5de9c4f99c4edb4e49
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1548221060
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1139b07c60
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+ef5de9c4f99c4edb4...@syzkaller.appspotmail.com
>
> ==
> BUG: KASAN: use-after-free in usbhid_power+0xca/0xe0
> /drivers/hid/usbhid/hid-core.c:1234
> Read of size 8 at addr 8881ce8a4008 by task syz-executor373/1763
#syz test: https://github.com/google/kasan.git usb-fuzzer-usb-testing-2019.07.11
>From a3455c4527bdfe86536856ea96288b7dcc36590b Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Wed, 24 Jul 2019 22:49:57 +0200
Subject: [PATCH] usbhid: test for disconnected state in raw opening
If the device has been disconnected, we should not proceed to use
runtime PM on it.
Reported-by: syzbot+ef5de9c4f99c4edb4...@syzkaller.appspotmail.com
Signed-off-by: Oliver Neukum
---
drivers/hid/usbhid/hid-core.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index c7bc9db5b192..98b996ecf4d3 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -1229,6 +1229,17 @@ static int usbhid_power(struct hid_device *hid, int lvl)
struct usbhid_device *usbhid = hid->driver_data;
int r = 0;
+ spin_lock_irq(&usbhid->lock);
+ if (test_bit(HID_DISCONNECTED, &usbhid->iofl)) {
+ r = -ENODEV;
+ spin_unlock_irq(&usbhid->lock);
+ goto bail_out;
+ } else {
+ /* protect against disconnect */
+ usb_get_dev(interface_to_usbdev(usbhid->intf));
+ spin_unlock_irq(&usbhid->lock);
+ }
+
switch (lvl) {
case PM_HINT_FULLON:
r = usb_autopm_get_interface(usbhid->intf);
@@ -1238,7 +1249,9 @@ static int usbhid_power(struct hid_device *hid, int lvl)
usb_autopm_put_interface(usbhid->intf);
break;
}
+ usb_put_dev(interface_to_usbdev(usbhid->intf));
+bail_out:
return r;
}
Re: KASAN: use-after-free Read in usbhid_power
Am Mittwoch, den 24.07.2019, 17:02 -0400 schrieb Alan Stern:
> On Wed, 24 Jul 2019, Oliver Neukum wrote:
>
> > drivers/hid/usbhid/hid-core.c | 13 +
> > 1 file changed, 13 insertions(+)
> >
> > diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> > index c7bc9db5b192..98b996ecf4d3 100644
> > --- a/drivers/hid/usbhid/hid-core.c
> > +++ b/drivers/hid/usbhid/hid-core.c
> > @@ -1229,6 +1229,17 @@ static int usbhid_power(struct hid_device *hid, int
> > lvl)
> > struct usbhid_device *usbhid = hid->driver_data;
> > int r = 0;
> >
> > + spin_lock_irq(&usbhid->lock);
> > + if (test_bit(HID_DISCONNECTED, &usbhid->iofl)) {
> > + r = -ENODEV;
> > + spin_unlock_irq(&usbhid->lock);
> > + goto bail_out;
> > + } else {
> > + /* protect against disconnect */
> > + usb_get_dev(interface_to_usbdev(usbhid->intf));
> > + spin_unlock_irq(&usbhid->lock);
> > + }
> > +
> > switch (lvl) {
> > case PM_HINT_FULLON:
> > r = usb_autopm_get_interface(usbhid->intf);
> > @@ -1238,7 +1249,9 @@ static int usbhid_power(struct hid_device *hid, int
> > lvl)
> > usb_autopm_put_interface(usbhid->intf);
> > break;
> > }
> > + usb_put_dev(interface_to_usbdev(usbhid->intf));
> >
> > +bail_out:
> > return r;
> > }
>
> Isn't this treating the symptom instead of the cause?
Sort of. Holding a reference for the whole time would have merit,
but I doubt it is strictly necessary.
> Shouldn't the hid_device hold a reference to usbhid->intf throughout
> its lifetime? That way this sort of problem wouldn't arise in any
> routine, not just usbhid_power().
Unfortunately the semantics would still be wrong without the check
in corner cases. In case disconnect() is called without a physical
unplug, we must not touch the power state.
I am indeed afraid that in that case my putative fix is still racy.
But I don't to just introduce a mutex just for this. Any ideas?
Regards
Oliver
Re: KASAN: use-after-free Read in usbhid_power
Am Dienstag, den 23.07.2019, 05:48 -0700 schrieb syzbot:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit:6a3599ce 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=11b13e7860
> kernel config: https://syzkaller.appspot.com/x/.config?x=700ca426ab83faae
> dashboard link: https://syzkaller.appspot.com/bug?extid=ef5de9c4f99c4edb4e49
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1548221060
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1139b07c60
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+ef5de9c4f99c4edb4...@syzkaller.appspotmail.com
>
> ==
> BUG: KASAN: use-after-free in usbhid_power+0xca/0xe0
> /drivers/hid/usbhid/hid-core.c:1234
> Read of size 8 at addr 8881ce8a4008 by task syz-executor373/1763
#syz test: https://github.com/google/kasan.git usb-fuzzer
>From a3455c4527bdfe86536856ea96288b7dcc36590b Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Wed, 24 Jul 2019 22:49:57 +0200
Subject: [PATCH] usbhid: test for disconnected state in raw opening
If the device has been disconnected, we should not proceed to use
runtime PM on it.
Reported-by: syzbot+ef5de9c4f99c4edb4...@syzkaller.appspotmail.com
Signed-off-by: Oliver Neukum
---
drivers/hid/usbhid/hid-core.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index c7bc9db5b192..98b996ecf4d3 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -1229,6 +1229,17 @@ static int usbhid_power(struct hid_device *hid, int lvl)
struct usbhid_device *usbhid = hid->driver_data;
int r = 0;
+ spin_lock_irq(&usbhid->lock);
+ if (test_bit(HID_DISCONNECTED, &usbhid->iofl)) {
+ r = -ENODEV;
+ spin_unlock_irq(&usbhid->lock);
+ goto bail_out;
+ } else {
+ /* protect against disconnect */
+ usb_get_dev(interface_to_usbdev(usbhid->intf));
+ spin_unlock_irq(&usbhid->lock);
+ }
+
switch (lvl) {
case PM_HINT_FULLON:
r = usb_autopm_get_interface(usbhid->intf);
@@ -1238,7 +1249,9 @@ static int usbhid_power(struct hid_device *hid, int lvl)
usb_autopm_put_interface(usbhid->intf);
break;
}
+ usb_put_dev(interface_to_usbdev(usbhid->intf));
+bail_out:
return r;
}
--
2.16.4
Re: KASAN: use-after-free Read in usbhid_power
Am Dienstag, den 23.07.2019, 05:48 -0700 schrieb syzbot: > > Freed by task 243: > save_stack+0x1b/0x80 /mm/kasan/common.c:71 > set_track /mm/kasan/common.c:79 [inline] > __kasan_slab_free+0x130/0x180 /mm/kasan/common.c:451 > slab_free_hook /mm/slub.c:1421 [inline] > slab_free_freelist_hook /mm/slub.c:1448 [inline] > slab_free /mm/slub.c:2994 [inline] > kfree+0xd7/0x280 /mm/slub.c:3949 > skb_free_head+0x8b/0xa0 /net/core/skbuff.c:588 > skb_release_data+0x41f/0x7c0 /net/core/skbuff.c:608 > skb_release_all+0x46/0x60 /net/core/skbuff.c:662 > __kfree_skb /net/core/skbuff.c:676 [inline] > consume_skb /net/core/skbuff.c:736 [inline] > consume_skb+0xc0/0x2f0 /net/core/skbuff.c:730 > skb_free_datagram+0x16/0xf0 /net/core/datagram.c:328 > netlink_recvmsg+0x65e/0xea0 /net/netlink/af_netlink.c:2001 > sock_recvmsg_nosec /net/socket.c:877 [inline] > sock_recvmsg /net/socket.c:894 [inline] > sock_recvmsg+0xca/0x110 /net/socket.c:890 > ___sys_recvmsg+0x271/0x5a0 /net/socket.c:2448 > __sys_recvmsg+0xe9/0x1b0 /net/socket.c:2497 > do_syscall_64+0xb7/0x560 /arch/x86/entry/common.c:301 > entry_SYSCALL_64_after_hwframe+0x49/0xbe How reliable is this trace? It seems very likely to me that this bug is the same bug as syzbot KASAN: use-after-free Read in hidraw_ioctl which shows a race with disconnect() instead of some networking code, which I really cannot fathom. Regards Oliver
Re: Titan Ridge xHCI may stop to working after re-plugging the dock
Am Dienstag, den 09.07.2019, 21:10 +0800 schrieb Kai-Heng Feng: > Hi Mika and Mathias, > > I’ve filed a bug [1] which renders docking station unusable. > > I am not sure it's a bug in PCI, Thunderbolt or xHCI so raise the issue to > you both. > > [1] https://bugzilla.kernel.org/show_bug.cgi?id=203885 > > Kai-Heng > The issue starts before you unplug. In fact it starts before the dock is even detected the first time: [ 13.171167] rfkill: input handler disabled [ 19.781905] pcieport :00:1c.0: PME: Spurious native interrupt! [ 19.781909] pcieport :00:1c.0: PME: Spurious native interrupt! [ 20.109251] usb 4-1: new SuperSpeedPlus Gen 2 USB device number 2 using xhci_hcd [ 20.136000] usb 4-1: New USB device found, idVendor=0bda, idProduct=0487, bcdDevice= 1.47 [ 20.136004] usb 4-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0 [ 20.136007] usb 4-1: Product: Dell dock [ 20.136009] usb 4-1: Manufacturer: Dell Inc. [ 20.140607] hub 4-1:1.0: USB hub found [ 20.141004] hub 4-1:1.0: 4 ports detected [ 20.253025] usb 1-4: new high-speed USB device number 5 using xhci_hcd [ 20.403520] usb 1-4: New USB device found, idVendor=0bda, idProduct=5487, bcdDevice= 1.47 [ 20.403521] usb 1-4: New USB device strings: Mfr=1, Product=2, SerialNumber=0 [ 20.403522] usb 1-4: Product: Dell dock [ 20.403522] usb 1-4: Manufacturer: Dell Inc. [ 20.404348] hub 1-4:1.0: USB hub found This looks like a PCI issue. In general, this kind of reporting sucks. We have to guess what you did at 19.781905 Regards Oliver
Re: [PATCH] Fix chipmunk-like voice when using Logitech C270 for recording audio.
Am Donnerstag, den 20.06.2019, 21:19 +0100 schrieb Aidan Thornton:
> What's particularly annoying is that since this is an intermittent
> problem, it's hard to tell if I'm chasing a phantom solution for it
> again. Haven't managed to replicate it since applying this fix and did
> so pretty quickly before but you never know.
>
This is time for the sledge hammer. No more surgical solutions.
Could you test the attached patch?
Regards
Oliver
From 06b826bf3e2d3fb15aee676185c632b9f08a10db Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Wed, 3 Jul 2019 15:31:05 +0200
Subject: [PATCH] Revert "usb: Add USB_QUIRK_RESET_RESUME for all Logitech UVC
webcams"
This s a partial revert of commit e387ef5c47ddeaeaa3cbdc54424cdb7a28dae2c0.
The original fix for the "squeaky voice" bug of some Logitech webcams
when used as audio devices was in
2394d67e446bf616a0885167d5f0d397bdacfdfc ("USB: add RESET_RESUME for
webcams shown to be quirky")
which in subsequent development was undone. Later tests have shown
the consolodiation to be not equivalent.
So I am taking the conservative approach, blacklisting everything that
was blacklisted at any point in the past. We have reports of multiple
devices affected and retesting this on so many old devices is
impractical.
Signed-off-by: Oliver Neukum
Reported-by: Aidan Thornton
Reported-by: Marco Zatta
---
drivers/usb/core/quirks.c | 53 ++-
1 file changed, 43 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index 533fe8c0f0a2..e3c24d5b2959 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -234,20 +234,53 @@ static const struct usb_device_id usb_quirk_list[] = {
/* Logitech Quickcam Fusion */
{ USB_DEVICE(0x046d, 0x08c1), .driver_info = USB_QUIRK_RESET_RESUME },
- /* Logitech Quickcam Orbit MP */
- { USB_DEVICE(0x046d, 0x08c2), .driver_info = USB_QUIRK_RESET_RESUME },
+ /* Logitech Webcam C200 */
+ { USB_DEVICE(0x046d, 0x0802), .driver_info = USB_QUIRK_RESET_RESUME },
- /* Logitech Quickcam Pro for Notebook */
- { USB_DEVICE(0x046d, 0x08c3), .driver_info = USB_QUIRK_RESET_RESUME },
+ /* Logitech Webcam C250 */
+ { USB_DEVICE(0x046d, 0x0804), .driver_info = USB_QUIRK_RESET_RESUME },
- /* Logitech Quickcam Pro 5000 */
- { USB_DEVICE(0x046d, 0x08c5), .driver_info = USB_QUIRK_RESET_RESUME },
+ /* Logitech Webcam C300 */
+ { USB_DEVICE(0x046d, 0x0805), .driver_info = USB_QUIRK_RESET_RESUME },
- /* Logitech Quickcam OEM Dell Notebook */
- { USB_DEVICE(0x046d, 0x08c6), .driver_info = USB_QUIRK_RESET_RESUME },
+ /* Logitech Webcam B/C500 */
+ { USB_DEVICE(0x046d, 0x0807), .driver_info = USB_QUIRK_RESET_RESUME },
- /* Logitech Quickcam OEM Cisco VT Camera II */
- { USB_DEVICE(0x046d, 0x08c7), .driver_info = USB_QUIRK_RESET_RESUME },
+ /* Logitech Webcam C600 */
+ { USB_DEVICE(0x046d, 0x0808), .driver_info = USB_QUIRK_RESET_RESUME },
+
+ /* Logitech Webcam Pro 9000 */
+ { USB_DEVICE(0x046d, 0x0809), .driver_info = USB_QUIRK_RESET_RESUME },
+
+ /* Logitech Webcam C905 */
+ { USB_DEVICE(0x046d, 0x080a), .driver_info = USB_QUIRK_RESET_RESUME },
+
+ /* Logitech Webcam C210 */
+ { USB_DEVICE(0x046d, 0x0819), .driver_info = USB_QUIRK_RESET_RESUME },
+
+ /* Logitech Webcam C260 */
+ { USB_DEVICE(0x046d, 0x081a), .driver_info = USB_QUIRK_RESET_RESUME },
+
+ /* Logitech Webcam C310 */
+ { USB_DEVICE(0x046d, 0x081b), .driver_info = USB_QUIRK_RESET_RESUME },
+
+ /* Logitech Webcam C910 */
+ { USB_DEVICE(0x046d, 0x0821), .driver_info = USB_QUIRK_RESET_RESUME },
+
+ /* Logitech Webcam C160 */
+ { USB_DEVICE(0x046d, 0x0824), .driver_info = USB_QUIRK_RESET_RESUME },
+
+ /* Logitech Webcam C270 */
+ { USB_DEVICE(0x046d, 0x0825), .driver_info = USB_QUIRK_RESET_RESUME },
+
+ /* Logitech Quickcam Pro 9000 */
+ { USB_DEVICE(0x046d, 0x0990), .driver_info = USB_QUIRK_RESET_RESUME },
+
+ /* Logitech Quickcam E3500 */
+ { USB_DEVICE(0x046d, 0x09a4), .driver_info = USB_QUIRK_RESET_RESUME },
+
+ /* Logitech Quickcam Vision Pro */
+ { USB_DEVICE(0x046d, 0x09a6), .driver_info = USB_QUIRK_RESET_RESUME },
/* Logitech Harmony 700-series */
{ USB_DEVICE(0x046d, 0xc122), .driver_info = USB_QUIRK_DELAY_INIT },
--
2.16.4
Re: [PATCH] usb: storage: skip only when uas driver is loaded
Am Dienstag, den 02.07.2019, 22:05 +0800 schrieb JC Kuo: Hi, > I don't see the uas issue myself. I was trying to describe a situation that > user having issue with UAS storage and would like to blacklist uas module when > the user is not aware of the usb-storage quirks parameter. > > https://marc.info/?l=linux-usb&m=143385909823645&w=2 So we are not meeting the expectations of some users. Ideally we would. Yet there are things we cannot do. > UAS capable devices are backward-compatible with legacy Bulk-only protocol. > Therefore, IMHO, ideally if system software doesn't have UAS support, system Exact. At compilation time this is a valid consideration. > software should enable the UAS device with Bulk-only protocol, unless > usb-storage driver is not there as well. What you could do, if we cannot change what the kernel does, is improving documentation. We can at least tell users how it is done correctly. Regards Oliver
Re: [PATCH] Fix chipmunk-like voice when using Logitech C270 for recording audio.
Am Donnerstag, den 20.06.2019, 21:19 +0100 schrieb Aidan Thornton: > This doesn't make much sense though. e387ef5c47dd should apply this > quirk to all Logitech UVC webcams, and this is definitely a UVC-based > Logitech webcam with the appropriate VID and interface descriptor. > Surely it shouldn't make any difference whether I add an entry for the > specific VID+PID pair of my particular camera or not? The C270 is as > well, I think. Well, it is applied a little later. > What's particularly annoying is that since this is an intermittent > problem, it's hard to tell if I'm chasing a phantom solution for it > again. Haven't managed to replicate it since applying this fix and did > so pretty quickly before but you never know. Should I just readd everything removed in e387ef5c47dd? Greg, what do you think? This is hard to test, the hardware is old and I want to avoid chasing them all separately. Regards Oliver
Re: [PATCH] usb: core: devio: add ioctls for suspend and resume
Am Montag, den 01.07.2019, 10:17 -0400 schrieb Alan Stern: > On Mon, 1 Jul 2019, Oliver Neukum wrote: > > > Am Donnerstag, den 27.06.2019, 09:52 -0400 schrieb Alan Stern: > > > > > > > > Or maybe the WAIT_FOR_RESUME ioctl returns because there was a remote > > > wakeup. In this case also you would call FORBID_SUSPEND. > > > > > > In fact, you should call FORBID_SUSPEND _whenever_ WAIT_FOR_RESUME > > > > Well, this kind of indicates that the original syscall should bump > > the counter. > > Perhaps it does, but... > > > > returns, unless your program has decided not to use the device any more > > > (in which case you don't care whether the device is suspended or > > > resumed). > > > > Then you should close the device. > > Exactly. Suppose WAIT_FOR_RESUME is interrupted and then the program > closes the device. There's no need to force the device back to full > power in this situation. But that is the error case. You return an error code. The point of that is to report that a syscall did not have all requested effects. Regards Oliver
