Re: [PATCH v5 09/11] ALSA: virtio: prioritize staged capture period work

[sashiko-bot@kernel.org]

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

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/rfc-v2-vz-capture-compat-1.b9ab4c1540dc4691@gmail.com/

[ ... ]

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