Re: [syzbot] [usb?] KASAN: slab-out-of-bounds Read in read_descriptors (3)

[Khazhy Kumykov]

On Tue, Jul 25, 2023 at 2:30 PM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx/#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

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature