* Re: [PATCH] ALSA: usb-audio: Abort properly at errors for Extidy boot quirk
2026-11-03 15:31 [PATCH] ALSA: usb-audio: Abort properly at errors for Extidy boot quirk Takashi Iwai
@ 2026-11-03 15:41 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-11-03 15:41 UTC (permalink / raw)
To: Takashi Iwai, linux-sound
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ALSA: usb-audio: short read and config leak in snd_usb_extigy_boot_quirk()
--
commit a1ee9b55505d63c42061631f3b97d61b782917c1
Author: Takashi Iwai <tiwai@suse.de>
ALSA: usb-audio: Abort properly at errors for Extidy boot quirk
This commit updates snd_usb_extigy_boot_quirk() to abort with -ENODEV and log
with dev_err() when usb_get_descriptor() fails or returns a bNumConfigurations
value exceeding the original descriptor's count.
> diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
> index 05a9f6727a3a5..220ac9b10238c 100644
> --- a/sound/usb/quirks.c
> +++ b/sound/usb/quirks.c
> @@ -575,13 +575,18 @@ static int snd_usb_extigy_boot_quirk(struct usb_device *dev, struct usb_interfac
> return -ENOMEM;
> err = usb_get_descriptor(dev, USB_DT_DEVICE, 0,
> 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)
> - dev_dbg(&dev->dev, "error too large bNumConfigurations: %d\n",
> + if (err < 0) {
> + dev_err(&dev->dev, "error usb_get_descriptor: %d\n", err);
> + return -ENODEV;
> + }
> +
> + if (new_device_descriptor->bNumConfigurations > dev->descriptor.bNumConfigurations) {
> + dev_err(&dev->dev, "error too large bNumConfigurations: %d\n",
> new_device_descriptor->bNumConfigurations);
> - else
> - memcpy(&dev->descriptor, new_device_descriptor, sizeof(dev->descriptor));
> + return -ENODEV;
> + }
> +
> + memcpy(&dev->descriptor, new_device_descriptor, sizeof(dev->descriptor));
[Severity: Medium]
Can a short read from usb_get_descriptor() or a smaller bNumConfigurations
value cause dev->descriptor to be overwritten with a zero-padded descriptor
and leak per-configuration resources in usb_destroy_configuration()?
In drivers/usb/core/message.c:usb_get_descriptor(), the buffer is first zeroed
with memset() and the function returns the number of bytes transferred:
drivers/usb/core/message.c:usb_get_descriptor() {
memset(buf, 0, size); /* Make sure we parse really received data */
for (i = 0; i < 3; ++i) {
/* retry on length 0 or error; some devices are flakey */
result = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
USB_REQ_GET_DESCRIPTOR, USB_DIR_IN,
(type << 8) + index, 0, buf, size,
USB_CTRL_GET_TIMEOUT);
if (result <= 0 && result != -ETIMEDOUT)
continue;
if (result > 1 && ((u8 *)buf)[1] != type) {
result = -ENODATA;
continue;
}
break;
}
return result;
}
If the transfer returns 0 bytes on all retries, or returns a partial descriptor
shorter than sizeof(*new_device_descriptor), usb_get_descriptor() returns a
non-negative byte count smaller than sizeof(*new_device_descriptor). Because
bNumConfigurations is the final byte of struct usb_device_descriptor, it
remains 0 on any short read, passing both the err < 0 check and the
new_device_descriptor->bNumConfigurations > dev->descriptor.bNumConfigurations
check before memcpy() overwrites dev->descriptor.
Similarly, if a full read returns a smaller bNumConfigurations than the
original dev->descriptor.bNumConfigurations used when usb_get_configuration()
allocated dev->config and dev->rawdescriptors, overwriting dev->descriptor
reduces dev->descriptor.bNumConfigurations. When the device is later cleaned
up in drivers/usb/core/config.c:usb_destroy_configuration():
drivers/usb/core/config.c:usb_destroy_configuration() {
if (dev->rawdescriptors) {
for (i = 0; i < dev->descriptor.bNumConfigurations; i++)
kfree(dev->rawdescriptors[i]);
kfree(dev->rawdescriptors);
dev->rawdescriptors = NULL;
}
for (c = 0; c < dev->descriptor.bNumConfigurations; c++) {
struct usb_host_config *cf = &dev->config[c];
kfree(cf->string);
for (i = 0; i < cf->desc.bNumInterfaces; i++) {
if (cf->intf_cache[i])
kref_put(&cf->intf_cache[i]->ref,
usb_release_interface_cache);
}
}
}
wouldn't this leak dev->rawdescriptors[i], cf->string, and cf->intf_cache[i]
for the remaining configurations? Should snd_usb_extigy_boot_quirk() check
err < sizeof(*new_device_descriptor) (or err != sizeof(*new_device_descriptor))
andnew_device_descriptor->bNumConfigurations != dev->descriptor.bNumConfigurations
instead?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261103153123.112097-1-tiwai@suse.de?part=1
^ permalink raw reply [flat|nested] 2+ messages in thread