[PATCH] usb: gadget: u_audio: read period size while holding stream lock

[PATCH] usb: gadget: u_audio: read period size while holding stream lock

[Yaohan Li]

u_audio_iso_complete() reads the PCM runtime while holding the
substream lock, but calls snd_pcm_lib_period_bytes() after dropping
that lock. A concurrent close can detach and free the runtime in
between, causing a use-after-free when the completion callback reads
the period size.

Calculate the period size while the stream lock is held and use the
saved value after unlocking. This removes the unprotected second
runtime access without changing the period-notification condition.

KASAN reported the use-after-free in u_audio_iso_complete() on the
unmodified kernel. An injection-assisted test stopped the fixed
callback at the protected read while a concurrent close ran; no
use-after-free was reported under that test.

Fixes: 773e53d50e227 ("usb: gadget: u_audio: remove cached period bytes value")
Assisted-by: ZCode:GLM-4
Signed-off-by: Yaohan Li <shunsheng1218@gmail.com>
---
 drivers/usb/gadget/function/u_audio.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c
index f0ee83abcebc7..7f378858146a1 100644
--- a/drivers/usb/gadget/function/u_audio.c
+++ b/drivers/usb/gadget/function/u_audio.c
@@ -156,6 +156,7 @@ static void u_audio_iso_complete(struct usb_ep *ep, struct usb_request *req)
 	struct uac_rtd_params *prm = req->context;
 	struct snd_uac_chip *uac = prm->uac;
 	unsigned int frames, p_pktsize;
+	unsigned int period_bytes;
 	unsigned long long pitched_rate_mil, p_pktsize_residue_mil,
 			residue_frames_mil, div_result;
 
@@ -267,9 +268,10 @@ static void u_audio_iso_complete(struct usb_ep *ep, struct usb_request *req)
 	/* update hw_ptr after data is copied to memory */
 	prm->hw_ptr = (hw_ptr + req->actual) % runtime->dma_bytes;
 	hw_ptr = prm->hw_ptr;
+	period_bytes = snd_pcm_lib_period_bytes(substream);
 	snd_pcm_stream_unlock(substream);
 
-	if ((hw_ptr % snd_pcm_lib_period_bytes(substream)) < req->actual)
+	if ((hw_ptr % period_bytes) < req->actual)
 		snd_pcm_period_elapsed(substream);
 
 exit:
-- 
2.34.1

Re: [PATCH] usb: gadget: u_audio: read period size while holding stream lock

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] usb: gadget: u_audio: race with close in snd_pcm_period_elapsed() after unlock
--

commit a295a8414332708b19b9379c3e997a95e5d868c8
Author: Yaohan Li <shunsheng1218@gmail.com>

usb: gadget: u_audio: read period size while holding stream lock

This commit moves the snd_pcm_lib_period_bytes() call in
u_audio_iso_complete() inside the substream lock critical section to avoid
reading substream->runtime after unlocking.

> diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c
> index f0ee83abcebc7..7f378858146a1 100644
> --- a/drivers/usb/gadget/function/u_audio.c
> +++ b/drivers/usb/gadget/function/u_audio.c
> @@ -156,6 +156,7 @@ static void u_audio_iso_complete(struct usb_ep *ep, struct usb_request *req)
>  	struct uac_rtd_params *prm = req->context;
>  	struct snd_uac_chip *uac = prm->uac;
>  	unsigned int frames, p_pktsize;
> +	unsigned int period_bytes;
>  	unsigned long long pitched_rate_mil, p_pktsize_residue_mil,
>  			residue_frames_mil, div_result;
>  
> @@ -267,9 +268,10 @@ static void u_audio_iso_complete(struct usb_ep *ep, struct usb_request *req)
>  	/* update hw_ptr after data is copied to memory */
>  	prm->hw_ptr = (hw_ptr + req->actual) % runtime->dma_bytes;
>  	hw_ptr = prm->hw_ptr;
> +	period_bytes = snd_pcm_lib_period_bytes(substream);
>  	snd_pcm_stream_unlock(substream);
>  
> -	if ((hw_ptr % snd_pcm_lib_period_bytes(substream)) < req->actual)
> +	if ((hw_ptr % period_bytes) < req->actual)
>  		snd_pcm_period_elapsed(substream);

[Severity: Critical]
Can snd_pcm_period_elapsed() still race with a concurrent close once
u_audio_iso_complete() calls snd_pcm_stream_unlock(substream)?

Because uac_pcm_ops does not implement .sync_stop and card->sync_irq is not
set, snd_pcm_sync_stop() is a no-op. As soon as u_audio_iso_complete() drops
the stream lock, a concurrent close in
snd_pcm_release()->snd_pcm_release_substream() can complete snd_pcm_drop(),
run do_hw_free(), and enter snd_pcm_detach_substream(), which executes
without holding pcm_stream_lock:

sound/core/pcm.c:snd_pcm_detach_substream() {
	...
	if (substream->timer) {
		scoped_guard(spinlock_irq, &substream->timer->lock)
			substream->runtime = NULL;
	} else {
		substream->runtime = NULL;
	}
	mutex_destroy(&runtime->buffer_mutex);
	snd_fasync_free(runtime->fasync);
	kfree(runtime);
	...
}

When (hw_ptr % period_bytes) < req->actual is true, u_audio_iso_complete()
then calls snd_pcm_period_elapsed()->snd_pcm_period_elapsed_under_stream_lock():

sound/core/pcm_lib.c:snd_pcm_period_elapsed_under_stream_lock() {
	struct snd_pcm_runtime *runtime;

	if (PCM_RUNTIME_CHECK(substream))
		return;
	runtime = substream->runtime;

	if (!snd_pcm_running(substream) ||
	    snd_pcm_update_hw_ptr0(substream, 1) < 0)
		goto _end;
	...
 _end:
	snd_kill_fasync(runtime->fasync, SIGIO, POLL_IN);
}

If snd_pcm_detach_substream() has already cleared substream->runtime,
PCM_RUNTIME_CHECK(substream) triggers a kernel warning via snd_BUG_ON(). If
snd_pcm_detach_substream() clears substream->runtime and frees runtime right
after PCM_RUNTIME_CHECK(substream) passes, snd_pcm_running(substream)
dereferences a NULL or freed substream->runtime, and snd_kill_fasync() reads
runtime->fasync from the freed runtime struct.

Would calling snd_pcm_period_elapsed_under_stream_lock(substream) before
snd_pcm_stream_unlock(substream) avoid dropping and reacquiring the stream
lock across the period check?

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