Re: [RFC PATCH 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 3 potential issue(s) to consider:
- [High] Use-After-Free of `snd_kcontrol` and potentially `snd_card` in `topping_raw_event` due to dereferencing pointers after releasing the spinlock
- [High] Race condition in `topping_unbind` can strand delayed work `m62->keepalive`, leading to a Use-After-Free
- [Medium] API Contract Violation: Unconditional cleanup loop passes NULL pointers to `snd_ctl_remove()`, triggering kernel warnings
--

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

    HID: topping-m62: driver for the M62's vendor controls
    
    The Topping M62 is a USB audio interface whose analogue input gains,
    output volumes and output source selectors are not described by the USB
    Audio Class. They live behind a vendor protocol on a HID-class
    interface, spoken by Topping's M Control Center, which has no Linux
    build.

> diff --git a/drivers/hid/hid-topping-m62.c b/drivers/hid/hid-topping-m62.c
> index 0000000000000..0ef407ffb46b7
> --- /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)
> +{
[ ... ]
> +	scoped_guard(spinlock_irqsave, &m62->lock) {
> +		card = m62->card;
> +		if (m62->val[idx] != value) {
> +			m62->val[idx] = value;
> +			kctl = m62->kctl[idx];
> +		}
> +	}
> +
> +	/*
> +	 * A frame that arrived just as the audio side was unbinding can
> +	 * notify an id that has already gone.  snd_ctl_notify() takes a
> +	 * copy of the id and touches nothing that unbind frees, so such
> +	 * an event is merely wasted.
> +	 */
> +	if (card && kctl)
> +		snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_VALUE, &kctl->id);
                                                                 ^^^^^^^^^

[Severity: High]
Does this code risk a use-after-free by passing a pointer to the kctl id
after releasing the lock?

If the audio component unbinds concurrently, topping_unbind() will execute,
clearing m62->card under the lock and freeing the kctl structures via
snd_ctl_remove(). 

When topping_raw_event() resumes and calls snd_ctl_notify(), the &kctl->id
pointer references freed memory. Because snd_ctl_notify() takes the ID by
reference rather than by copy, it will dereference this freed memory,
leading to memory corruption.

[ ... ]
> +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]);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Medium]
Will this unconditionally pass a NULL pointer to snd_ctl_remove() if a
control fails to allocate during probe?

If topping_add_kctl() fails, topping_bind() jumps to the err_drop label
and calls topping_drop_kctls(). The loop iterates over all indices in
the array and calls snd_ctl_remove() on them.

For any controls that were never allocated, m62->kctl[i] will be NULL.
Calling snd_ctl_remove() on a NULL pointer triggers ALSA's internal
snd_BUG_ON macro, which generates stack traces in dmesg.

> +		m62->kctl[i] = NULL;
> +	}
> +}
[ ... ]
> +static void topping_unbind(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;
> +
> +	cancel_delayed_work_sync(&m62->keepalive);
> +
> +	/*
> +	 * Stop reporting before the controls go, so that a frame in
> +	 * flight cannot look up a control this function is removing.
> +	 */
> +	scoped_guard(spinlock_irqsave, &m62->lock)
> +		m62->card = NULL;
                ^^^^^^^^^^^^^^^^

[Severity: High]
Can a concurrent resume strand the delayed work and cause a use-after-free
if it is scheduled after cancel_delayed_work_sync() executes?

Because the work is cancelled before the m62->card synchronization flag is
cleared, a runtime resume occurring in this narrow window can observe that
m62->card is still valid. 

The topping_resume() function would then erroneously call
schedule_delayed_work(). Since the component is already unbound,
topping_remove() will eventually free the m62 structure without canceling
this newly scheduled work, allowing the timer to fire on freed memory.

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