Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider: - [High] ALSA: virtio: UAF and NULL deref of runtime in virtsnd_pcm_period_elapsed() - [High] ALSA: virtio: virtsnd_pcm_pause_restart_send() bricks stream on signal or error - [Medium] ALSA: virtio: pause_restart_state not reconciled across suspend and resume - [Medium] ALSA: virtio: virtsnd_pcm_pause_restart_pause() skips flush on RELEASE_SENT - [Medium] ALSA: virtio: unlocked state reset in virtsnd_pcm_prepare() races with ack - [Medium] ALSA: virtio: virtsnd_pcm_cp_ack() checks acked bytes against copied_total - [Low] ALSA: virtio: undocumented module parameters in virtio_pcm_ops.c - [Low] ALSA: virtio: unused pause_restart parameter in virtsnd_pcm_start_failed() --
commit d608f6a99cc3b67a96be954c9e03a4e0866d62bf Author: Joonho Lee <[email protected]> ALSA: virtio: stage VZ capture and restart paused streams This commit adds opt-in capture staging with response validation and accounting, along with a pause release and reprepare state machine for capture streams. > diff --git a/sound/virtio/virtio_pcm.c b/sound/virtio/virtio_pcm.c > index be3893de40a55..a3478fc8848db 100644 > --- a/sound/virtio/virtio_pcm.c > +++ b/sound/virtio/virtio_pcm.c > @@ -310,8 +310,94 @@ static void virtsnd_pcm_period_elapsed(struct > work_struct *work) > { > struct virtio_pcm_substream *vss = > container_of(work, struct virtio_pcm_substream, elapsed_period); > + struct snd_pcm_substream *substream = vss->substream; > + struct snd_pcm_runtime *runtime; > + size_t period_bytes; > + u64 period_floor; > + u64 snapshot; > + bool staged_capture; > + bool paused_xrun = false; > + bool notify = false; > + bool requeue = false; > + > + scoped_guard(spinlock_irqsave, &vss->lock) { > + staged_capture = vss->direction == SNDRV_PCM_STREAM_CAPTURE && > + vss->capture_staging; > + } > + > + if (!staged_capture) { > + snd_pcm_period_elapsed(substream); > + return; > + } > + > + /* Serialize the pointer snapshot with ALSA's pointer and ACK > callbacks. */ > + guard(pcm_stream_lock_irqsave)(substream); > + if (!snd_pcm_running(substream)) { > + scoped_guard(spinlock_irqsave, &vss->lock) { > + runtime = substream->runtime; > + paused_xrun = runtime && > + runtime->state == SNDRV_PCM_STATE_PAUSED > && > + vss->runtime_active && vss->xfer_xrun; > + } [Severity: High] Can virtsnd_pcm_period_elapsed() dereference a null or freed substream->runtime when calling snd_pcm_running(substream) here? In virtsnd_pcm_pause_restart_sync_stop() in sound/virtio/virtio_pcm_ops.c, cancel_work_sync(&vss->elapsed_period) is called at entry, but every subsequent error branch calls virtsnd_pcm_pause_restart_fail(): cancel_work_sync(&vss->elapsed_period); if (state == VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED || virtsnd_pcm_pause_restart_transitioning(state)) { virtsnd_pcm_pause_restart_fail(vss, true); ... return -EIO; } In turn, virtsnd_pcm_pause_restart_fail() checks !vss->xfer_xrun and schedules vss->elapsed_period even when vss->runtime_active and vss->xfer_enabled are false: scoped_guard(spinlock_irqsave, &vss->lock) { notify_xrun = !vss->xfer_xrun; ... } if (notify_xrun) schedule_work(&vss->elapsed_period); When virtsnd_pcm_open() calls virtsnd_pcm_sync_stop() -> virtsnd_pcm_pause_restart_sync_stop() and it fails (for example, after a prior failed control message or a RELEASE flush timeout), vss->elapsed_period is re-queued and virtsnd_pcm_open() returns an error to snd_pcm_open_substream(). Because substream->ops->open() failed before substream->hw_opened is set, snd_pcm_release_substream() skips virtsnd_pcm_close() and frees substream->runtime via snd_pcm_detach_substream() without holding pcm_stream_lock. When virtsnd_pcm_period_elapsed() then runs, snd_pcm_running(substream) dereferences substream->runtime->state before substream->runtime is checked for null. Should virtsnd_pcm_pause_restart_fail() avoid scheduling vss->elapsed_period when the stream is not active, and should virtsnd_pcm_period_elapsed() also verify substream->runtime before calling snd_pcm_running(substream)? [ ... ] > diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c > index f74bf365065b6..003832d112551 100644 > --- a/sound/virtio/virtio_pcm_ops.c > +++ b/sound/virtio/virtio_pcm_ops.c > @@ -3,10 +3,41 @@ > * virtio-snd: Virtio sound device > * Copyright (C) 2021 OpenSynergy GmbH > */ > +#include <linux/moduleparam.h> > +#include <linux/overflow.h> > #include <sound/pcm_params.h> > > #include "virtio_card.h" > > +bool virtsnd_capture_staging; > +module_param_named(capture_staging, virtsnd_capture_staging, bool, 0444); > +MODULE_PARM_DESC(capture_staging, > + "Use request-owned buffers for experimental PCM capture"); > + > +bool virtsnd_capture_packed_status; > +module_param_named(capture_packed_status, virtsnd_capture_packed_status, > + bool, 0444); > +MODULE_PARM_DESC(capture_packed_status, > + "Read staged capture status after the actual audio payload"); > + > +bool virtsnd_capture_allow_short; > +module_param_named(capture_allow_short, virtsnd_capture_allow_short, > + bool, 0444); > +MODULE_PARM_DESC(capture_allow_short, > + "Accept non-empty short staged capture responses"); > + > +bool virtsnd_capture_allow_empty; > +module_param_named(capture_allow_empty, virtsnd_capture_allow_empty, > + bool, 0444); > +MODULE_PARM_DESC(capture_allow_empty, > + "Retry bounded empty startup capture responses"); > + > +static bool virtsnd_capture_pause_restart; > +module_param_named(capture_pause_restart, virtsnd_capture_pause_restart, > + bool, 0444); > +MODULE_PARM_DESC(capture_pause_restart, > + "Release and reprepare staged capture streams across pause"); [Severity: Low] Should these five new module parameters (capture_staging, capture_packed_status, capture_allow_short, capture_allow_empty, and capture_pause_restart) be documented in Documentation/admin-guide/kernel-parameters.txt? > @@ -98,15 +129,195 @@ static const struct virtsnd_a2v_rate g_a2v_rate_map[] = > { > > static int virtsnd_pcm_sync_stop(struct snd_pcm_substream *substream); > > -static void virtsnd_pcm_start_failed(struct virtio_pcm_substream *vss) > +static void virtsnd_pcm_start_failed(struct virtio_pcm_substream *vss, > + bool pause_restart) > { > scoped_guard(spinlock_irqsave, &vss->lock) { > vss->xfer_enabled = false; > vss->stopped = true; > + if (pause_restart) > + vss->xfer_xrun = true; > } > vss->substream->runtime->stop_operating = true; > } [Severity: Low] Is the pause_restart parameter in virtsnd_pcm_start_failed() still needed? Both call sites in virtsnd_pcm_trigger() pass false because the pause_restart path returns early via virtsnd_pcm_pause_restart_start(). [ ... ] > +static int > +virtsnd_pcm_pause_restart_send(struct virtio_pcm_substream *vss, > + struct virtio_snd_msg *msg, > + unsigned long allowed_states, > + enum virtio_pcm_pause_restart_state > submitted_state, > + enum virtio_pcm_pause_restart_state > success_state) > +{ > + enum virtio_pcm_pause_restart_state state; > + int rc; > + > + if (!virtsnd_pcm_pause_restart_enabled(vss)) > + return virtsnd_ctl_msg_send_sync(vss->snd, msg); > + > + scoped_guard(spinlock_irqsave, &vss->lock) { > + state = vss->pause_restart_state; > + if (state == VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED) { > + rc = -EIO; > + } else if (virtsnd_pcm_pause_restart_transitioning(state)) { > + vss->pause_restart_state = > + VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED; > + rc = -EIO; > + } else if (!(allowed_states & BIT(state))) { > + rc = -EBADFD; > + } else { > + vss->pause_restart_state = submitted_state; > + rc = 0; > + } > + } > + > + if (rc) { > + virtsnd_ctl_msg_unref(msg); > + return rc; > + } > + > + rc = virtsnd_ctl_msg_send_sync(vss->snd, msg); > + scoped_guard(spinlock_irqsave, &vss->lock) { > + if (rc || vss->pause_restart_state != submitted_state) { > + vss->pause_restart_state = > + VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED; > + if (!rc) > + rc = -EIO; > + } else { > + vss->pause_restart_state = success_state; > + } > + } [Severity: High] What happens if virtsnd_ctl_msg_send_sync() returns -ERESTARTSYS due to a pending signal (or fails with a control error) during SET_PARAMS, PREPARE, or START? In virtsnd_ctl_msg_send(), only VIRTIO_SND_R_PCM_STOP and VIRTIO_SND_R_PCM_RELEASE use uninterruptible waits; SET_PARAMS, PREPARE, and START use wait_for_completion_interruptible_timeout(), which returns -ERESTARTSYS immediately when a signal is pending (including during close or snd_pcm_drop() of a paused stream, where ALSA core first unpauses via SNDRV_PCM_TRIGGER_PAUSE_RELEASE -> virtsnd_pcm_pause_restart_start()). Furthermore, in virtsnd_pcm_pause_restart_start(), virtsnd_pcm_capture_send() queues RX capture buffers into the virtqueue (incrementing vss->msg_count) before calling virtsnd_pcm_pause_restart_send() for VIRTIO_SND_R_PCM_START: scoped_guard(spinlock_irqsave, &queue->lock) { guard(spinlock)(&vss->lock); ... rc = virtsnd_pcm_capture_send(vss); if (!rc) vss->xfer_enabled = true; } ... rc = virtsnd_pcm_pause_restart_send(vss, msg, BIT(VIRTSND_PCM_PAUSE_RESTART_PREPARED), VIRTSND_PCM_PAUSE_RESTART_START_SUBMITTED, VIRTSND_PCM_PAUSE_RESTART_RUNNING); When virtsnd_pcm_pause_restart_send() sets vss->pause_restart_state to VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED on any non-zero rc, virtsnd_pcm_pause_restart_sync_stop() immediately aborts with -EIO without sending VIRTIO_SND_R_PCM_RELEASE or waiting for vss->msg_empty. Doesn't this permanently wedge the substream until device reprobe, while also leaving the queued RX buffers in the virtqueue so virtsnd_pcm_hw_free() cannot free vss->msgs and msg->data on close? [ ... ] > @@ -296,9 +564,22 @@ static int virtsnd_pcm_prepare(struct snd_pcm_substream > *substream) > { > struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream); > struct virtio_device *vdev = vss->snd->vdev; > - struct virtio_snd_msg *msg; > + bool pause_restart = virtsnd_pcm_pause_restart_enabled(vss); > + int rc; > bool stopped; > > + if (pause_restart) { > + enum virtio_pcm_pause_restart_state state; > + > + scoped_guard(spinlock_irqsave, &vss->lock) > + state = vss->pause_restart_state; > + if (state == VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED || > + virtsnd_pcm_pause_restart_transitioning(state)) { > + virtsnd_pcm_pause_restart_fail(vss, true); > + return -EIO; > + } > + } > + > scoped_guard(spinlock_irqsave, &vss->lock) > stopped = vss->stopped; > if (stopped) { [Severity: Medium] How are vss->pause_restart_state and vss->stopped reconciled across system suspend and resume? During virtsnd_freeze(), virtsnd_ctl_msg_cancel_all() cancels any in-flight control messages (which sets vss->pause_restart_state to VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED in virtsnd_pcm_pause_restart_send()) and virtsnd_reset_vqs() resets the VirtIO device, returning all streams on the device to their initial unconfigured state. However, neither virtsnd_freeze() nor virtsnd_restore() resets vss->pause_restart_state to VIRTSND_PCM_PAUSE_RESTART_NONE. In addition, if snd_pcm_suspend_all() suspends a stream that is currently in SNDRV_PCM_STATE_PAUSED with capture_pause_restart enabled, snd_pcm_do_suspend() skips ops->trigger(SNDRV_PCM_TRIGGER_SUSPEND) and does not set runtime->stop_operating because snd_pcm_running(substream) is false. That leaves vss->stopped set to true and vss->suspended set to false in SNDRV_PCM_STATE_SUSPENDED, so a subsequent snd_pcm_prepare() skips virtsnd_pcm_sync_stop() and fails at the if (stopped) check in virtsnd_pcm_prepare() with -EBADFD. Should virtsnd_freeze() / virtsnd_restore() and virtsnd_pcm_prepare() reconcile vss->pause_restart_state and vss->stopped after suspend? > @@ -321,16 +602,23 @@ static int virtsnd_pcm_prepare(struct snd_pcm_substream > *substream) > struct snd_pcm_runtime *runtime = substream->runtime; > unsigned int buffer_bytes = snd_pcm_lib_buffer_bytes(substream); > unsigned int period_bytes = snd_pcm_lib_period_bytes(substream); > - int rc; > > rc = virtsnd_pcm_dev_set_params(vss, buffer_bytes, period_bytes, > runtime->channels, > runtime->format, runtime->rate); > - if (rc) > + if (rc) { > + if (pause_restart) > + virtsnd_pcm_pause_restart_fail(vss, false); > return rc; > + } > } > > - vss->xfer_xrun = false; > + /* ALSA prepare resets its pointers even after a suspended stream. */ > + if (vss->capture_staging) > + vss->hw_ptr = 0; > + > + if (!pause_restart) > + vss->xfer_xrun = false; > vss->suspended = false; > vss->msg_count = 0; > virtsnd_pcm_msg_reset(vss); > @@ -340,12 +628,210 @@ static int virtsnd_pcm_prepare(struct > snd_pcm_substream *substream) > vss->pcm_indirect.hw_buffer_size = > snd_pcm_lib_buffer_bytes(substream); [Severity: Medium] Can these unlocked updates to vss->buffer_bytes, vss->hw_ptr, vss->xfer_xrun, vss->msg_count, and vss->pcm_indirect in virtsnd_pcm_prepare() race with a concurrent virtsnd_pcm_cp_ack() call? Because vpcm->pcm->nonatomic is true, snd_pcm_prepare() calls virtsnd_pcm_prepare() under runtime->buffer_mutex without holding pcm_stream_lock. Meanwhile, SNDRV_PCM_IOCTL_SYNC_PTR in snd_pcm_sync_ptr() acquires pcm_stream_lock and invokes pcm_lib_apply_appl_ptr() -> virtsnd_pcm_cp_ack(), which enters the staged capture path whenever vss->runtime_active is true (set in virtsnd_pcm_hw_params()). If virtsnd_pcm_cp_ack() runs after virtsnd_pcm_prepare() sets vss->hw_ptr = 0 but before virtsnd_pcm_msg_reset() zeroes vss->capture_copied_total under vss->lock, virtsnd_pcm_capture_counters_valid(vss) in virtsnd_pcm_cp_ack() can see vss->hw_ptr != vss->capture_copied_total % vss->buffer_bytes, set vss->xfer_xrun = true, and return -EPIPE, while memset(&vss->pcm_indirect, 0, sizeof(vss->pcm_indirect)) also races with virtsnd_pcm_cp_ack() reading and updating vss->pcm_indirect.appl_ptr. Should all of these state resets in virtsnd_pcm_prepare() be performed together under vss->lock? [ ... ] > +static int virtsnd_pcm_pause_restart_pause(struct virtio_pcm_substream *vss) > +{ > + struct virtio_snd_msg *msg; > + enum virtio_pcm_pause_restart_state state; > + int rc; > + > + scoped_guard(spinlock_irqsave, &vss->lock) { > + state = vss->pause_restart_state; > + if (state == VIRTSND_PCM_PAUSE_RESTART_RELEASE_SENT || > + state == VIRTSND_PCM_PAUSE_RESTART_RELEASED) > + return 0; > + vss->xfer_enabled = false; > + vss->stopped = true; > + } [Severity: Medium] Should virtsnd_pcm_pause_restart_pause() return 0 immediately when state is VIRTSND_PCM_PAUSE_RESTART_RELEASE_SENT? In virtsnd_pcm_pause_release_flush(), sending VIRTIO_SND_R_PCM_RELEASE sets vss->pause_restart_state to VIRTSND_PCM_PAUSE_RESTART_RELEASE_SENT before waiting on vss->msg_empty: rc = virtsnd_pcm_pause_restart_send(vss, msg, allowed_states, VIRTSND_PCM_PAUSE_RESTART_RELEASE_SUBMITTED, VIRTSND_PCM_PAUSE_RESTART_RELEASE_SENT); if (rc) return rc; waited = wait_event_timeout(vss->msg_empty, !virtsnd_pcm_msg_pending_num(vss), js); If that wait times out, vss->pause_restart_state remains VIRTSND_PCM_PAUSE_RESTART_RELEASE_SENT while RX messages may still be pending, and ALSA keeps the substream in SNDRV_PCM_STATE_RUNNING. Unlike virtsnd_pcm_pause_restart_sync_stop(), which waits on vss->msg_empty when in VIRTSND_PCM_PAUSE_RESTART_RELEASE_SENT and transitions to VIRTSND_PCM_PAUSE_RESTART_RELEASED, virtsnd_pcm_pause_restart_pause() returns 0 immediately without waiting for the queue to drain or updating the state to VIRTSND_PCM_PAUSE_RESTART_RELEASED. A subsequent SNDRV_PCM_TRIGGER_PAUSE_RELEASE in virtsnd_pcm_pause_restart_start(vss, true) will then fail with -EBADFD because it requires state == VIRTSND_PCM_PAUSE_RESTART_RELEASED. [ ... ] > @@ -586,11 +1233,66 @@ static int virtsnd_pcm_cp_ack(struct snd_pcm_substream > *substream) > { > struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream); > struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss); > + struct snd_pcm_runtime *runtime = substream->runtime; > struct virtsnd_pcm_transfer transfer = {}; > int rc; > > scoped_guard(spinlock_irqsave, &queue->lock) { > guard(spinlock)(&vss->lock); > + if (vss->capture_staging) { > + snd_pcm_uframes_t appl_ptr; > + snd_pcm_sframes_t diff; > + size_t bytes; > + u64 next_acked_total; > + > + if (!vss->runtime_active || !runtime || > !runtime->control || > + !runtime->dma_area) > + return -EBADFD; > + if (vss->xfer_xrun) > + return -EPIPE; > + if (!virtsnd_pcm_capture_counters_valid(vss)) { > + vss->xfer_xrun = true; > + schedule_work(&vss->elapsed_period); > + return -EPIPE; > + } > + appl_ptr = runtime->control->appl_ptr; > + diff = appl_ptr - vss->pcm_indirect.appl_ptr; > + > + if (diff < -(snd_pcm_sframes_t)(runtime->boundary / 2)) > + diff += runtime->boundary; > + if (diff < 0 || > + (snd_pcm_uframes_t)diff > runtime->buffer_size) { > + vss->xfer_xrun = true; > + schedule_work(&vss->elapsed_period); > + return -EPIPE; > + } > + > + /* ACK includes any subperiod tail already copied to > the ring. */ > + bytes = frames_to_bytes(runtime, diff); > + if (bytes > vss->capture_ready || > + check_add_overflow(vss->capture_acked_total, > (u64)bytes, > + &next_acked_total) || > + next_acked_total > vss->capture_copied_total) { > + vss->xfer_xrun = true; > + schedule_work(&vss->elapsed_period); > + return -EPIPE; > + } [Severity: Medium] Should virtsnd_pcm_cp_ack() validate next_acked_total against vss->capture_pointer_seen_total rather than vss->capture_copied_total? When virtsnd_pcm_capture_complete() copies completed RX payloads into runtime->dma_area, it advances vss->capture_copied_total and vss->capture_ready immediately, whereas runtime->status->hw_ptr and vss->capture_pointer_seen_total only advance when virtsnd_pcm_cp_pointer() runs. If userspace invokes SNDRV_PCM_IOCTL_SYNC_PTR without SNDRV_PCM_SYNC_PTR_HWSYNC (or while in SNDRV_PCM_STATE_PAUSED, where do_pcm_hwsync() is a no-op) with appl_ptr advancing into the range between vss->capture_pointer_seen_total and vss->capture_copied_total, snd_pcm_sync_ptr() calls pcm_lib_apply_appl_ptr() -> virtsnd_pcm_cp_ack() without calling virtsnd_pcm_cp_pointer(). Because virtsnd_pcm_cp_ack() checks next_acked_total > vss->capture_copied_total, it accepts the update and allows runtime->control->appl_ptr to move ahead of runtime->status->hw_ptr, causing snd_pcm_capture_avail() to underflow and wrap around runtime->boundary. (Note: a separate issue here where a virtsnd_pcm_capture_send() failure returns an error after committing vss->capture_acked_total is addressed in a later commit in this series, "ALSA: virtio: account staged capture reads before RX refill".) -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
