Re: [PATCH v5 03/11] ALSA: virtio: Retain queued messages through device reset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ALSA: virtio: NULL vqueue dereference in virtsnd_pcm_msg_send() after freeze
- [High] ALSA: virtio: virtsnd_reset_vqs() leaves stale substream state on reset
--

commit 00a0945381a2dc9d6b91d5ecac16bfd8128b97f3
Author: Joonho Lee <mynameisjune111@gmail.com>

ALSA: virtio: Retain queued messages through device reset

This patch retains the control virtqueue through snd_card_free() during
device removal, blocks new control messages and cancels pending ones across
device reset, and detaches unused virtqueue buffers in virtsnd_reset_vqs()
before deleting the virtqueues.

> diff --git a/sound/virtio/virtio_card.c b/sound/virtio/virtio_card.c
> index 6f35276416fed..df7091b74a521 100644
> --- a/sound/virtio/virtio_card.c
> +++ b/sound/virtio/virtio_card.c
> @@ -337,6 +337,38 @@ static int virtsnd_probe(struct virtio_device *vdev)
>  	return rc;
>  }
>  
> +/**
> + * virtsnd_reset_vqs() - Stop device access and unmap outstanding buffers.
> + * @snd: VirtIO sound device.
> + *
> + * Context: Any context that permits to sleep.
> + */
> +static void virtsnd_reset_vqs(struct virtio_snd *snd)
> +{
> +	struct virtio_device *vdev = snd->vdev;
> +	unsigned int i;
> +
> +	virtio_reset_device(vdev);
> +
> +	/* Deleting a virtqueue does not unmap its outstanding descriptors. */
> +	for (i = 0; i < VIRTIO_SND_VQ_MAX; ++i) {
> +		struct virtio_snd_queue *queue = &snd->queues[i];
> +		unsigned long flags;
> +		void *msg;
> +
> +		spin_lock_irqsave(&queue->lock, flags);
> +		if (queue->vqueue)
> +			while ((msg = virtqueue_detach_unused_buf(queue->vqueue)))
> +				if (i == VIRTIO_SND_VQ_TX || i == VIRTIO_SND_VQ_RX)
> +					virtsnd_pcm_msg_cancel(msg);
> +		spin_unlock_irqrestore(&queue->lock, flags);
> +	}
> +
> +	vdev->config->del_vqs(vdev);
> +	for (i = 0; i < VIRTIO_SND_VQ_MAX; ++i)
> +		snd->queues[i].vqueue = NULL;
> +}

[Severity: High]
Can this lead to a NULL pointer dereference in virtsnd_pcm_msg_send() after
virtsnd_freeze() clears snd->queues[i].vqueue?

During system sleep, do_pcm_suspend() -> snd_pcm_suspend_all() leaves
vss->stopped as false (either because the substream was not running, because
virtsnd_pcm_sync_stop() flushed the queue and cleared vss->stopped, or
because virtsnd_pcm_msg_cancel() only clears vss->xfer_enabled).
virtsnd_freeze() then calls virtsnd_reset_vqs(), which deletes the
virtqueues and sets snd->queues[i].vqueue = NULL, but unlike the control
queue, nothing blocks submissions to the TX/RX PCM queues.

If virtsnd_restore() fails (for example in virtsnd_find_vqs() or feature
negotiation), snd->queues[VIRTIO_SND_VQ_TX/RX].vqueue remains NULL. If
userspace then issues SNDRV_PCM_IOCTL_SYNC_PTR with an advanced appl_ptr,
snd_pcm_common_ioctl() only checks for SNDRV_PCM_STATE_DISCONNECTED (and
snd_power_wait() is a no-op since virtio-snd does not update
card->power_state), calling:

snd_pcm_sync_ptr()
  -> pcm_lib_apply_appl_ptr()
    -> virtsnd_pcm_pb_ack() / virtsnd_pcm_cp_ack()

In sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_pb_ack():
	scoped_guard(spinlock_irqsave, &queue->lock) {
		guard(spinlock)(&vss->lock);
		transfer.indirect = vss->pcm_indirect;
		if (vss->stopped) {
			rc = -EPIPE;
		} else {
			rc = snd_pcm_indirect_playback_transfer(substream,
								&transfer.indirect,
								virtsnd_pcm_trans_copy);
		}

Because vss->stopped is false, this calls virtsnd_pcm_trans_copy() ->
virtsnd_pcm_msg_send():

	struct virtqueue *vqueue = virtsnd_pcm_queue(vss)->vqueue;
	...
	rc = virtqueue_add_sgs(vqueue, psgs, 2, 1, msg, GFP_ATOMIC);

which passes a NULL vqueue to virtqueue_add_sgs() under the spinlocks. Even
when virtsnd_restore() succeeds, the same SNDRV_PCM_IOCTL_SYNC_PTR -> ack()
path can queue PCM buffers onto the newly recreated virtqueue while the
substream is still in SNDRV_PCM_STATE_SUSPENDED (or SNDRV_PCM_STATE_SETUP)
before VIRTIO_SND_R_PCM_SET_PARAMS and VIRTIO_SND_R_PCM_PREPARE have been
sent to the reset device.

[ ... ]

> diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c
> index 5371f4985fd06..762480363b861 100644
> --- a/sound/virtio/virtio_pcm_msg.c
> +++ b/sound/virtio/virtio_pcm_msg.c
> @@ -307,6 +307,35 @@ unsigned int virtsnd_pcm_msg_pending_num(struct virtio_pcm_substream *vss)
>  	return vss->msg_count;
>  }
>  
> +/**
> + * virtsnd_pcm_msg_cancel() - Cancel a message detached during device reset.
> + * @msg: Message returned by virtqueue_detach_unused_buf().
> + *
> + * Reset cancellation retires the queue token without treating it as a device
> + * completion. In particular, it does not advance the PCM hardware pointer or
> + * schedule period work.
> + *
> + * Context: Process context with the PCM queue lock held.
> + */
> +void virtsnd_pcm_msg_cancel(struct virtio_pcm_msg *msg)
> +{
> +	struct virtio_pcm_substream *vss = msg->substream;
> +	bool wake;
> +
> +	{
> +		guard(spinlock)(&vss->lock);
> +		msg->length = 0;
> +		vss->xfer_enabled = false;
> +		if (WARN_ON_ONCE(!vss->msg_count))
> +			return;
> +		vss->msg_count--;
> +		wake = !vss->msg_count;
> +	}
> +
> +	if (wake)
> +		wake_up_all(&vss->msg_empty);
> +}

[Severity: High]
Should virtsnd_reset_vqs() / virtsnd_pcm_msg_cancel() also reconcile
vss->stopped and vss->suspended when resetting the device during
virtsnd_freeze()?

First, if virtsnd_pcm_sync_stop() fails or times out during
snd_pcm_suspend_all() before virtsnd_freeze() (or if a prior stop/start
failed before suspend), vss->stopped remains true while buffers remain
queued. virtsnd_freeze() -> virtsnd_reset_vqs() then resets the VirtIO
device and detaches all pending buffers via virtsnd_pcm_msg_cancel(),
decrementing vss->msg_count to 0, but leaves vss->stopped as true.

After virtsnd_restore(), vss->stopped blocks virtsnd_pcm_prepare() and
virtsnd_pcm_hw_params() with -EBADFD:

sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_prepare() {
	scoped_guard(spinlock_irqsave, &vss->lock)
		stopped = vss->stopped;
	if (stopped) {
		dev_err(&vdev->dev, "SID %u: stream release failed\n",
			vss->sid);
		substream->runtime->stop_operating = true;
		return -EBADFD;
	}

And if userspace tries to recover via snd_pcm_sync_stop() or by closing and
reopening the substream via virtsnd_pcm_open():

sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_open() {
	...
	scoped_guard(spinlock_irqsave, &vss->lock)
		vss->stopped |= !!vss->msg_count;
	vss->suspended = false;

	return virtsnd_pcm_sync_stop(substream);
}

virtsnd_pcm_sync_stop() sees vss->stopped == true and sends
VIRTIO_SND_R_PCM_RELEASE to the freshly reset device before
VIRTIO_SND_R_PCM_SET_PARAMS has been sent. If the device rejects RELEASE for
the unconfigured stream, virtsnd_pcm_sync_stop() fails and leaves
vss->stopped set, permanently wedging the substream.

Second, if a substream has queued buffers while in a state where
snd_pcm_running() is false at suspend time (such as SNDRV_PCM_STATE_PAUSED,
capture SNDRV_PCM_STATE_DRAINING, or playback SNDRV_PCM_STATE_PREPARED with
pre-queued buffers), snd_pcm_do_suspend() skips
virtsnd_pcm_trigger(SNDRV_PCM_TRIGGER_SUSPEND) while snd_pcm_post_suspend()
places the substream into SNDRV_PCM_STATE_SUSPENDED with vss->suspended ==
false. When virtsnd_pcm_msg_cancel() drains vss->msg_count to 0 without
setting vss->suspended = true, a subsequent virtsnd_pcm_prepare() after
virtsnd_restore() sees !vss->suspended and virtsnd_pcm_msg_pending_num(vss)
== 0, skips virtsnd_pcm_dev_set_params(), and sends VIRTIO_SND_R_PCM_PREPARE
to a reset device that has no stream parameters configured.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027234334.32551-1-mynameisjune111@gmail.com?part=3