Re: [PATCH v2] ALSA: usb-audio: Fix potential out-of-bound accesses for Extigy devices

[Benoît Sevens <bsevens@xxxxxxxxxx>]

Hi Takashi,

On Tue, 19 Nov 2024 at 15:43, Takashi Iwai <tiwai@xxxxxxx> wrote:
>
> On Tue, 19 Nov 2024 15:28:13 +0100,
> Benoît Sevens wrote:
> >
> > A bogus devices can provide a bNumConfigurations value that exceeds the
> > initial value used in usb_get_configuration for allocating dev->config.
> >
> > This can lead to out-of-bounds accesses later, e.g. in
> > usb_destroy_configuration.
> >
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Cc: stable@xxxxxxxxxx
> > Signed-off-by: Benoît Sevens <bsevens@xxxxxxxxxx>
> > ---
> > v2:
> >  - Also add sanity checks to snd_usb_mbox2_boot_quirk and
> >    snd_usb_mbox3_boot_quirk
>
> Thanks for the revised patch.
>
> Although we should fix better in a long term, a duct-tape fix like
> this wouldn't hurt.
>
> But one thing I'm considering is...
>
> > @@ -565,8 +566,11 @@ static int snd_usb_extigy_boot_quirk(struct usb_device *dev, struct usb_interfac
> >                                     0x10, 0x43, 0x0001, 0x000a, NULL, 0);
> >               if (err < 0)
> >                       dev_dbg(&dev->dev, "error sending boot message: %d\n", err);
> > +             num_configs = dev->descriptor.bNumConfigurations;
> >               err = usb_get_descriptor(dev, USB_DT_DEVICE, 0,
> >                               &dev->descriptor, sizeof(dev->descriptor));
> > +             if (dev->descriptor.bNumConfigurations > num_configs)
> > +                     dev->descriptor.bNumConfigurations = num_configs;
>
> IMO, we can get the new descriptor locally, then do
> compare-and-store.  If bNumConfiguration is unexpectedly large, it can
> return an error without modifying the original descriptor.
>

That sounds like a good idea. I'll send a revised patch.

>
> Takashi

Thank you,

Benoit