[bug report] ALSA: usb-audio: Add quirk to enable Avid Mbox 3 support

[bug report] ALSA: usb-audio: Add quirk to enable Avid Mbox 3 support

[Dan Carpenter]

Hello Conner Knox,

Commit b01104fc62b6 ("ALSA: usb-audio: Add quirk to enable Avid Mbox
3 support") from Aug 18, 2022 (linux-next), leads to the following
Smatch static checker warning:

sound/usb/quirks.c:1275 snd_usb_mbox3_boot_quirk() error: doing dma on the stack (&new_device_descriptor)
sound/usb/quirks.c:569 snd_usb_extigy_boot_quirk() error: doing dma on the stack (&new_device_descriptor)
sound/usb/quirks.c:944 snd_usb_mbox2_boot_quirk() error: doing dma on the stack (&new_device_descriptor)

sound/usb/quirks.c
    1259 static int snd_usb_mbox3_boot_quirk(struct usb_device *dev)
    1260 {
    1261         struct usb_host_config *config = dev->actconfig;
    1262         struct usb_device_descriptor new_device_descriptor;
                                              ^^^^^^^^^^^^^^^^^^^^^^
    1263         int err;
    1264         int descriptor_size;
    1265 
    1266         descriptor_size = le16_to_cpu(get_cfg_desc(config)->wTotalLength);
    1267 
    1268         if (descriptor_size != MBOX3_DESCRIPTOR_SIZE) {
    1269                 dev_err(&dev->dev, "MBOX3: Invalid descriptor size=%d.\n", descriptor_size);
    1270                 return -ENODEV;
    1271         }
    1272 
    1273         dev_dbg(&dev->dev, "MBOX3: device initialised!\n");
    1274 
--> 1275         err = usb_get_descriptor(dev, USB_DT_DEVICE, 0,
    1276                 &new_device_descriptor, sizeof(new_device_descriptor));
                         ^^^^^^^^^^^^^^^^^^^^^^
The comments in usb_fill_control_urb() say this has to be "suitable for DMA"
but this is stack data.  It has to be allocated with kmalloc() to be suitable.

I don't know why I'm only seeing this warning now two years later...

    1277         if (err < 0)
    1278                 dev_dbg(&dev->dev, "MBOX3: error usb_get_descriptor: %d\n", err);
    1279         if (new_device_descriptor.bNumConfigurations > dev->descriptor.bNumConfigurations)
    1280                 dev_dbg(&dev->dev, "MBOX3: error too large bNumConfigurations: %d\n",
    1281                         new_device_descriptor.bNumConfigurations);
    1282         else
    1283                 memcpy(&dev->descriptor, &new_device_descriptor, sizeof(dev->descriptor));
    1284 
    1285         err = usb_reset_configuration(dev);
    1286         if (err < 0)
    1287                 dev_dbg(&dev->dev, "MBOX3: error usb_reset_configuration: %d\n", err);
    1288 
    1289         dev_dbg(&dev->dev, "MBOX3: new boot length = %d\n",
    1290                 le16_to_cpu(get_cfg_desc(config)->wTotalLength));
    1291 
    1292         mbox3_setup_defaults(dev);
    1293         dev_info(&dev->dev, "MBOX3: Initialized.");
    1294 
    1295         return 0; /* Successful boot */
    1296 }

regards,
dan carpenter

Re: [bug report] ALSA: usb-audio: Add quirk to enable Avid Mbox 3 support

[Dan Carpenter]

On Sat, Nov 30, 2024 at 01:01:49PM +0300, Dan Carpenter wrote:
> Hello Conner Knox,
> 
> Commit b01104fc62b6 ("ALSA: usb-audio: Add quirk to enable Avid Mbox
> 3 support") from Aug 18, 2022 (linux-next), leads to the following
> Smatch static checker warning:
> 
> sound/usb/quirks.c:1275 snd_usb_mbox3_boot_quirk() error: doing dma on the stack (&new_device_descriptor)
> sound/usb/quirks.c:569 snd_usb_extigy_boot_quirk() error: doing dma on the stack (&new_device_descriptor)
> sound/usb/quirks.c:944 snd_usb_mbox2_boot_quirk() error: doing dma on the stack (&new_device_descriptor)
> 
> sound/usb/quirks.c
>     1259 static int snd_usb_mbox3_boot_quirk(struct usb_device *dev)
>     1260 {
>     1261         struct usb_host_config *config = dev->actconfig;
>     1262         struct usb_device_descriptor new_device_descriptor;
>                                               ^^^^^^^^^^^^^^^^^^^^^^
>     1263         int err;
>     1264         int descriptor_size;
>     1265 
>     1266         descriptor_size = le16_to_cpu(get_cfg_desc(config)->wTotalLength);
>     1267 
>     1268         if (descriptor_size != MBOX3_DESCRIPTOR_SIZE) {
>     1269                 dev_err(&dev->dev, "MBOX3: Invalid descriptor size=%d.\n", descriptor_size);
>     1270                 return -ENODEV;
>     1271         }
>     1272 
>     1273         dev_dbg(&dev->dev, "MBOX3: device initialised!\n");
>     1274 
> --> 1275         err = usb_get_descriptor(dev, USB_DT_DEVICE, 0,
>     1276                 &new_device_descriptor, sizeof(new_device_descriptor));
>                          ^^^^^^^^^^^^^^^^^^^^^^
> The comments in usb_fill_control_urb() say this has to be "suitable for DMA"
> but this is stack data.  It has to be allocated with kmalloc() to be suitable.
> 
> I don't know why I'm only seeing this warning now two years later...
> 

My script looks at the line number for the function and not for the buffer so it
blamed the wrong patch.

Don't stress about this, I'll send a fix in a few hours.

regards,
dan carpenter

Re: [bug report] ALSA: usb-audio: Add quirk to enable Avid Mbox 3 support

[Takashi Iwai]

On Mon, 02 Dec 2024 10:04:41 +0100,
Dan Carpenter wrote:
> 
> On Sat, Nov 30, 2024 at 01:01:49PM +0300, Dan Carpenter wrote:
> > Hello Conner Knox,
> > 
> > Commit b01104fc62b6 ("ALSA: usb-audio: Add quirk to enable Avid Mbox
> > 3 support") from Aug 18, 2022 (linux-next), leads to the following
> > Smatch static checker warning:
> > 
> > sound/usb/quirks.c:1275 snd_usb_mbox3_boot_quirk() error: doing dma on the stack (&new_device_descriptor)
> > sound/usb/quirks.c:569 snd_usb_extigy_boot_quirk() error: doing dma on the stack (&new_device_descriptor)
> > sound/usb/quirks.c:944 snd_usb_mbox2_boot_quirk() error: doing dma on the stack (&new_device_descriptor)
> > 
> > sound/usb/quirks.c
> >     1259 static int snd_usb_mbox3_boot_quirk(struct usb_device *dev)
> >     1260 {
> >     1261         struct usb_host_config *config = dev->actconfig;
> >     1262         struct usb_device_descriptor new_device_descriptor;
> >                                               ^^^^^^^^^^^^^^^^^^^^^^
> >     1263         int err;
> >     1264         int descriptor_size;
> >     1265 
> >     1266         descriptor_size = le16_to_cpu(get_cfg_desc(config)->wTotalLength);
> >     1267 
> >     1268         if (descriptor_size != MBOX3_DESCRIPTOR_SIZE) {
> >     1269                 dev_err(&dev->dev, "MBOX3: Invalid descriptor size=%d.\n", descriptor_size);
> >     1270                 return -ENODEV;
> >     1271         }
> >     1272 
> >     1273         dev_dbg(&dev->dev, "MBOX3: device initialised!\n");
> >     1274 
> > --> 1275         err = usb_get_descriptor(dev, USB_DT_DEVICE, 0,
> >     1276                 &new_device_descriptor, sizeof(new_device_descriptor));
> >                          ^^^^^^^^^^^^^^^^^^^^^^
> > The comments in usb_fill_control_urb() say this has to be "suitable for DMA"
> > but this is stack data.  It has to be allocated with kmalloc() to be suitable.
> > 
> > I don't know why I'm only seeing this warning now two years later...
> > 
> 
> My script looks at the line number for the function and not for the buffer so it
> blamed the wrong patch.
> 
> Don't stress about this, I'll send a fix in a few hours.

Yep, the culprit is rather the recent fix commit b909df18ce2a:
  ALSA: usb-audio: Fix potential out-of-bound accesses for Extigy and
  Mbox devices


thanks,

Takashi