Re: KASAN: slab-out-of-bounds Read in usb_reset_and_verify_device

[Andrey Konovalov]

On Wed, Sep 4, 2019 at 4:41 PM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Tue, 3 Sep 2019, syzbot wrote:
>
> > Hello,
> >
> > syzbot has tested the proposed patch but the reproducer still triggered
> > crash:
> > KASAN: slab-out-of-bounds Read in usb_reset_and_verify_device
> >
> > usb 6-1: Using ep0 maxpacket: 16
> > usb 6-1: BOS total length 54, descriptor 168
> > usb 6-1: Old BOS ffff8881cd814f60  Len 0xa8
> > usb 6-1: New BOS ffff8881cd257ae0  Len 0xa8
> > ==================================================================
> > BUG: KASAN: slab-out-of-bounds in memcmp+0xa6/0xb0 lib/string.c:904
> > Read of size 1 at addr ffff8881cd257c36 by task kworker/1:0/17
>
> Very sneaky!  A BOS descriptor whose wTotalLength field varies
> depending on how many bytes you read.
>
> This should fix it.  It's the same approach we use for the Config
> descriptor.

Nice, core USB bug :)

Can this potentially lead to something worse than a out-of-bounds memcmp?

>
> Alan Stern
>
> #syz test: https://github.com/google/kasan.git eea39f24
>
>  drivers/usb/core/config.c |   12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> Index: usb-devel/drivers/usb/core/config.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/core/config.c
> +++ usb-devel/drivers/usb/core/config.c
> @@ -921,7 +921,7 @@ int usb_get_bos_descriptor(struct usb_de
>         struct usb_bos_descriptor *bos;
>         struct usb_dev_cap_header *cap;
>         struct usb_ssp_cap_descriptor *ssp_cap;
> -       unsigned char *buffer;
> +       unsigned char *buffer, *buffer0;
>         int length, total_len, num, i, ssac;
>         __u8 cap_type;
>         int ret;
> @@ -966,10 +966,12 @@ int usb_get_bos_descriptor(struct usb_de
>                         ret = -ENOMSG;
>                 goto err;
>         }
> +
> +       buffer0 = buffer;
>         total_len -= length;
> +       buffer += length;
>
>         for (i = 0; i < num; i++) {
> -               buffer += length;
>                 cap = (struct usb_dev_cap_header *)buffer;
>
>                 if (total_len < sizeof(*cap) || total_len < cap->bLength) {
> @@ -983,8 +985,6 @@ int usb_get_bos_descriptor(struct usb_de
>                         break;
>                 }
>
> -               total_len -= length;
> -
>                 if (cap->bDescriptorType != USB_DT_DEVICE_CAPABILITY) {
>                         dev_warn(ddev, "descriptor type invalid, skip\n");
>                         continue;
> @@ -1019,7 +1019,11 @@ int usb_get_bos_descriptor(struct usb_de
>                 default:
>                         break;
>                 }
> +
> +               total_len -= length;
> +               buffer += length;
>         }
> +       dev->bos->desc->wTotalLength = cpu_to_le16(buffer - buffer0);
>
>         return 0;
>
>