[SeaBIOS] Re: [SEABIOS] [PATCH] usb-hid: Fix keyboards using non-primary interface descriptor

[Kevin O'Connor]

On Thu, Sep 03, 2020 at 09:03:50PM -0500, Matt DeVillier wrote:
> On Thu, Sep 3, 2020 at 7:28 PM Kevin O'Connor  wrote:
> > If we're going to support multiple interfaces, I think it would be
> > preferable to expand the loop so that it also works for MASS_STORAGE
> > and HUB devices.
> >
> > Also, if an alternate interface is used, I think the usb SET_INTERFACE
> > command needs to be sent.  (That particular keyboard may work without
> > SET_INTERFACE, but it may not work on another.)
>
> how about something like:
>
> int i;
> ret = -1;
> for (i=0; i<config->bNumInterfaces; i++) {
>     if (iface->bInterfaceClass == USB_CLASS_HUB)
>         ret = usb_hub_setup(usbdev);
>     else if (iface->bInterfaceClass == USB_CLASS_MASS_STORAGE) {
>         if (iface->bInterfaceProtocol == US_PR_BULK)
>             ret = usb_msc_setup(usbdev);
>         else if (iface->bInterfaceProtocol == US_PR_UAS)
>             ret = usb_uas_setup(usbdev);
>     } else
>         ret = usb_hid_setup(usbdev);
> if (ret == 0)
>     break;
>
> iface = (void*)iface + iface->bLength;
> usb_set_interface(iface);  //need to create this
> usbdev->iface = iface;
>
> }

Wouldn't it be better to run the check before calling set_configure()?

Perhaps something like the below (totally untested).

-Kevin

diff --git a/src/hw/usb.c b/src/hw/usb.c
index 4f9a852..732d4cd 100644
--- a/src/hw/usb.c
+++ b/src/hw/usb.c
@@ -248,14 +248,14 @@ get_device_config(struct usb_pipe *pipe)
     if (ret)
         return NULL;

-    void *config = malloc_tmphigh(cfg.wTotalLength);
+    struct usb_config_descriptor *config = malloc_tmphigh(cfg.wTotalLength);
     if (!config) {
         warn_noalloc();
         return NULL;
     }
     req.wLength = cfg.wTotalLength;
     ret = usb_send_default_control(pipe, &req, config);
-    if (ret) {
+    if (ret || config->wTotalLength != cfg.wTotalLength) {
         free(config);
         return NULL;
     }
@@ -367,19 +367,33 @@ configure_usb_device(struct usbdevice_s *usbdev)
         return 0;

// Determine if a driver exists for this device - only look at the
-    // first interface of the first configuration.
+    // interfaces of the first configuration.
+    int num_iface = config->bNumInterfaces;
+    void *config_end = (void*)config + config->wTotalLength;
     struct usb_interface_descriptor *iface = (void*)(&config[1]);
-    if (iface->bInterfaceClass != USB_CLASS_HID
-        && iface->bInterfaceClass != USB_CLASS_MASS_STORAGE
-        && iface->bInterfaceClass != USB_CLASS_HUB)
-        // Not a supported device.
-        goto fail;
+    for (;;) {
+        if (!num_iface-- || (void*)iface + iface->bLength >= config_end)
+            // Not a supported device.
+            goto fail;
+        if (iface->bInterfaceClass == USB_CLASS_HUB
+            || (iface->bInterfaceClass == USB_CLASS_MASS_STORAGE
+                && (iface->bInterfaceProtocol == US_PR_BULK
+                    || iface->bInterfaceProtocol == US_PR_UAS))
+            || (iface->bInterfaceClass == USB_CLASS_HID
+                && iface->bInterfaceSubClass == USB_INTERFACE_SUBCLASS_BOOT))
+            break;
+        iface = (void*)iface + iface->bLength;
+    }

// Set the configuration.
     ret = set_configuration(usbdev->defpipe, config->bConfigurationValue);
     if (ret)
         goto fail;

+    if (iface->bAlternateSetting)
+        // XXX
+        set_interface(iface);
+
     // Configure driver.
     usbdev->config = config;
     usbdev->iface = iface;