Re: [PATCH v5 1/2] ALSA: usb-audio: expose the Topping M62's analogue gains as mixer controls

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] ALSA mixer callbacks access `private_data` without holding `snd_usb_lock_shutdown()`, leading to a Use-After-Free on device disconnect.
- [High] Memory Allocation Deadlock during System Resume due to `GFP_KERNEL` allocation inside `usb_interrupt_msg()`.
- [High] Use-After-Free of `tm->iface` in `topping_private_free()` because no reference count was taken on the USB interface.
- [Medium] The URB completion handler permanently drops the interrupt polling loop on transient USB errors.
--

--- Patch [1]: [PATCH v5 1/2] ALSA: usb-audio: expose the Topping M62's analogue gains as mixer controls ---
commit da50971ed7815f82050b21f7d59c4472171b1ad1
Author: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>
Subject: ALSA: usb-audio: expose the Topping M62's analogue gains as mixer controls

This commit introduces a mixer quirk for the Topping M62 to expose its
hardware analogue gains and output volumes. The device hides these behind
a custom vendor protocol on a HID-class interface. The patch claims the
interface and uses an interrupt URB and scheduled keepalive messages to
sync the state with ALSA controls.

> diff --git a/sound/usb/mixer_topping.c b/sound/usb/mixer_topping.c
> new file mode 100644
> index 0000000000000..2f42b26336266
> --- /dev/null
> +++ b/sound/usb/mixer_topping.c
> @@ -0,0 +1,587 @@

[ ... ]

> +static int topping_send(struct topping_mixer *tm, u8 target, u8 prop,
> +			s32 value)
> +{
> +	/*
> +	 * NOIO rather than KERNEL: this is called from the resume path
> +	 * too, where reclaim can wait on a block device that has not
> +	 * woken yet.  The frame is fifteen bytes; nothing is lost by
> +	 * asking for it without I/O.
> +	 */
> +	u8 *buf __free(kfree) = kzalloc(TOPPING_EP_BUF, GFP_NOIO);
> +	int err, actual;
> +
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	/*
> +	 * The shutdown lock is what makes a write safe against disconnect:
> +	 * the teardown waits for everyone holding it before the card goes
> +	 * away, so the device and this mixer are alive for as long as it
> +	 * is held.
> +	 */
> +	CLASS(snd_usb_lock, pm)(tm->mixer->chip);
> +	if (pm.err < 0)
> +		return -EIO;
> +	topping_build(buf, target, prop, value);
> +	err = usb_interrupt_msg(tm->mixer->chip->dev, tm->pipe_out,
> +				buf, TOPPING_FRAME_LEN, &actual, 1000);

[Severity: High]
Could the use of usb_interrupt_msg() here cause a memory allocation deadlock
during system resume? 

While the buffer is allocated with GFP_NOIO, usb_interrupt_msg() calls
usb_bulk_msg() which unconditionally allocates a new URB using GFP_KERNEL.
Since topping_send() is called from topping_resume(), this GFP_KERNEL
allocation might sleep on block I/O while the device is suspended, leading
to a deadlock.

> +	if (err < 0)
> +		usb_audio_err(tm->mixer->chip,
> +			      "Topping: write %02x/%02x failed: %d\n",
> +			      target, prop, err);
> +	return err;
> +}

[ ... ]

