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

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

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.

[ ... ]

+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