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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The ALSA control callbacks lack synchronization with device unplug, leading to a Use-After-Free or NULL pointer dereference.
- [Medium] Restoring the default 'Unknown' state via alsactl will fail with -EINVAL, potentially causing the alsa-restore service to fail on boot.
--

--- Patch [2]: [PATCH v2 2/2] ALSA: usb-audio: let the M62's outputs say what they listen to ---
commit 897c9f5bff5f6d9c9756bc3709ad74756868ab0e
Author: Mikhail Gavrilov <[email protected]>

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

This patch adds an enumerated control to select the playback source for the
Topping M62's outputs. Because the device doesn't report its selector state
on probe, the initial state is reported as "Unknown".

> diff --git a/sound/usb/mixer_topping.c b/sound/usb/mixer_topping.c
> index 3405f28dec66c..029f54865e220 100644
> --- a/sound/usb/mixer_topping.c
> +++ b/sound/usb/mixer_topping.c

[ ... ]

> @@ -312,6 +352,58 @@ 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;
> +
> +	ucontrol->value.enumerated.item[0] = tm->sel[elem->control];

[Severity: High]
Does this code race with device unplug, leading to a UAF or NULL pointer
dereference?

If the USB device is disconnected, snd_usb_audio_disconnect() calls
snd_usb_mixer_disconnect(), which invokes topping_private_free().
topping_private_free() frees tm and sets mixer->private_data = NULL.

If a concurrently running ALSA control ioctl invokes topping_sel_get() or
topping_sel_put(), they do not call snd_usb_lock_shutdown() to
synchronize with the disconnect teardown, nor do they verify tm is not NULL.

Could this lead to a blind dereference of tm and cause a crash?

> +	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;
> +	if (!item)
> +		return -EINVAL;	/* "Unknown" is a report, not a choice */
> +	if (tm->sel[elem->control] == item)
> +		return 0;

[Severity: Medium]
Will restoring the default 'Unknown' state via alsactl fail with -EINVAL?

On probe, tm->sel defaults to 0 ('Unknown'). When alsactl store saves 0, and
alsactl restore subsequently calls this put callback with 0 on boot, the
check for !item returns -EINVAL before the code checks if the value is
unchanged (tm->sel[elem->control] == item).

Could this cause the alsa-restore systemd service to fail on boot and produce
spurious error logs, instead of just returning 0 for an unchanged value?

> +
> +	d = &topping_m62_enums[elem->control];
> +	err = topping_send(tm, d->target, d->prop,
> +			   topping_source_value[item]);
> +	if (err < 0)
> +		return err;
> +
> +	tm->sel[elem->control] = item;
> +	return 1;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2