Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] ALSA: virtio: UAF/NULL deref in virtsnd_pcm_period_elapsed() after open fail --
commit fc06cf9505ce3c738b1bc2bf02c081ba34868b30 Author: Joonho Lee <[email protected]> ALSA: virtio: prioritize staged capture period work This commit introduces virtsnd_pcm_schedule_period_elapsed() to queue elapsed_period work onto system_highpri_wq for staged capture streams while keeping playback and non-staged streams on the normal workqueue. All ten producers of elapsed_period work are updated to use this helper. Link: https://lore.kernel.org/all/[email protected]/ [ ... ] > diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c > index 85e7576903d56..d50924b792f65 100644 > --- a/sound/virtio/virtio_pcm_ops.c > +++ b/sound/virtio/virtio_pcm_ops.c > @@ -232,7 +232,7 @@ virtsnd_pcm_pause_restart_fail(struct > virtio_pcm_substream *vss, > VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED; > } > if (notify_xrun) > - schedule_work(&vss->elapsed_period); > + virtsnd_pcm_schedule_period_elapsed(vss); > vss->substream->runtime->stop_operating = true; > } [Severity: High] Can queuing vss->elapsed_period onto system_highpri_wq from virtsnd_pcm_pause_restart_fail() cause a NULL pointer dereference or use-after-free of substream->runtime if virtsnd_pcm_open() fails? During virtsnd_pcm_open(), the driver calls virtsnd_pcm_sync_stop(), which dispatches to virtsnd_pcm_pause_restart_sync_stop() when pause-restart is enabled: sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_pause_restart_sync_stop() { scoped_guard(spinlock_irqsave, &vss->lock) { vss->xfer_enabled = false; vss->stopped = true; state = vss->pause_restart_state; } 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; } ... } Although virtsnd_pcm_pause_restart_sync_stop() cancels vss->elapsed_period at the beginning, all of its error paths call virtsnd_pcm_pause_restart_fail(). Whenever vss->xfer_xrun was false (for example, after a prior session failed in virtsnd_pcm_dev_set_params() -> virtsnd_pcm_pause_restart_send() or left the stream in VIRTSND_PCM_PAUSE_RESTART_PREPARED without starting), notify_xrun is true and virtsnd_pcm_pause_restart_fail() re-queues vss->elapsed_period onto system_highpri_wq after cancel_work_sync() has already completed. When virtsnd_pcm_open() returns this error to snd_pcm_open_substream(), substream->hw_opened is still 0, so snd_pcm_release_substream() skips virtsnd_pcm_close() and immediately calls snd_pcm_detach_substream(), freeing substream->runtime and setting substream->runtime = NULL without holding pcm_stream_lock. When virtsnd_pcm_period_elapsed() then runs on system_highpri_wq for a staged capture stream, it calls snd_pcm_running(substream) before checking whether substream->runtime is NULL: sound/virtio/virtio_pcm.c:virtsnd_pcm_period_elapsed() { ... guard(pcm_stream_lock_irqsave)(substream); if (!snd_pcm_running(substream)) { scoped_guard(spinlock_irqsave, &vss->lock) { runtime = substream->runtime; paused_xrun = runtime && ... } Since snd_pcm_running(substream) unconditionally reads substream->runtime->state, can this dereference a freed or NULL substream->runtime, or leave vss->elapsed_period pending after virtsnd_pcm_sync_stop() returns? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=9
