Re: [PATCH v4 2/2] ALSA: usb-audio: let the M62's outputs say what they listen to

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Concurrent ALSA control access during USB disconnect causes a Use-After-Free or NULL pointer dereference in the get/put handlers.
--

--- Patch [2]: [PATCH v4 2/2] ALSA: usb-audio: let the M62's outputs say what they listen to ---
commit 4881f3c183895a107d4e643799ad16e76377b10a
Author: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>

ALSA: usb-audio: let the M62's outputs say what they listen to

This commit adds enumerated controls for the Topping M62 mixer outputs to
select their playback source. It handles the specific hardware behaviour
where the current selection cannot be queried from the device.

> diff --git a/sound/usb/mixer_topping.c b/sound/usb/mixer_topping.c
> index 2f2365b9b4762..e71ea70ec4b00 100644
> --- a/sound/usb/mixer_topping.c
> +++ b/sound/usb/mixer_topping.c
[ ... ]
> @@ -332,6 +372,66 @@ static int topping_ctl_put(struct snd_kcontrol *kctl,
>  	return 1;
>  }
>  
> +static int topping_sel_info(struct snd_kcontrol *kctl,
> +			    struct snd_ctl_elem_info *uinfo)
> +{
> +	return snd_ctl_enum_info(uinfo, 1, ARRAY_SIZE(topping_sources),
> +				 topping_sources);
> +}
> +
> +static int topping_sel_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(mutex)(&tm->write_lock);

[Severity: High]
This is a pre-existing issue, but does this dereference of tm risk a
use-after-free or NULL pointer dereference if the USB device is disconnected
concurrently?

If an ALSA control ioctl is executing while the USB device is disconnected,
the disconnect sequence calls usb_audio_disconnect_components() which calls
snd_usb_mixer_disconnect(). This triggers topping_private_free() which
frees the tm struct and sets mixer->private_data to NULL.

If the control callback executes without first calling
snd_usb_lock_shutdown() to check for device disconnection, it can extract
and dereference tm (e.g., locking tm->write_lock here) either after it has
been freed or while it is NULL.

> +	ucontrol->value.enumerated.item[0] = tm->sel[elem->control];
> +	return 0;
> +}
> +
> +static int topping_sel_put(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;
> +	const struct topping_enum_desc *d;
> +	unsigned int item;
> +	int err;
> +
> +	item = ucontrol->value.enumerated.item[0];
> +	if (item >= ARRAY_SIZE(topping_sources))
> +		return -EINVAL;
> +
> +	guard(mutex)(&tm->write_lock);

[Severity: High]
This is a pre-existing issue, but could this also trigger a use-after-free
or NULL pointer dereference for the same reasons as in topping_sel_get()?

As this issue is also present in the pre-existing topping_ctl_put() and
topping_ctl_get(), perhaps snd_usb_lock_shutdown() needs to be used to
safeguard these control callbacks against concurrent USB teardown.

> +
> +	/*
> +	 * "Unknown" is what this control reports until a hand has chosen,
> +	 * and alsactl stores and restores it like any other value.  It is
> +	 * not a choice, so writing it changes nothing -- quietly, rather
> +	 * than failing a restore of the driver's own report.
> +	 */
> +	if (!item || tm->sel[elem->control] == item)
> +		return 0;

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