Re: [PATCH v3 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 1 potential issue(s) to consider:
- [High] Concurrent ALSA control accesses during USB disconnect will use a freed `tm` pointer.
--

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

ALSA: usb-audio: expose the Topping M62's analogue gains as mixer controls

This commit adds a mixer driver for the Topping M62 to expose its analog
input gains and output volumes. The device uses a vendor protocol on a
HID-class interface, requiring the driver to claim the HID interface and
use interrupt endpoints for communication.

> diff --git a/sound/usb/mixer_topping.c b/sound/usb/mixer_topping.c
> new file mode 100644
> index 0000000000000..2f2365b9b4762
> --- /dev/null
> +++ b/sound/usb/mixer_topping.c
[ ... ]
> +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]
Can this dereference a freed tm pointer if called concurrently with
a USB disconnect?

When the USB device is unplugged, topping_private_free() frees tm
and sets mixer->private_data to NULL. However, ALSA controls are not
removed until snd_card_free(). If a userspace application accesses
the control via ioctl (e.g., SNDRV_CTL_IOCTL_ELEM_READ) during
disconnect, tm might already be freed.

> +	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;
> +	const struct topping_ctl_desc *d = &tm->ctls[elem->control];

[Severity: High]
Similarly, can this race with topping_private_free() and use a freed tm?

Although topping_send() later takes the snd_usb_lock_shutdown lock to
protect against disconnects, that happens after tm has already been
dereferenced here.

> +	int value, err;
[ ... ]
> +static void topping_private_free(struct usb_mixer_interface *mixer)
> +{
> +	struct topping_mixer *tm = mixer->private_data;
[ ... ]
> +	if (tm->claimed)
> +		snd_usb_release_iface(tm->iface);
> +	kfree(tm->val);
> +	kfree(tm->kctl);
> +	kfree(tm);
> +	mixer->private_data = NULL;
> +}

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