Re: [PATCH] ALSA: usb-audio: Reset Blue Yeti firmware 0.20 after resume

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] ALSA: usb-audio: module text UAF via module_put() in snd_usb_yeti_reset_work()
- [Medium] ALSA: usb-audio: Blue Yeti resume quirk skipped or fails under runtime PM
- [Medium] ALSA: usb-audio: untracked work in snd_usb_yeti_resume_quirk() causes duplicate resets and post-disconnect I/O
--

commit 592f2406e0255360e88f55c391bf3d248002c71a
Author: Jasmeet (Jazz) Bhatia <jasmeet.bhatia.us@gmail.com>

ALSA: usb-audio: Reset Blue Yeti firmware 0.20 after resume

This commit adds a resume quirk for Blue Yeti Classic microphones with
firmware 0.20 (USB ID 046d:0ab7, bcdDevice 0x0020) that schedules a deferred
volatile ROM reset command on system_freezable_wq after system resume so the
device re-enumerates and restores audio capture.

> diff --git a/sound/usb/card.c b/sound/usb/card.c
> index 9307da95efbef..ebacb98fc34e4 100644
> --- a/sound/usb/card.c
> +++ b/sound/usb/card.c
> @@ -1280,6 +1280,9 @@ static int usb_audio_resume(struct usb_interface *intf)
>  	if (chip->num_suspended_intf > 1)
>  		goto out;
>  
> +	if (chip->system_suspend)
> +		snd_usb_apply_resume_quirk(chip);

[Severity: Medium]
Can gating snd_usb_apply_resume_quirk() on chip->system_suspend in
sound/usb/card.c:usb_audio_resume() cause the reset quirk to be skipped when
the device is runtime-suspended prior to system suspend?

Because usb_audio_driver enables .supports_autosuspend = 1 and 046d:0ab7 does
not set QUIRK_FLAG_DISABLE_AUTOSUSPEND, a runtime suspend before system sleep
calls usb_audio_suspend(intf, PMSG_AUTO_SUSPEND), which leaves
chip->system_suspend as 0:

sound/usb/card.c:usb_audio_suspend() {
    ...
	if (!PMSG_IS_AUTO(message) && !chip->system_suspend) {
		snd_power_change_state(chip->card, SNDRV_CTL_POWER_D3hot);
		chip->system_suspend = chip->num_suspended_intf;
	}
    ...
}

When the system subsequently enters system suspend (PMSG_SUSPEND),
drivers/usb/core/driver.c:usb_suspend_both() sees that udev->state is already
USB_STATE_SUSPENDED and skips calling usb_suspend_interface() ->
usb_audio_suspend():

drivers/usb/core/driver.c:usb_suspend_both() {
	if (udev->state == USB_STATE_NOTATTACHED ||
			udev->state == USB_STATE_SUSPENDED)
		goto done;
    ...
}

On system resume, usb_resume_both(udev, PMSG_RESUME) calls usb_audio_resume()
with chip->system_suspend still equal to 0, so snd_usb_apply_resume_quirk()
will not run.

Also, when snd_usb_yeti_resume_quirk() queues yeti->work onto
system_freezable_wq, it only takes a kobject reference via usb_get_dev(dev)
without holding a runtime PM reference (such as usb_autopm_get_interface() or
usb_autoresume_device()). Since system_freezable_wq remains frozen until
thaw_workqueues() runs after dpm_resume_end(), could the device autosuspend
before snd_usb_yeti_reset_work() executes, causing usb_control_msg_send() to
fail with -EHOSTUNREACH?

