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

[Alan Stern]

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.

> > @@ -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?

Alan Stern