| Message ID | 20241115130833.1077260-1-bsevens@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ALSA: usb-audio: Fix potential out-of-bound accesses for Extigy devices | expand |
On Fri, 15 Nov 2024 14:08:33 +0100, Benoit 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. It'd be more helpful if you can elaborate how it can happen; the code you touched doesn't call much stuff, just send a boot sequence and reset the configuration. Yet your patch tries to overwrite a usb_device field, which isn't really a task for the driver side. thanks, Takashi > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > Cc: stable@kernel.org > Signed-off-by: Benoit Sevens <bsevens@google.com> > --- > sound/usb/quirks.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c > index c5fd180357d1..970d36d13ad8 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); > -- > 2.47.0.338.g60cca15819-goog >
On Sun, 17 Nov 2024 at 15:00, Takashi Iwai <tiwai@suse.de> wrote: > > On Fri, 15 Nov 2024 14:08:33 +0100, > Benoit 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. > > It'd be more helpful if you can elaborate how it can happen; the code > you touched doesn't call much stuff, just send a boot sequence and > reset the configuration. Yet your patch tries to overwrite a > usb_device field, which isn't really a task for the driver side. I am touching a field of the usb_device->descriptor, which the current code is already doing in usb_get_descriptor(). A malicious device can use the snd_usb_extigy_boot_quirk() quirk to provide a new device descriptor with an increased bNumConfigurations, after dev->config has already been allocated for a smaller bNumConfigurations. This will lead to out of bounds accesses later on, such as freeing out-of-bounds pointers in usb_destroy_configuration. Actually, I noticed snd_usb_mbox2_boot_quirk and snd_usb_mbox3_boot_quirk can probably be abused in the same way, so I'll propose a new patch that also does a check there. Unless anyone knows of a better/cleaner way to fix this? > > > thanks, > > Takashi > > > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > > Cc: stable@kernel.org > > Signed-off-by: Benoit Sevens <bsevens@google.com> > > --- > > sound/usb/quirks.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c > > index c5fd180357d1..970d36d13ad8 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); > > -- > > 2.47.0.338.g60cca15819-goog > > Thanks, Benoit
diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c index c5fd180357d1..970d36d13ad8 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);
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: Benoit Sevens <bsevens@google.com> --- sound/usb/quirks.c | 4 ++++ 1 file changed, 4 insertions(+)