Re: [PATCH v7 2/2] ALSA: usb-audio: bind the Topping M62's vendor controls

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] sound/usb: Use-After-Free in topping_private_free() via TOCTOU race
--

commit a868f2097e7f0e27683675ee55b1cecdef99afcf
Author: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>
Subject: ALSA: usb-audio: bind the Topping M62's vendor controls

This patch introduces a component master for the Topping M62's vendor
controls, binding them to the hid driver that speaks the vendor protocol.
It utilizes the component framework and devres to manage the lifetime
of the controls when either the audio or hid interfaces bind and unbind.

> diff --git a/sound/usb/mixer_topping.c b/sound/usb/mixer_topping.c
> new file mode 100644
> index 0000000000000..5557bc41f05f0
> --- /dev/null
> +++ b/sound/usb/mixer_topping.c
[ ... ]
> +static void topping_master_teardown(void *data)
> +{
> +	struct topping_master *tm = data;
> +
> +	component_master_del(tm->dev, &topping_master_ops);
> +
> +	if (tm->mixer) {
> +		tm->mixer->private_data = NULL;
> +		tm->mixer->private_free = NULL;
> +	}
> +}

[Severity: Critical]
Can this clearing of mixer->private_data happen concurrently with
snd_usb_mixer_disconnect() without synchronization?

If a secondary interface fails to probe after creating a mixer,
usb_audio_probe() returns an error but leaves the mixer on the card's
mixer_list because chip->num_interfaces > 0.

The driver core then unwinds devres for the secondary interface without
holding ALSA's register_mutex.

Concurrently, if the device is unplugged, snd_usb_mixer_disconnect()
iterates the mixers and invokes:

    if (mixer->private_free)
        mixer->private_free(mixer);

Does this lockless teardown open a time-of-check to time-of-use race window
before the tm context is freed by devres?

> +
> +static void topping_private_free(struct usb_mixer_interface *mixer)
> +{
> +	struct topping_master *tm = mixer->private_data;
> +
> +	if (!tm)
> +		return;
> +
> +	/*
> +	 * Reached from snd_usb_mixer_disconnect(), on an unplug and on an
> +	 * unbind of the audio interface alike.  The action clears
> +	 * mixer->private_data on its way through, so nothing is left
> +	 * pointing at the storage dropped below.
> +	 */
> +	devm_release_action(tm->dev, topping_master_teardown, tm);

[Severity: Critical]
Could this dereference of tm->dev result in a use-after-free regression?

If snd_usb_mixer_disconnect() reads mixer->private_data into tm, but is
then preempted by the concurrent devres unwind, tm could be freed before
devm_release_action() is called.

For example, could this sequence occur:

cpu2 (snd_usb_mixer_disconnect)
topping_private_free()
   tm = mixer->private_data;

cpu1 (devres unwind)
topping_master_teardown()
   tm->mixer->private_data = NULL;
   devres frees tm

cpu2 (snd_usb_mixer_disconnect)
   devm_release_action(tm->dev, ...)

Is there any synchronization missing here to protect against this?

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