Re: [PATCH] usb-host: use correct altsetting in usb_host_ep_update

[Nick Rosbrook]

Hi,

Just wanted to ping this. Patchwork link is here:
20210201213021.500277-1-rosbrookn@ainfosec.com/">https://patchwork.kernel.org/project/qemu-devel/patch/20210201213021.500277-1-rosbrookn@ainfosec.com/.

Thanks,
NR

On Mon, Feb 1, 2021 at 4:30 PM Nick Rosbrook <rosbrookn@gmail.com> wrote:
>
> In order to keep track of the alternate setting that should be used for
> a given interface, the USBDevice struct keeps an array of alternate
> setting values, which is indexed by the interface number. In
> usb_host_set_interface, when this array is updated, usb_host_ep_update
> is called as a result. However, when usb_host_ep_update accesses the
> active libusb_config_descriptor, it indexes udev->altsetting with the
> loop variable, rather than the interface number.
>
> With the simple trace backend enable, this behavior can be seen:
>
>   [...]
>
>   usb_xhci_xfer_start 0.440 pid=1215 xfer=0x5596a4b85930 slotid=0x1 epid=0x1 
> streamid=0x0
>   usb_packet_state_change 1.703 pid=1215 bus=0x1 port=b'1' ep=0x0 
> p=0x5596a4b85938 o=b'undef' n=b'setup'
>   usb_host_req_control 2.269 pid=1215 bus=0x1 addr=0x5 p=0x5596a4b85938 
> req=0x10b value=0x1 index=0xd
>   usb_host_set_interface 0.449 pid=1215 bus=0x1 addr=0x5 interface=0xd alt=0x1
>   usb_host_parse_config 2542.648 pid=1215 bus=0x1 addr=0x5 value=0x2 
> active=0x1
>   usb_host_parse_interface 1.804 pid=1215 bus=0x1 addr=0x5 num=0xc alt=0x0 
> active=0x1
>   usb_host_parse_endpoint 2.012 pid=1215 bus=0x1 addr=0x5 ep=0x2 dir=b'in' 
> type=b'int' active=0x1
>   usb_host_parse_interface 1.598 pid=1215 bus=0x1 addr=0x5 num=0xd alt=0x0 
> active=0x1
>   usb_host_req_emulated 3.593 pid=1215 bus=0x1 addr=0x5 p=0x5596a4b85938 
> status=0x0
>   usb_packet_state_change 2.550 pid=1215 bus=0x1 port=b'1' ep=0x0 
> p=0x5596a4b85938 o=b'setup' n=b'complete'
>   usb_xhci_xfer_success 4.298 pid=1215 xfer=0x5596a4b85930 bytes=0x0
>
>   [...]
>
> In particular, it is seen that although usb_host_set_interface sets the
> alternate setting of interface 0xd to 0x1, usb_host_ep_update uses 0x0
> as the alternate setting due to using the incorrect index to
> udev->altsetting.
>
> Fix this problem by getting the interface number from the active
> libusb_config_descriptor, and then using that as the index to
> udev->altsetting.
>
> Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
> ---
>  hw/usb/host-libusb.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
> index fcf48c0193..6ab75e2feb 100644
> --- a/hw/usb/host-libusb.c
> +++ b/hw/usb/host-libusb.c
> @@ -810,7 +810,7 @@ static void usb_host_ep_update(USBHostDevice *s)
>      struct libusb_ss_endpoint_companion_descriptor *endp_ss_comp;
>  #endif
>      uint8_t devep, type;
> -    int pid, ep;
> +    int pid, ep, alt;
>      int rc, i, e;
>
>      usb_ep_reset(udev);
> @@ -822,8 +822,20 @@ static void usb_host_ep_update(USBHostDevice *s)
>                                  conf->bConfigurationValue, true);
>
>      for (i = 0; i < conf->bNumInterfaces; i++) {
> -        assert(udev->altsetting[i] < conf->interface[i].num_altsetting);
> -        intf = &conf->interface[i].altsetting[udev->altsetting[i]];
> +        /*
> +         * The udev->altsetting array indexes alternate settings
> +         * by the interface number. Get the 0th alternate setting
> +         * first so that we can grab the interface number, and
> +         * then correct the alternate setting value if necessary.
> +         */
> +        intf = &conf->interface[i].altsetting[0];
> +        alt = udev->altsetting[intf->bInterfaceNumber];
> +
> +        if (alt != 0) {
> +            assert(alt < conf->interface[i].num_altsetting);
> +            intf = &conf->interface[i].altsetting[alt];
> +        }
> +
>          trace_usb_host_parse_interface(s->bus_num, s->addr,
>                                         intf->bInterfaceNumber,
>                                         intf->bAlternateSetting, true);
> --
> 2.17.1
>