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

["Janne Grunau" <j@jannau.net>]

Hej,

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@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@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.

>> +}
>> +
>> +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.

Janne