> +static void topping_urb_complete(struct urb *urb)
> +{
> +	struct topping_mixer *tm = urb->context;
> +	const u8 *f = urb->transfer_buffer;
> +	int idx, value, err;
> +	bool changed;
> +
> +	if (urb->status)
> +		return;		/* resubmitted below only when running */

[Severity: Medium]
Will this early return permanently drop the interrupt polling loop on
transient USB errors? 

If urb->status returns a non-terminal error such as -EPROTO or -EILSEQ due
to bus noise, the handler exits without resubmitting the URB. This would
stop the hardware update stream permanently until the device is replugged.

> +	if (urb->actual_length < TOPPING_FRAME_LEN)
> +		goto resubmit;

[ ... ]

> +static int topping_ctl_get(struct snd_kcontrol *kctl,
> +			   struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct usb_mixer_elem_info *elem = kctl->private_data;
> +	struct topping_mixer *tm = elem->head.mixer->private_data;
> +
> +	guard(spinlock_irqsave)(&tm->lock);

[Severity: High]
Does this code need to take the ALSA USB shutdown lock before dereferencing
the private data? 

If a userspace process continuously accesses this mixer control while the
USB device is physically disconnected, topping_private_free() could free
the tm structure during or just before the execution of this callback.
Without CLASS(snd_usb_lock, pm)(chip), this might lead to a use-after-free.

> +	ucontrol->value.integer.value[0] = tm->val[elem->control];
> +	return 0;
> +}
> +
> +static int topping_ctl_put(struct snd_kcontrol *kctl,
> +			   struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct usb_mixer_elem_info *elem = kctl->private_data;
> +	struct usb_mixer_interface *mixer = elem->head.mixer;
> +	struct topping_mixer *tm = mixer->private_data;

[Severity: High]
Is it possible for the mixer->private_data pointer to be freed concurrently
here?

Similar to topping_ctl_get(), if the device is disconnected, this callback
might execute after topping_private_free() has already freed the tm
structure. Taking the ALSA USB shutdown lock before dereferencing the data
would prevent this use-after-free.

> +	const struct topping_ctl_desc *d = &tm->ctls[elem->control];
> +	int value, err;

[ ... ]

> +static void topping_private_free(struct usb_mixer_interface *mixer)
> +{
> +	struct topping_mixer *tm = mixer->private_data;
> +
> +	if (!tm)
> +		return;
> +	cancel_delayed_work_sync(&tm->keepalive);
> +	if (tm->urb) {
> +		usb_kill_urb(tm->urb);
> +		usb_free_coherent(mixer->chip->dev, TOPPING_EP_BUF,
> +				  tm->inbuf, tm->inbuf_dma);
> +		usb_free_urb(tm->urb);
> +	}
> +	if (tm->claimed)
> +		snd_usb_release_iface(tm->iface);

[Severity: High]
Could tm->iface have already been freed by the USB core here?

Since the interface was claimed without incrementing its reference count
via usb_get_intf(), the USB core might unbind and free this secondary HID
interface upon device unplug while the ALSA sound card is still held open by
userspace. Later, when the card is closed, this function will pass the freed
pointer to snd_usb_release_iface(), causing a use-after-free.

> +	kfree(tm->val);
> +	kfree(tm->kctl);
> +	kfree(tm);
> +	mixer->private_data = NULL;
> +}

[ ... ]

> +int snd_topping_init(struct usb_mixer_interface *mixer)
> +{
> +	struct snd_usb_audio *chip = mixer->chip;
> +	struct usb_interface *iface;
> +	struct topping_mixer *tm;
> +	int ep_in = 0, ep_out = 0, interval = 5;
> +	int i, err;
> +
> +	iface = topping_find_iface(chip, &ep_in, &ep_out, &interval);
> +	if (!iface) {
> +		usb_audio_err(chip, "Topping: no vendor HID interface\n");
> +		return 0;	/* not fatal: the card still plays */
> +	}
> +	if (usb_interface_claimed(iface)) {
> +		usb_audio_err(chip,
> +			      "Topping: the HID interface is already claimed\n");
> +		return 0;
> +	}
> +
> +	tm = kzalloc_obj(*tm);
> +	if (!tm)
> +		return -ENOMEM;
> +
> +	tm->mixer = mixer;
> +	tm->iface = iface;

[Severity: High]
Should we be taking a reference to the interface here using usb_get_intf()?

Caching the pointer without an additional reference can leave the pointer
dangling if the USB core unbinds the interface on physical disconnect before
topping_private_free() is invoked.

> +	tm->ctls = topping_m62_ctls;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824201331.304705-1-mikhail.v.gavrilov@gmail.com?part=1