linux-kernel - Re: [syzbot] [usb?] KASAN: slab-out-of-bounds Read in read

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACGdZY+qJ7P8FZj6ZGmcDkf2YH7LRBnfvuwiro4ZF37+owHo9g@mail.gmail.com>
Date:   Tue, 25 Jul 2023 14:46:37 -0700
From:   Khazhy Kumykov <khazhy@...gle.com>
To:     Alan Stern <stern@...land.harvard.edu>
Cc:     syzbot <syzbot+18996170f8096c6174d0@...kaller.appspotmail.com>,
        gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
        linux-usb@...r.kernel.org, syzkaller-bugs@...glegroups.com
Subject: Re: [syzbot] [usb?] KASAN: slab-out-of-bounds Read in
 read_descriptors (3)

On Tue, Jul 25, 2023 at 2:30 PM Alan Stern <stern@...land.harvard.edu> wrote:
>
> On Tue, Jul 25, 2023 at 01:42:01PM -0700, Khazhy Kumykov wrote:
> > On Tue, Jul 25, 2023 at 12:26 PM Alan Stern <stern@...land.harvard.edu> wrote:
>
> > > @@ -2671,12 +2671,17 @@ int usb_authorize_device(struct usb_devi
> > >         }
> > >
> > >         if (usb_dev->wusb) {
> > > -               result = usb_get_device_descriptor(usb_dev, sizeof(usb_dev->descriptor));
> > > -               if (result < 0) {
> > > +               struct usb_device_descriptor *descr;
> > > +
> > > +               descr = usb_get_device_descriptor(usb_dev);
> > > +               if (IS_ERR(descr)) {
> > > +                       result = PTR_ERR(descr);
> > >                         dev_err(&usb_dev->dev, "can't re-read device descriptor for "
> > >                                 "authorization: %d\n", result);
> > >                         goto error_device_descriptor;
> > >                 }
> > > +               usb_dev->descriptor = *descr;
> > Hmm, in your first patch you rejected diffs to the descriptor here,
> > which might be necessary - since we don't re-initialize the device so
> > can get a similar issue if the bad usb device decides to change
> > bNumConfigurations to cause a buffer overrun. (This also stuck out to
> > me as an exception to the "descriptors should be immutable" comment
> > later in the patch.
>
> I removed that part of the previous patch because there's no point to
> it.  None of this code ever gets executed; the usb_dev->wusb test can
> never succeed because the kernel doesn't support wireless USB any more.
> (I asked Greg KH about that in a separate email thread:
> <https://lore.kernel.org/linux-usb/2a21cefa-99a7-497c-901f-3ea097361a80@rowland.harvard.edu/#r>.)
>
> A later patch will remove all of the wireless USB stuff.  The only real
> reason for leaving this much of the code there now is to prevent
> compilation errors and keep the form looking right.
Ah ok, cool.

>
> > > @@ -6018,7 +6064,7 @@ static int usb_reset_and_verify_device(s
> > >                 /* ep0 maxpacket size may change; let the HCD know about it.
> > >                  * Other endpoints will be handled by re-enumeration. */
> > >                 usb_ep0_reinit(udev);
> > > -               ret = hub_port_init(parent_hub, udev, port1, i);
> > > +               ret = hub_port_init(parent_hub, udev, port1, i, &descriptor);
> > Looks like this is the only caller that passes &descriptor, and it
> > just checks that it didn't change. Would it make sense to put the
> > enitre descriptors_changed stanza in hub_port_init, for the re-init
> > case?
>
> The descriptors_changed check has to be _somewhere_, either here or
> there.  I don't see what difference it makes whether the check is done
> in this routine or in hub_port_init.  Since it doesn't matter, why
> change the existing code?
No strong feelings, but it lets us remove the variable in
usb_reset_and_verify_device() and directly check on the malloc'd copy,
instead of copying back up to here.

Overall, looks good to my naive eyes.

CVE-2023-37453 was filed for this syzbot report, I'm not sure how that
system gets tracked, but might be good to mention for folks looking
for a fix.

Thanks,
Khazhy

Download attachment "smime.p7s" of type "application/pkcs7-signature" (3999 bytes)