| Message ID | 20241119142813.3528722-1-bsevens@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] ALSA: usb-audio: Fix potential out-of-bound accesses for Extigy devices | expand |
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@kernel.org > Signed-off-by: Benoît Sevens <bsevens@google.com> > --- > 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. Takashi
Hi Takashi, On Tue, 19 Nov 2024 at 15:43, Takashi Iwai <tiwai@suse.de> 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@kernel.org > > Signed-off-by: Benoît Sevens <bsevens@google.com> > > --- > > 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
diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c index c5fd180357d1..919091c1f17a 100644 --- a/sound/usb/quirks.c +++ b/sound/usb/quirks.c @@ -555,6 +555,7 @@ int snd_usb_create_quirk(struct snd_usb_audio *chip, static int snd_usb_extigy_boot_quirk(struct usb_device *dev, struct usb_interface *intf) { struct usb_host_config *config = dev->actconfig; + int num_configs; int err; if (le16_to_cpu(get_cfg_desc(config)->wTotalLength) == EXTIGY_FIRMWARE_SIZE_OLD || @@ -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; config = dev->actconfig; if (err < 0) dev_dbg(&dev->dev, "error usb_get_descriptor: %d\n", err); @@ -901,6 +905,7 @@ static void mbox2_setup_48_24_magic(struct usb_device *dev) static int snd_usb_mbox2_boot_quirk(struct usb_device *dev) { struct usb_host_config *config = dev->actconfig; + int num_configs; int err; u8 bootresponse[0x12]; int fwsize; @@ -935,8 +940,11 @@ static int snd_usb_mbox2_boot_quirk(struct usb_device *dev) dev_dbg(&dev->dev, "device initialised!\n"); + 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; config = dev->actconfig; if (err < 0) dev_dbg(&dev->dev, "error usb_get_descriptor: %d\n", err); @@ -1249,6 +1257,7 @@ static void mbox3_setup_defaults(struct usb_device *dev) static int snd_usb_mbox3_boot_quirk(struct usb_device *dev) { struct usb_host_config *config = dev->actconfig; + int num_configs; int err; int descriptor_size; @@ -1261,8 +1270,11 @@ static int snd_usb_mbox3_boot_quirk(struct usb_device *dev) dev_dbg(&dev->dev, "MBOX3: device initialised!\n"); + 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; config = dev->actconfig; if (err < 0) dev_dbg(&dev->dev, "MBOX3: error usb_get_descriptor: %d\n", err);
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@kernel.org Signed-off-by: Benoît Sevens <bsevens@google.com> --- v2: - Also add sanity checks to snd_usb_mbox2_boot_quirk and snd_usb_mbox3_boot_quirk --- sound/usb/quirks.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)