| Message ID | 60e3aa09-039d-46d2-934c-6f123026c2eb@stanley.mountain (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ALSA: usb-audio: Fix a DMA to stack memory bug | expand |
Hi Dan, On Mon, 2 Dec 2024 at 13:57, Dan Carpenter <dan.carpenter@linaro.org> wrote: > > The usb_get_descriptor() function does DMA so we're not allowed > to use a stack buffer for that. Doing DMA to the stack is not portable > all architectures. Move the "new_device_descriptor" from being stored > on the stack and allocate it with kmalloc() instead. > Thanks for fixing this. It looks good to me. Note that the commit that is being fixed is already queued for backporting, so I don't know how this usually goes then. > Fixes: b909df18ce2a ("ALSA: usb-audio: Fix potential out-of-bound accesses for Extigy and Mbox devices") > Cc: stable@kernel.org > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > sound/usb/quirks.c | 42 +++++++++++++++++++++++++++--------------- > 1 file changed, 27 insertions(+), 15 deletions(-) > > diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c > index 8bc959b60be3..7c9d352864da 100644 > --- a/sound/usb/quirks.c > +++ b/sound/usb/quirks.c > @@ -555,7 +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; > - struct usb_device_descriptor new_device_descriptor; > + struct usb_device_descriptor *new_device_descriptor __free(kfree) = NULL; > int err; > > if (le16_to_cpu(get_cfg_desc(config)->wTotalLength) == EXTIGY_FIRMWARE_SIZE_OLD || > @@ -566,15 +566,19 @@ 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); > + > + new_device_descriptor = kmalloc(sizeof(*new_device_descriptor), GFP_KERNEL); > + if (!new_device_descriptor) > + return -ENOMEM; > err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, > - &new_device_descriptor, sizeof(new_device_descriptor)); > + new_device_descriptor, sizeof(*new_device_descriptor)); > if (err < 0) > dev_dbg(&dev->dev, "error usb_get_descriptor: %d\n", err); > - if (new_device_descriptor.bNumConfigurations > dev->descriptor.bNumConfigurations) > + if (new_device_descriptor->bNumConfigurations > dev->descriptor.bNumConfigurations) > dev_dbg(&dev->dev, "error too large bNumConfigurations: %d\n", > - new_device_descriptor.bNumConfigurations); > + new_device_descriptor->bNumConfigurations); > else > - memcpy(&dev->descriptor, &new_device_descriptor, sizeof(dev->descriptor)); > + memcpy(&dev->descriptor, new_device_descriptor, sizeof(dev->descriptor)); > err = usb_reset_configuration(dev); > if (err < 0) > dev_dbg(&dev->dev, "error usb_reset_configuration: %d\n", err); > @@ -906,7 +910,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; > - struct usb_device_descriptor new_device_descriptor; > + struct usb_device_descriptor *new_device_descriptor __free(kfree) = NULL; > int err; > u8 bootresponse[0x12]; > int fwsize; > @@ -941,15 +945,19 @@ static int snd_usb_mbox2_boot_quirk(struct usb_device *dev) > > dev_dbg(&dev->dev, "device initialised!\n"); > > + new_device_descriptor = kmalloc(sizeof(*new_device_descriptor), GFP_KERNEL); > + if (!new_device_descriptor) > + return -ENOMEM; > + > err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, > - &new_device_descriptor, sizeof(new_device_descriptor)); > + new_device_descriptor, sizeof(*new_device_descriptor)); > if (err < 0) > dev_dbg(&dev->dev, "error usb_get_descriptor: %d\n", err); > - if (new_device_descriptor.bNumConfigurations > dev->descriptor.bNumConfigurations) > + if (new_device_descriptor->bNumConfigurations > dev->descriptor.bNumConfigurations) > dev_dbg(&dev->dev, "error too large bNumConfigurations: %d\n", > - new_device_descriptor.bNumConfigurations); > + new_device_descriptor->bNumConfigurations); > else > - memcpy(&dev->descriptor, &new_device_descriptor, sizeof(dev->descriptor)); > + memcpy(&dev->descriptor, new_device_descriptor, sizeof(dev->descriptor)); > > err = usb_reset_configuration(dev); > if (err < 0) > @@ -1259,7 +1267,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; > - struct usb_device_descriptor new_device_descriptor; > + struct usb_device_descriptor *new_device_descriptor __free(kfree) = NULL; > int err; > int descriptor_size; > > @@ -1272,15 +1280,19 @@ static int snd_usb_mbox3_boot_quirk(struct usb_device *dev) > > dev_dbg(&dev->dev, "MBOX3: device initialised!\n"); > > + new_device_descriptor = kmalloc(sizeof(*new_device_descriptor), GFP_KERNEL); > + if (!new_device_descriptor) > + return -ENOMEM; > + > err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, > - &new_device_descriptor, sizeof(new_device_descriptor)); > + new_device_descriptor, sizeof(*new_device_descriptor)); > if (err < 0) > dev_dbg(&dev->dev, "MBOX3: error usb_get_descriptor: %d\n", err); > - if (new_device_descriptor.bNumConfigurations > dev->descriptor.bNumConfigurations) > + if (new_device_descriptor->bNumConfigurations > dev->descriptor.bNumConfigurations) > dev_dbg(&dev->dev, "MBOX3: error too large bNumConfigurations: %d\n", > - new_device_descriptor.bNumConfigurations); > + new_device_descriptor->bNumConfigurations); > else > - memcpy(&dev->descriptor, &new_device_descriptor, sizeof(dev->descriptor)); > + memcpy(&dev->descriptor, new_device_descriptor, sizeof(dev->descriptor)); > > err = usb_reset_configuration(dev); > if (err < 0) > -- > 2.45.2 >
On Mon, 02 Dec 2024 16:05:01 +0100, Benoît Sevens wrote: > > Hi Dan, > > On Mon, 2 Dec 2024 at 13:57, Dan Carpenter <dan.carpenter@linaro.org> wrote: > > > > The usb_get_descriptor() function does DMA so we're not allowed > > to use a stack buffer for that. Doing DMA to the stack is not portable > > all architectures. Move the "new_device_descriptor" from being stored > > on the stack and allocate it with kmalloc() instead. > > > > Thanks for fixing this. It looks good to me. > > Note that the commit that is being fixed is already queued for > backporting, so I don't know how this usually goes then. We just follow the same pattern as the Linus upstream, applying the fix on top of the previous fix. In anyway, now I merged it. Will be included in the next PR for rc2. Thanks! Takashi
On Mon, Dec 02, 2024 at 04:05:01PM +0100, Benoît Sevens wrote: > Hi Dan, > > On Mon, 2 Dec 2024 at 13:57, Dan Carpenter <dan.carpenter@linaro.org> wrote: > > > > The usb_get_descriptor() function does DMA so we're not allowed > > to use a stack buffer for that. Doing DMA to the stack is not portable > > all architectures. Move the "new_device_descriptor" from being stored > > on the stack and allocate it with kmalloc() instead. > > > > Thanks for fixing this. It looks good to me. > > Note that the commit that is being fixed is already queued for > backporting, so I don't know how this usually goes then. > It's fine. The stable scripts look for fixes to stable patches. But I also CC'd stable because your commit is CC'd for stable. Even CC'ing stable shouldn't be necessary here, maybe there is a rebase or something so the Fixes tag gets broken or maybe something else goes wrong. CC'ing stable is just an extra way to be careful. regards, dan carpenter
diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c index 8bc959b60be3..7c9d352864da 100644 --- a/sound/usb/quirks.c +++ b/sound/usb/quirks.c @@ -555,7 +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; - struct usb_device_descriptor new_device_descriptor; + struct usb_device_descriptor *new_device_descriptor __free(kfree) = NULL; int err; if (le16_to_cpu(get_cfg_desc(config)->wTotalLength) == EXTIGY_FIRMWARE_SIZE_OLD || @@ -566,15 +566,19 @@ 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); + + new_device_descriptor = kmalloc(sizeof(*new_device_descriptor), GFP_KERNEL); + if (!new_device_descriptor) + return -ENOMEM; err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, - &new_device_descriptor, sizeof(new_device_descriptor)); + new_device_descriptor, sizeof(*new_device_descriptor)); if (err < 0) dev_dbg(&dev->dev, "error usb_get_descriptor: %d\n", err); - if (new_device_descriptor.bNumConfigurations > dev->descriptor.bNumConfigurations) + if (new_device_descriptor->bNumConfigurations > dev->descriptor.bNumConfigurations) dev_dbg(&dev->dev, "error too large bNumConfigurations: %d\n", - new_device_descriptor.bNumConfigurations); + new_device_descriptor->bNumConfigurations); else - memcpy(&dev->descriptor, &new_device_descriptor, sizeof(dev->descriptor)); + memcpy(&dev->descriptor, new_device_descriptor, sizeof(dev->descriptor)); err = usb_reset_configuration(dev); if (err < 0) dev_dbg(&dev->dev, "error usb_reset_configuration: %d\n", err); @@ -906,7 +910,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; - struct usb_device_descriptor new_device_descriptor; + struct usb_device_descriptor *new_device_descriptor __free(kfree) = NULL; int err; u8 bootresponse[0x12]; int fwsize; @@ -941,15 +945,19 @@ static int snd_usb_mbox2_boot_quirk(struct usb_device *dev) dev_dbg(&dev->dev, "device initialised!\n"); + new_device_descriptor = kmalloc(sizeof(*new_device_descriptor), GFP_KERNEL); + if (!new_device_descriptor) + return -ENOMEM; + err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, - &new_device_descriptor, sizeof(new_device_descriptor)); + new_device_descriptor, sizeof(*new_device_descriptor)); if (err < 0) dev_dbg(&dev->dev, "error usb_get_descriptor: %d\n", err); - if (new_device_descriptor.bNumConfigurations > dev->descriptor.bNumConfigurations) + if (new_device_descriptor->bNumConfigurations > dev->descriptor.bNumConfigurations) dev_dbg(&dev->dev, "error too large bNumConfigurations: %d\n", - new_device_descriptor.bNumConfigurations); + new_device_descriptor->bNumConfigurations); else - memcpy(&dev->descriptor, &new_device_descriptor, sizeof(dev->descriptor)); + memcpy(&dev->descriptor, new_device_descriptor, sizeof(dev->descriptor)); err = usb_reset_configuration(dev); if (err < 0) @@ -1259,7 +1267,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; - struct usb_device_descriptor new_device_descriptor; + struct usb_device_descriptor *new_device_descriptor __free(kfree) = NULL; int err; int descriptor_size; @@ -1272,15 +1280,19 @@ static int snd_usb_mbox3_boot_quirk(struct usb_device *dev) dev_dbg(&dev->dev, "MBOX3: device initialised!\n"); + new_device_descriptor = kmalloc(sizeof(*new_device_descriptor), GFP_KERNEL); + if (!new_device_descriptor) + return -ENOMEM; + err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, - &new_device_descriptor, sizeof(new_device_descriptor)); + new_device_descriptor, sizeof(*new_device_descriptor)); if (err < 0) dev_dbg(&dev->dev, "MBOX3: error usb_get_descriptor: %d\n", err); - if (new_device_descriptor.bNumConfigurations > dev->descriptor.bNumConfigurations) + if (new_device_descriptor->bNumConfigurations > dev->descriptor.bNumConfigurations) dev_dbg(&dev->dev, "MBOX3: error too large bNumConfigurations: %d\n", - new_device_descriptor.bNumConfigurations); + new_device_descriptor->bNumConfigurations); else - memcpy(&dev->descriptor, &new_device_descriptor, sizeof(dev->descriptor)); + memcpy(&dev->descriptor, new_device_descriptor, sizeof(dev->descriptor)); err = usb_reset_configuration(dev); if (err < 0)
The usb_get_descriptor() function does DMA so we're not allowed to use a stack buffer for that. Doing DMA to the stack is not portable all architectures. Move the "new_device_descriptor" from being stored on the stack and allocate it with kmalloc() instead. Fixes: b909df18ce2a ("ALSA: usb-audio: Fix potential out-of-bound accesses for Extigy and Mbox devices") Cc: stable@kernel.org Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- sound/usb/quirks.c | 42 +++++++++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 15 deletions(-)