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

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

[Takashi Iwai]

The boot quirk for Extigy tries to continue even if the extraction of
a new device descriptor fails, and it means an uninitialized data is
copied onto the device.

Abort properly upon errors for safety, instead.
Also, make the error messages more visible, as they are now supposed to
be real errors.

Fixes: b909df18ce2a ("ALSA: usb-audio: Fix potential out-of-bound accesses for Extigy and Mbox devices")
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/usb/quirks.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
index 05a9f6727a3a..220ac9b10238 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));
 		err = usb_reset_configuration(dev);
 		if (err < 0)
 			dev_dbg(&dev->dev, "error usb_reset_configuration: %d\n", err);
-- 
2.55.0

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

[sashiko-bot]

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