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

[Greg KH]

On Tue, Jul 25, 2023 at 02:46:37PM -0700, Khazhy Kumykov wrote:
> 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.

Who filed CVEs for random syzbot reports?  That's crazy, and no, we
aren't going to track it as CVEs mean nothing for the kernel as I've
said many times.

thanks,

greg k-h