Re: [PATCH v13 03/12] ALSA: virtio: Retain queued messages through device reset

[sashiko-bot@kernel.org]

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 <mynameisjune111@gmail.com>

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/20261103080821.16983-1-mynameisjune111@gmail.com?part=3