Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <000000000000b580440591ac8df5@google.com> <Pine.LNX.4.44L0.1909041038340.1722-100000@iolanthe.rowland.org>
In-Reply-To: <Pine.LNX.4.44L0.1909041038340.1722-100000@iolanthe.rowland.org>
From:   Andrey Konovalov <andreyknvl@google.com>
Date:   Wed, 4 Sep 2019 17:01:17 +0200
Message-ID: <CAAeHK+xegKOayZw+kvw7ndA4v6Fy77rNM_VQnufZWXEHSjoqhg@mail.gmail.com>
Subject: Re: KASAN: slab-out-of-bounds Read in usb_reset_and_verify_device
To:     Alan Stern <stern@rowland.harvard.edu>
Cc:     syzbot <syzbot+35f4d916c623118d576e@syzkaller.appspotmail.com>,
        Thinh.Nguyen@synopsys.com, dianders@chromium.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        jflat@chromium.org, Kai Heng Feng <kai.heng.feng@canonical.com>,
        LKML <linux-kernel@vger.kernel.org>,
        USB list <linux-usb@vger.kernel.org>, malat@debian.org,
        mathias.nyman@linux.intel.com, nsaenzjulienne@suse.de,
        syzkaller-bugs <syzkaller-bugs@googlegroups.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Sep 4, 2019 at 4:41 PM Alan Stern <stern@rowland.harvard.edu> 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;
>
>