Re: [PATCH] ALSA: usb-audio: Abort properly at errors for Extidy boot quirk

[sashiko-bot@kernel.org]

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