> +
>  	list_for_each_entry(as, &chip->pcm_list, list) {
>  		err = snd_usb_pcm_resume(as);
>  		if (err < 0) {
> diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
> index a54fabce7920d..42d955c7b7a4d 100644
> --- a/sound/usb/quirks.c
> +++ b/sound/usb/quirks.c

[ ... ]

> @@ -1626,6 +1628,81 @@ int snd_usb_apply_interface_quirk(struct snd_usb_audio *chip,
>  	return 0;
>  }
>  
> +/*
> + * Blue Yeti Classic firmware 0.20 may stop delivering capture samples
> + * after a system suspend/resume while otherwise remaining enumerated.
> + *
> + * Sending the device's volatile ROM reset command restarts its MCU and
> + * USB logic, causing it to re-enumerate and restoring capture.
> + *
> + * Do not send the command directly from the USB resume callback because
> + * it deliberately disconnects the device.  Queue it on the freezable
> + * workqueue so it runs after device resume callbacks have completed.
> + */
> +struct snd_usb_yeti_reset {
> +	struct work_struct work;
> +	struct usb_device *dev;
> +};
> +
> +static void snd_usb_yeti_reset_work(struct work_struct *work)
> +{
> +	struct snd_usb_yeti_reset *yeti =
> +		container_of(work, struct snd_usb_yeti_reset, work);
> +	struct usb_device *dev = yeti->dev;
> +	static const u8 reset_cmd[8] = {
> +		0x00, 0x09, 0x00, 0x00,
> +		0x00, 0x00, 0x00, 0x00
> +	};
> +	int err;
> +
> +	err = usb_control_msg_send(dev, 0, UAC_SET_CUR,
> +				   USB_DIR_OUT | USB_TYPE_CLASS |
> +				   USB_RECIP_INTERFACE,
> +				   0x0a00, 0x1900,
> +				   reset_cmd, sizeof(reset_cmd),
> +				   USB_CTRL_SET_TIMEOUT, GFP_KERNEL);
> +
> +	if (err && err != -ENODEV)
> +		dev_warn(&dev->dev,
> +			 "Blue Yeti resume reset failed: %d\n", err);
> +
> +	usb_put_dev(dev);
> +	kfree(yeti);
> +	module_put(THIS_MODULE);
> +}

[Severity: High]
Does calling module_put(THIS_MODULE) at the end of
sound/usb/quirks.c:snd_usb_yeti_reset_work() leave a race window where the
module text can be unmapped before the worker thread returns from the
function?

Because yeti->work is allocated as an untracked object and is not flushed via
cancel_work_sync() in usb_audio_disconnect() or on module exit,
snd_usb_yeti_reset_work() relies on __module_get(THIS_MODULE) to keep
snd-usb-audio loaded while the work runs.

When the reset command disconnects the USB device (or if the device was
already unbound), module_put(THIS_MODULE) can drop the last reference to the
module while execution is still inside snd_usb_yeti_reset_work(). Once the
refcount reaches zero, a concurrent delete_module() in
kernel/module/main.c:delete_module() can proceed:

kernel/module/main.c:delete_module() {
	ret = try_stop_module(mod, flags, &forced);
	if (ret != 0)
		goto out;

	mutex_unlock(&module_mutex);
	/* Final destruction now no one is using it. */
	if (mod->exit != NULL)
		mod->exit();
    ...
	free_module(mod);
}

This can unmap the module's executable text before the worker thread finishes
returning from module_put() and executing the epilogue of
snd_usb_yeti_reset_work().

> +
> +static void snd_usb_yeti_resume_quirk(struct snd_usb_audio *chip)
> +{
> +	struct usb_device *dev = chip->dev;
> +	struct snd_usb_yeti_reset *yeti;
> +
> +	if (chip->usb_id != USB_ID(0x046d, 0x0ab7) ||
> +	    le16_to_cpu(dev->descriptor.bcdDevice) != 0x0020)
> +		return;
> +
> +	yeti = kmalloc_obj(*yeti);
> +	if (!yeti) {
> +		dev_warn(&dev->dev,
> +			 "Blue Yeti resume reset allocation failed\n");
> +		return;
> +	}
> +
> +	INIT_WORK(&yeti->work, snd_usb_yeti_reset_work);
> +	yeti->dev = usb_get_dev(dev);
> +	__module_get(THIS_MODULE);
> +
> +	if (!queue_work(system_freezable_wq, &yeti->work)) {
> +		module_put(THIS_MODULE);
> +		usb_put_dev(yeti->dev);
> +		kfree(yeti);
> +	}
> +}

[Severity: Medium]
What happens in sound/usb/quirks.c:snd_usb_yeti_resume_quirk() across
hibernation freeze/thaw transitions or if the driver unbinds before
system_freezable_wq runs?

Allocating a new struct snd_usb_yeti_reset and calling INIT_WORK() on every
invocation means WORK_STRUCT_PENDING_BIT is always clear when queue_work() is
called, which makes the !queue_work() cleanup path unreachable and prevents
workqueue deduplication.

During hibernation in kernel/power/hibernate.c:hibernation_snapshot(),
freeze_kernel_threads() freezes system_freezable_wq before
dpm_suspend(PMSG_FREEZE) calls usb_audio_suspend(intf, PMSG_FREEZE). Because
!PMSG_IS_AUTO(PMSG_FREEZE) is true, chip->system_suspend is set even though
usb_generic_driver_suspend() does not suspend USB2 ports on PM_EVENT_FREEZE.
When dpm_resume(PMSG_THAW) then calls usb_audio_resume(), a reset work item
is queued onto the still-frozen system_freezable_wq. In hybrid sleep
(HIBERNATION_SUSPEND) or if hibernation_platform_enter() rolls back via
dpm_resume_end(PMSG_RESTORE), usb_audio_suspend() and usb_audio_resume() run
a second time before workqueues are thawed, queuing a second reset work item
for the same usb_device.

Furthermore, because yeti->work is not tracked in struct snd_usb_audio and
canceled in sound/usb/card.c:__usb_audio_disconnect():

sound/usb/card.c:__usb_audio_disconnect() {
    ...
	if (atomic_inc_return(&chip->shutdown) == 1) {
		/* wait until all pending tasks done;
		 * they are protected by snd_usb_lock_shutdown()
		 */
		snd_refcount_sync(&chip->usage_count);
		snd_card_disconnect(card);
		usb_audio_disconnect_components(chip);
	}
    ...
}

unbinding snd-usb-audio before the deferred work executes leaves
snd_usb_yeti_reset_work() active to send the interface-targeted UAC_SET_CUR
control message after usb_audio_disconnect() has returned, whereas
drivers/usb/core/message.c:usb_control_msg_send() requires:

 * If a thread in your driver uses this call,
 * make sure your disconnect() method can wait for it to complete. Since you
 * don't have a handle on the URB used, you can't cancel the request.

Would embedding the work_struct in struct snd_usb_audio and canceling it via
cancel_work_sync() on disconnect avoid both this issue and the module_put()
race above?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021231754.16928-1-jasmeet.bhatia.us@gmail.com?part=1