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
