Re: [RFC PATCH v2 1/2] HID: topping-m62: driver for the M62's vendor controls

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Use-After-Free in topping_bind error path when handling concurrent USB events.
--

commit a582809964c0e1ad990a6ee419fbee8c5cf85dae
Author: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>

HID: topping-m62: driver for the M62's vendor controls

This commit adds a HID driver for the Topping M62 USB audio interface to
expose vendor-specific controls like analog input gains, output volumes, 
and source selectors. These controls are exposed as ALSA mixer elements on
the sound card created by snd-usb-audio.

> diff --git a/drivers/hid/hid-topping-m62.c b/drivers/hid/hid-topping-m62.c
> new file mode 100644
> index 0000000000000..66c037e2b8688
> --- /dev/null
> +++ b/drivers/hid/hid-topping-m62.c
[ ... ]
> +static int topping_raw_event(struct hid_device *hdev,
> +			     struct hid_report *report, u8 *data, int size)
> +{
[ ... ]
> +	guard(spinlock_irqsave)(&m62->lock);
> +
> +	if (m62->val[idx] == value)
> +		return 0;
> +	m62->val[idx] = value;
> +
> +	if (m62->card && m62->kctl[idx])
> +		snd_ctl_notify(m62->card, SNDRV_CTL_EVENT_MASK_VALUE,
> +			       &m62->kctl[idx]->id);

[Severity: High]
If an interrupt packet arrives while the controls are being freed, could
this lead to a use-after-free?

Since hid_device_io_start() was called earlier in probe, topping_raw_event()
can execute concurrently with the error path in topping_bind().

> +
> +	return 0;
> +}
[ ... ]
> +static void topping_drop_kctls(struct topping_m62 *m62, struct snd_card *card)
> +{
> +	int i;
> +
> +	for (i = 0; i < TOPPING_NUM_KCTLS; i++) {
> +		snd_ctl_remove(card, m62->kctl[i]);
> +		m62->kctl[i] = NULL;
> +	}
> +}
[ ... ]
> +static int topping_bind(struct device *comp, struct device *master,
> +			void *master_data)
> +{
> +	struct hid_device *hdev = to_hid_device(comp);
> +	struct topping_m62 *m62 = hid_get_drvdata(hdev);
> +	struct snd_card *card = master_data;
> +	int i, err;
> +
> +	scoped_guard(spinlock_irqsave, &m62->lock)
> +		m62->card = card;
> +
> +	for (i = 0; i < TOPPING_NUM_CTLS; i++) {
> +		err = topping_add_kctl(m62, &topping_ctl,
> +				       topping_m62_ctls[i].name, i, i,
> +				       topping_m62_ctls[i].tlv);
> +		if (err < 0)
> +			goto err_drop;
> +	}
[ ... ]
> +err_drop:
> +	topping_drop_kctls(m62, card);
> +	scoped_guard(spinlock_irqsave, &m62->lock)
> +		m62->card = NULL;
> +	return err;
> +}

[Severity: High]
Could this error path allow the race described above?

If topping_add_kctl() fails, we jump to err_drop and call
topping_drop_kctls(). The controls are removed and freed via snd_ctl_remove()
without holding m62->lock.

Because m62->card is still non-NULL until after topping_drop_kctls() finishes,
a concurrent topping_raw_event() could take the lock, see m62->card is valid,
and access the already-freed m62->kctl[idx]->id.

It looks like topping_unbind() avoids this exact issue by clearing m62->card
under the lock before removing the controls. Should err_drop follow the same
ordering to prevent the race?

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