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

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 [off-list ref]

    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.

[ ... ]

+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