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

[Kevin O'Connor]

On Thu, Sep 03, 2020 at 10:32:24PM -0400, Kevin O'Connor wrote:
> 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

Looking a little closer at this, it's also necessary to verify that
the descriptor is an interface - so something like:

if (iface->bDescriptorType == USB_DT_INTERFACE
    && ...)
    break;

-Kevin
>        || (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;