Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] ALSA: virtio: NULL deref in virtsnd_disable_event_vq() on probe failure --
commit dbe53b6c2e9b3ca5c54f2da85fde0a3fdaa123b3 Author: Joonho Lee <[email protected]> ALSA: virtio: Retain queued messages through device reset This commit retains the control queue through snd_card_free() during device teardown, detaches remaining virtqueue buffers after device reset, and protects virtqueue callbacks under their respective queue locks. > diff --git a/sound/virtio/virtio_card.c b/sound/virtio/virtio_card.c > index 6f35276416fed..aec4959c973ca 100644 > --- a/sound/virtio/virtio_card.c > +++ b/sound/virtio/virtio_card.c [ ... ] > @@ -174,8 +188,9 @@ static void virtsnd_disable_event_vq(struct virtio_snd > *snd) > struct virtio_snd_event *event; > u32 length; > > + guard(spinlock_irqsave)(&queue->lock); > + snd->event_vq_enabled = false; > if (queue->vqueue) { > - guard(spinlock_irqsave)(&queue->lock); > virtqueue_disable_cb(queue->vqueue); > while ((event = virtqueue_get_buf(queue->vqueue, &length))) > virtsnd_event_dispatch(snd, event); [Severity: High] Can virtsnd_disable_event_vq() dereference a NULL pointer when cleaning up after a failure in virtsnd_probe()? In virtsnd_probe(), virtsnd_find_vqs() populates the event virtqueue with buffers while snd->event_vq_enabled is still false, and virtio_device_ready() marks the device live before virtsnd_build_devs() creates the ALSA objects: sound/virtio/virtio_card.c:virtsnd_probe() { ... rc = virtsnd_find_vqs(snd); if (rc) goto on_exit; virtio_device_ready(vdev); rc = virtsnd_build_devs(snd); if (rc) goto on_exit; virtsnd_enable_event_vq(snd); on_exit: if (rc) virtsnd_remove(vdev); ... } Because VIRTIO_CONFIG_S_DRIVER_OK is set before virtsnd_build_devs(), the device can place events into the event queue while virtsnd_build_devs() is running. With this change, virtsnd_event_notify_cb() sees !snd->event_vq_enabled and leaves those events in the ring. If virtsnd_build_devs() fails before virtsnd_enable_event_vq() is reached (for example, after virtsnd_kctl_parse_cfg() sets snd->nkctls and allocates snd->kctls, but before virtsnd_kctl_build_devs() populates each kctl->kctl), virtsnd_probe() calls virtsnd_remove() -> virtsnd_disable_event_vq(). Since virtsnd_disable_event_vq() unconditionally drains queue->vqueue and calls virtsnd_event_dispatch() without checking whether snd->event_vq_enabled was true, dispatching a pending event against partially initialized state (such as passing &kctl->kctl->id to snd_ctl_notify() in virtsnd_kctl_event() when kctl->kctl is NULL) will dereference a NULL pointer. Should virtsnd_disable_event_vq() only drain and dispatch events if snd->event_vq_enabled was previously true? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
