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

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

[Benoit Sevens]

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(+)

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

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

[Takashi Iwai]

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
> 

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

[Benoît Sevens]

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