linux-kernel - Re: [PATCH] usb: check for signals in chaoskey read function

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <86k2m343gd.fsf@hiro.keithp.com>
Date:	Wed, 17 Feb 2016 10:05:54 -0800
From:	Keith Packard <keithp@...thp.com>
To:	Oliver Neukum <oneukum@...e.com>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [PATCH] usb: check for signals in chaoskey read function

Oliver Neukum <oneukum@...e.com> writes:

> On Tue, 2016-02-16 at 11:09 -0800, Keith Packard wrote:
>> I could be convinced that the driver should be using a different path
>> through the USB stack that would allow a signal to wake up while
>> waiting
>> for the URB to complete, but this patch at least avoids needing to
>> wait
>> for a huge read to finish. The other option would be to eliminate the
>> loop reading multiple URBs from the device, but that would reduce the
>> available bandwidth from the device pretty considerably.
>
> Do these do the job?

Yup, with a few minor fixes to pass the right arguments:

diff --git a/drivers/usb/misc/chaoskey.c b/drivers/usb/misc/chaoskey.c
index 67102b4..76350e4 100644
--- a/drivers/usb/misc/chaoskey.c
+++ b/drivers/usb/misc/chaoskey.c
@@ -156,14 +156,14 @@ static int chaoskey_probe(struct usb_interface *interface,
 	if (dev->buf == NULL)
 		goto out;
 
-	dev->urb = usb_alloc_urb(GFP_KERNEL, 0);
+	dev->urb = usb_alloc_urb(0, GFP_KERNEL);
 
 	if (!dev->urb)
 		goto out;
 
 	usb_fill_bulk_urb(dev->urb,
 		udev,
-		usb_rcvbulkpipe(udev, altsetting->endpoint[in_ep].desc.bEndpointAddress),
+		usb_rcvbulkpipe(udev, in_ep),
 		dev->buf,
 		size,
 		chaos_read_callback,

The first patch also has the URB allocation, which should be in the
second patch. I removed the comment about 'more bandwidth' as the driver
is still synchronous and runs at the same speed as before.

Thanks very much for making this 'right', instead of just kludging it.

Here's a fixed sequence:


View attachment "0001-usb-misc-chaoskey-Cleanup-probe-failure-paths.patch" of type "text/x-diff" (2934 bytes)

View attachment "0002-usb-misc-chaoskey-introduce-an-URB-for-asynchronous-.patch" of type "text/x-diff" (4532 bytes)


-- 
-keith


Download attachment "signature.asc" of type "application/pgp-signature" (811 bytes)