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 <[email protected]> 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/[email protected]?part=3
