| 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
| ||
|
Message-Id: <FADFC390-CC1B-4418-AF21-C831C98090B2@canonical.com>
Date: Thu, 7 Dec 2017 15:06:18 +0800
From: Kai Heng Feng <kai.heng.feng@...onical.com>
To: Greg KH <gregkh@...uxfoundation.org>
Cc: corbet@....net, linux-usb@...r.kernel.org,
linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] usb: core: Add "quirks" parameter for usbcore
> On 6 Dec 2017, at 10:10 PM, Greg KH <gregkh@...uxfoundation.org> wrote:
>
> On Wed, Dec 06, 2017 at 06:26:21PM +0800, Kai-Heng Feng wrote:
>> Trying quirks in usbcore needs to rebuild the driver or the entire
>> kernel if it's builtin. It can save a lot of time if usbcore has similar
>> ability like "usbhid.quirks=" and "usb-storage.quirks=".
>>
>> Rename the original quirk detection function to "static" as we introduce
>> this new "dynamic" function.
>>
>> Now users can use "usbcore.quirks=" as short term workaround before the
>> next kernel release.
>>
>> This is inspired by usbhid and usb-storage.
>>
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@...onical.com>
>> ---
>> v2: use in-kernel tolower() function.
>>
>> Documentation/admin-guide/kernel-parameters.txt | 55 +++++++++++++
>> drivers/usb/core/quirks.c | 100 ++++++++++++++++++++++--
>> include/linux/usb/quirks.h | 2 +
>> 3 files changed, 151 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index 6571fbfdb2a1..d42fb278320b 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -4302,6 +4302,61 @@
>>
>> usbcore.nousb [USB] Disable the USB subsystem
>>
>> + usbcore.quirks=
>> + [USB] A list of quirks entries to supplement or
>> + override the built-in usb core quirk list. List
>> + entries are separated by commas. Each entry has
>> + the form VID:PID:Flags where VID and PID are Vendor
>> + and Product ID values (4-digit hex numbers) and
>> + Flags is a set of characters, each corresponding
>> + to a common usb core quirk flag as follows:
>> + a = USB_QUIRK_STRING_FETCH_255 (string
>> + descriptors must not be fetched using
>> + a 255-byte read);
>> + b = USB_QUIRK_RESET_RESUME (device can't resume
>> + correctly so reset it instead);
>> + c = USB_QUIRK_NO_SET_INTF (device can't handle
>> + Set-Interface requests);
>> + d = USB_QUIRK_CONFIG_INTF_STRINGS (device can't
>> + handle its Configuration or Interface
>> + strings);
>> + e = USB_QUIRK_RESET (device can't be reset
>> + (e.g morph devices), don't use reset);
>> + f = USB_QUIRK_HONOR_BNUMINTERFACES (device has
>> + more interface descriptions than the
>> + bNumInterfaces count, and can't handle
>> + talking to these interfaces);
>> + g = USB_QUIRK_DELAY_INIT (device needs a pause
>> + during initialization, after we read
>> + the device descriptor);
>> + h = USB_QUIRK_LINEAR_UFRAME_INTR_BINTERVAL (For
>> + high speed and super speed interrupt
>> + endpoints, the USB 2.0 and USB 3.0 spec
>> + require the interval in microframes (1
>> + microframe = 125 microseconds) to be
>> + calculated as interval = 2 ^
>> + (bInterval-1).
>> + Devices with this quirk report their
>> + bInterval as the result of this
>> + calculation instead of the exponent
>> + variable used in the calculation);
>> + i = USB_QUIRK_DEVICE_QUALIFIER (device can't
>> + handle device_qualifier descriptor
>> + requests);
>> + j = USB_QUIRK_IGNORE_REMOTE_WAKEUP (device
>> + generates spurious wakeup, ignore
>> + remote wakeup capability);
>> + k = USB_QUIRK_NO_LPM (device can't handle Link
>> + Power Management);
>> + l = USB_QUIRK_LINEAR_FRAME_INTR_BINTERVAL
>> + (Device reports its bInterval as linear
>> + frames instead of the USB 2.0
>> + calculation);
>> + m = USB_QUIRK_DISCONNECT_SUSPEND (Device needs
>> + to be disconnected before suspend to
>> + prevent spurious wakeup)
>> + Example: quirks=0781:5580:bk,0a5c:5834:gij
>> +
>> usbhid.mousepoll=
>> [USBHID] The interval which mice are to be polled at.
>>
>> diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
>> index f1dbab6f798f..5471765765be 100644
>> --- a/drivers/usb/core/quirks.c
>> +++ b/drivers/usb/core/quirks.c
>> @@ -11,6 +11,12 @@
>> #include <linux/usb/hcd.h>
>> #include "usb.h"
>>
>> +/* Quirks specified at module load time */
>> +static char *quirks_param[MAX_USB_BOOT_QUIRKS];
>> +module_param_array_named(quirks, quirks_param, charp, NULL, 0444);
>
> So you can't modify this at runtime? Why not?
Sure, I think make it runtime tunable is a good idea.
>
>> +MODULE_PARM_DESC(quirks, "Add/modify USB quirks by specifying"
>> + "quirks=vendorID:productID:quirks");
>
> Don't break strings over multiple lines.
Will fix in next version.
>
>> +
>> /* Lists of quirky USB devices, split in device quirks and interface quirks.
>> * Device quirks are applied at the very beginning of the enumeration process,
>> * right after reading the device descriptor. They can thus only match on device
>> @@ -310,8 +316,8 @@ static int usb_amd_resume_quirk(struct usb_device *udev)
>> return 0;
>> }
>>
>> -static u32 __usb_detect_quirks(struct usb_device *udev,
>> - const struct usb_device_id *id)
>> +static u32 usb_detect_static_quirks(struct usb_device *udev,
>> + const struct usb_device_id *id)
>> {
>> u32 quirks = 0;
>>
>> @@ -329,23 +335,105 @@ static u32 __usb_detect_quirks(struct usb_device *udev,
>> return quirks;
>> }
>>
>> +static u32 usb_detect_dynamic_quirks(struct usb_device *udev)
>> +{
>> + u16 dev_vid = le16_to_cpu(udev->descriptor.idVendor);
>> + u16 dev_pid = le16_to_cpu(udev->descriptor.idProduct);
>> + u16 quirk_vid, quirk_pid;
>> + char quirks[32];
>> + char *p;
>> + u32 flags = 0;
>> + int n, m;
>> +
>> + for (n = 0; n < MAX_USB_BOOT_QUIRKS && quirks_param[n]; n++) {
>> + m = sscanf(quirks_param[n], "%hx:%hx:%s",
>> + &quirk_vid, &quirk_pid, quirks);
>> + if (m != 3)
>> + pr_warn("Could not parse USB quirk module param %s\n",
>> + quirks_param[n]);
>
> dev_warn().
>
> Also, you are going to complain about the inability to parse the quirks
> for _EVERY_ USB device in the system? That feels really wrong. Parse
> it once, and then use the parsed table when matching things up. Don't
> parse the string each and every time a device is added, that's a
> horrible waste of time (not like USB enumeration is fast, but really,
> let's not make it worse for no reason.)
Makes sense. Will improve this in next version.
>
>> +
>> + if (quirk_vid == dev_vid && quirk_pid == dev_pid)
>> + break;
>> + }
>> + if (n == MAX_USB_BOOT_QUIRKS || !quirks_param[n])
>> + return 0;
>> +
>> + p = quirks;
>> +
>> + /* Collect the flags */
>> + for (; *p; p++) {
>> + switch (tolower(*p)) {
>> + case 'a':
>> + flags |= USB_QUIRK_STRING_FETCH_255;
>> + break;
>> + case 'b':
>> + flags |= USB_QUIRK_RESET_RESUME;
>> + break;
>> + case 'c':
>> + flags |= USB_QUIRK_NO_SET_INTF;
>> + break;
>> + case 'd':
>> + flags |= USB_QUIRK_CONFIG_INTF_STRINGS;
>> + break;
>> + case 'e':
>> + flags |= USB_QUIRK_RESET;
>> + break;
>> + case 'f':
>> + flags |= USB_QUIRK_HONOR_BNUMINTERFACES;
>> + break;
>> + case 'g':
>> + flags |= USB_QUIRK_DELAY_INIT;
>> + break;
>> + case 'h':
>> + flags |= USB_QUIRK_LINEAR_UFRAME_INTR_BINTERVAL;
>> + break;
>> + case 'i':
>> + flags |= USB_QUIRK_DEVICE_QUALIFIER;
>> + break;
>> + case 'j':
>> + flags |= USB_QUIRK_IGNORE_REMOTE_WAKEUP;
>> + break;
>> + case 'k':
>> + flags |= USB_QUIRK_NO_LPM;
>> + break;
>> + case 'l':
>> + flags |= USB_QUIRK_LINEAR_FRAME_INTR_BINTERVAL;
>> + break;
>> + case 'm':
>> + flags |= USB_QUIRK_DISCONNECT_SUSPEND;
>> + break;
>> + /* Ignore unrecognized flag characters */
>> + }
>> + }
>> +
>> + return flags;
>> +}
>> +
>> /*
>> * Detect any quirks the device has, and do any housekeeping for it if needed.
>> */
>> void usb_detect_quirks(struct usb_device *udev)
>> {
>> - udev->quirks = __usb_detect_quirks(udev, usb_quirk_list);
>> + udev->quirks = usb_detect_dynamic_quirks(udev);
>> +
>> + if (udev->quirks) {
>> + dev_dbg(&udev->dev, "Dynamic USB quirks for this device: %x\n",
>> + udev->quirks);
>> + return;
>> + }
>> +
>> + udev->quirks = usb_detect_static_quirks(udev, usb_quirk_list);
>
> Did you just overwrite the dynamic with the static ones? It feels like
> you just did :(
That’s my original intention. I want to let users can override quirk, in
case the quirk somehow regressed their device.
>
>> --- a/include/linux/usb/quirks.h
>> +++ b/include/linux/usb/quirks.h
>> @@ -8,6 +8,8 @@
>> #ifndef __LINUX_USB_QUIRKS_H
>> #define __LINUX_USB_QUIRKS_H
>>
>> +#define MAX_USB_BOOT_QUIRKS 4
>
> Why 4? And why in this .h file?
It’s from usbhid. I think I’ll make it 16 next version.
I’ll put it in the .c file.
>
> Also, Oliver's request is good, xor is a nice way to do things if at all
> possible.
Yea, I think Oliver’s suggestion is great. Will do in next version.
>
> thanks,
>
> greg k-h
Powered by blists - more mailing lists