[PATCH v2 4/6] usb: Add environment based device blocklist

[Janne Grunau]

On Mon, Mar 18, 2024, at 06:06, Marek Vasut wrote:
> On 3/17/24 7:15 PM, Janne Grunau wrote:
>> Hej,
>
> Hi,
>
>> On Sun, Mar 17, 2024, at 17:18, Marek Vasut wrote:
>>> On 3/17/24 12:07 PM, Janne Grunau via B4 Relay wrote:
>>>> From: Janne Grunau <j at jannau.net>
>>>>
>>>> Add the environment variable "usb_blocklist" to prevent USB devices
>>>> listed in it from being used. This allows to ignore devices which
>>>> trigger bugs in u-boot's USB stack or are undesirable for other reasons.
>>>> Devices emulating keyboards are one example. U-boot currently supports
>>>> only one USB keyboard device. Most commonly, people run into this with
>>>> Yubikeys, so let's ignore those in the default environment.
>>>>
>>>> Based on previous USB keyboard specific patches for the same purpose.
>>>>
>>>> Link: https://lore.kernel.org/u-boot/7ab604fb-0fec-4f5e-8708-7a3a7e2cb568@denx.de/
>>>> Signed-off-by: Janne Grunau <j at jannau.net>
>>>> ---
>>>>    common/usb.c              | 56 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>    doc/usage/environment.rst | 12 ++++++++++
>>>>    include/env_default.h     | 11 ++++++++++
>>>>    3 files changed, 79 insertions(+)
>>>>
>>>> diff --git a/common/usb.c b/common/usb.c
>>>> index 836506dcd9..73af5be066 100644
>>>> --- a/common/usb.c
>>>> +++ b/common/usb.c
>>>> @@ -1084,6 +1084,57 @@ static int usb_prepare_device(struct usb_device *dev, int addr, bool do_read,
>>>>    	return 0;
>>>>    }
>>>>    
>>>> +static int usb_blocklist_parse_error(const char *blocklist, size_t pos)
>>>> +{
>>>> +	printf("usb_blocklist parse error at char %zu in \"%s\"\n", pos,
>>>> +	       blocklist);
>>>> +	return 0;
>>>
>>> This could be static void without return 0 at the end.
>> 
>> the return is there to break out of the while loop on parsing errors in a single statement. This probably won't be necessary after using strsep and sscanf in the parsing function but see below.
>
> Ahh, now I see it. But then, shouldn't this return -ENODEV here already ?

It returns 0 so that parsing errors in the blocklist do not result
in blocking every USB device. That looked to me like the
less bad error behavior to me.

>>>> +}
>>>> +
>>>> +static int usb_device_is_blocked(u16 id_vendor, u16 id_product)
>>>> +{
>>>> +	ulong vid, pid;
>>>> +	char *end;
>>>> +	const char *block_str = env_get("usb_blocklist");
>>>> +	const char *cur = block_str;
>>>> +
>>>> +	/* parse "usb_blocklist" strictly */
>>>> +	while (cur && cur[0] != '\0') {
>>>
>>> Have a look at strsep() , namely strsep(block_str, ","); This will split
>>> the string up for you at "," delimiters.
>>>
>>> Example is in drivers/dfu/dfu.c dfu_config_interfaces() .
>> 
>> strsep() is probably a good idea even if it alone won't make the code that much simpler for strict parsing.
>> 
>>> And then, on each token, you can try and run sscanf(token, "%04x:%04x",
>>> vid, pid);, that will parse the token format for you. See e.g.
>>> test/lib/sscanf.c for further examples.
>>>
>>> That should simplify the parsing a lot.
>> 
>> It would but sscanf() is optional and is only selected by CONFIG_XEN so I assumed there would be concerns over binary size increase if USB_HOST would require sscanf.
>
> Good point, lets postpone sscanf() . Also, looking at it, sscanf would 
> work for numbers, but not for the special characters. So ... do you want 
> to try tweaking this further with strsep() or shall we go with this 
> implementation ?

I started converting it to strsep. It makes the intent clearer but it doesn't
simplify the implementation much. strsep() has the disadvantage that
it can't work in place and needs to copy the string. If we go with strsep
I would look into parsing the list once at USB init time and use a list of
numeric vendor, product ID pairs at device probe time.
If have a slight preference for the current implementation (with ignore
or deny instead of blocklist) as long as the list remains short.

Janne