[PATCH 4/6] usb: kbd: Ignore Yubikeys

[Marek Vasut]

On 2/27/24 8:13 AM, Janne Grunau wrote:
> Hej,
> 
> On Mon, Feb 26, 2024, at 21:47, Mark Kettenis wrote:
>>> Date: Sun, 25 Feb 2024 22:57:23 +0100
>>> From: Marek Vasut <marex at denx.de>
>>>
>>> On 2/25/24 5:07 PM, Janne Grunau wrote:
>>>> Hej,
>>>>
>>>> On Wed, Feb 21, 2024, at 13:41, Marek Vasut wrote:
>>>>> On 2/21/24 08:25, Janne Grunau via B4 Relay wrote:
>>>>>> From: Hector Martin <marcan at marcan.st>
>>>>>>
>>>>>> We currently only support one USB keyboard device, but some devices
>>>>>> emulate keyboards for other purposes. Most commonly, people run into
>>>>>> this with Yubikeys, so let's ignore those.
>>>>>>
>>>>>> Even if we end up supporting multiple keyboards in the future, it's
>>>>>> safer to ignore known non-keyboard devices.
>>>>>>
>>>>>> Signed-off-by: Hector Martin <marcan at marcan.st>
>>>>>> ---
>>>>>>     common/usb_kbd.c | 19 +++++++++++++++++++
>>>>>>     1 file changed, 19 insertions(+)
>>>>>>
>>>>>> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
>>>>>> index 4cbc9acb73..774d3555d9 100644
>>>>>> --- a/common/usb_kbd.c
>>>>>> +++ b/common/usb_kbd.c
>>>>>> @@ -120,6 +120,15 @@ struct usb_kbd_pdata {
>>>>>>     
>>>>>>     extern int __maybe_unused net_busy_flag;
>>>>>>     
>>>>>> +/*
>>>>>> + * Since we only support one usbkbd device in the iomux,
>>>>>> + * ignore common keyboard-emulating devices that aren't
>>>>>> + * real keyboards.
>>>>>> + */
>>>>>> +const uint16_t vid_blocklist[] = {
>>>>>> +	0x1050, /* Yubico */
>>>>>> +};
>>>>>> +
>>>>>>     /* The period of time between two calls of usb_kbd_testc(). */
>>>>>>     static unsigned long kbd_testc_tms;
>>>>>>     
>>>>>> @@ -465,6 +474,7 @@ static int usb_kbd_probe_dev(struct usb_device *dev, unsigned int ifnum)
>>>>>>     	struct usb_endpoint_descriptor *ep;
>>>>>>     	struct usb_kbd_pdata *data;
>>>>>>     	int epNum;
>>>>>> +	int i;
>>>>>>     
>>>>>>     	if (dev->descriptor.bNumConfigurations != 1)
>>>>>>     		return 0;
>>>>>> @@ -480,6 +490,15 @@ static int usb_kbd_probe_dev(struct usb_device *dev, unsigned int ifnum)
>>>>>>     	if (iface->desc.bInterfaceProtocol != USB_PROT_HID_KEYBOARD)
>>>>>>     		return 0;
>>>>>>     
>>>>>> +	for (i = 0; i < ARRAY_SIZE(vid_blocklist); i++) {
>>>>>> +		if (dev->descriptor.idVendor == vid_blocklist[i]) {
>>>>>> +			printf("Ignoring keyboard device 0x%x:0x%x\n",
>>>>>> +			       dev->descriptor.idVendor,
>>>>>> +			       dev->descriptor.idProduct);
>>>>>> +			return 0;
>>>>>> +		}
>>>>>> +	}
>>>>>
>>>>> I vaguely recall a discussion about previous version of this, I think
>>>>> the suggestion was to make the list of ignored devices configurable via
>>>>> environment variable, so users can add to that list from U-Boot shell.
>>>>> Would it be possible to make it work this way ?
>>>>
>>>> oh, I completely forgot that this patch was already submitted. I briefly looked through asahi tree for related patches and did not check whether this was previously submitted.
>>>> I've added environment based blocking as separate patch with blocking either complete vendor IDs or vendor, product ID combinations. A separate patch to simplify authorship tracking and the implementation doesn't share any code.
>>>
>>> It would be better to have only one patch which does not hard-code any
>>> USB IDs, and then add those blocked IDs via U-Boot default environment
>>> for this specific machine. We cannot predict what yubico will do in the
>>> future, whether they might make a device that shouldn't be blocked for
>>> example. If they do, the user should be able to unblock their device by
>>> running e.g. '=> setenv usb_blocklist' instead of updating their bootloader.
>>>
>>> I think a simple list of blocked VID:PID pairs, maybe with wildcards,
>>> would be nice, i.e. something like 'usb_blocklist=1234:5678,1050:*' to
>>> block device 0x1234:0x5678 and all devices with VID 0x1050 . That should
>>> be easy to parse with strtok()/strtol() or some such and the code should
>>> not be too complex.
>>
>> I do like the idea of having a configurable list of usb devices to
>> ignore.  The U-Boot USB stack is still not perfect and there are still
>> USB devices that will prevent us from booting when connected.  The
>> list will provide a nice workaround for that issue.
> 
> That sounds like we should ignore USB devices in usb_scan_device() and not in the keyboard driver.

Maybe that is even better and more generic solution ?

>> But the yubikeys will cause the same problem on other boards as well.
>> So I think it makes sense to put those in a default list.
> 
> We could move the list to a CONFIG symbol which has Yubikey's vendor ID as default value now that we do string parsing anyway.

There is include/env_default.h to define default env variable values too.