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

[Marek Vasut]

On 3/18/24 8:33 AM, Janne Grunau wrote:

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

In unix , 0 is considered success and non-zero failure .

How about this:

- Return -EINVAL here (parsing failed)
- Instead of 'return 1' in usb_device_is_blocked() return -ENODEV
- In usb_select_config(), check
   if usb_device_is_blocked returned 0, no device blocked OK
   if usb_device_is_blocked returned -ENODEV, device blocked,
     return -ENODEV
   if usb_device_is_blocked returned any other error, parsing error
     return that error

What do you think ?

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

OK, we can always improve this later, I would also like this 
functionality in soon-ish.