[PATCH] ALSA: usx2y: Initialize idle URBs before submitting streams

[PATCH] ALSA: usx2y: Initialize idle URBs before submitting streams

[YuanZhe Liu]

usb_stream_start() submits the first two capture and playback URBs and
only afterwards assigns sk->idle_inurb, sk->idle_outurb,
sk->completed_inurb and sk->completed_outurb to the remaining two URBs
of each direction.

A transfer can complete as soon as usb_submit_urb() has been called, so
the completion handler i_capture_start() or i_playback_start() may run
before those pointers are set.  While the stream is still being
synchronized (state below usb_stream_sync1), stream_start() then calls
playback_prep_freqn(sk, sk->idle_outurb) with sk->idle_outurb still
NULL, and the packet descriptors are written through that NULL pointer:

  Oops: general protection fault, probably for non-canonical address
  0xdffffc0000000018: 0000 [#1] SMP KASAN
  KASAN: null-ptr-deref in range [0x00000000000000c0-0x00000000000000c7]
  RIP: 0010:playback_prep_freqn+0x11c/0x4a0 sound/usb/usx2y/usb_stream.c:33
  Call Trace:
   <IRQ>
   stream_start+0x5dc/0x8d0 sound/usb/usx2y/usb_stream.c:596
   i_capture_start+0x3da/0x710 sound/usb/usx2y/usb_stream.c:654
   __usb_hcd_giveback_urb+0x238/0x550 drivers/usb/core/hcd.c:1657
   usb_giveback_urb_bh+0x20b/0x530 drivers/usb/core/hcd.c:1691
   tasklet_hi_action+0xa/0x20 kernel/softirq.c:994
   handle_softirqs+0x16f/0x440 kernel/softirq.c:645
   </IRQ>

The four URBs referenced by these pointers are allocated long before and
do not depend on any transfer state, so assign them before the submit
loop.  Keeping the assignment inside the retry path also restores a
well-defined URB rotation when the start frame mismatch handling kills
and resubmits the streams.

Fixes: 030a07e441296c372f946cd4065b5d831d8dc40c ("ALSA: Add USB US122L driver")
Cc: stable@vger.kernel.org
Co-developed-by: Bin Yu <byu@xidian.edu.cn>
Signed-off-by: Bin Yu <byu@xidian.edu.cn>
Co-developed-by: Cheng Wen <wencheng@xidian.edu.cn>
Signed-off-by: Cheng Wen <wencheng@xidian.edu.cn>
Signed-off-by: YuanZhe Liu <25031212351@stu.xidian.edu.cn>
---
 sound/usb/usx2y/usb_stream.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/sound/usb/usx2y/usb_stream.c b/sound/usb/usx2y/usb_stream.c
index 3122cf653273..7d621280fba3 100644
--- a/sound/usb/usx2y/usb_stream.c
+++ b/sound/usb/usx2y/usb_stream.c
@@ -682,6 +682,10 @@ int usb_stream_start(struct usb_stream_kernel *sk)
 	s->sync_packet = -1;
 	s->inpacket_head = -1;
 	sk->iso_frame_balance = 0;
+	sk->idle_inurb = sk->inurb[USB_STREAM_NURBS - 2];
+	sk->idle_outurb = sk->outurb[USB_STREAM_NURBS - 2];
+	sk->completed_inurb = sk->inurb[USB_STREAM_NURBS - 1];
+	sk->completed_outurb = sk->outurb[USB_STREAM_NURBS - 1];
 	++try;
 	for (u = 0; u < 2; u++) {
 		struct urb *inurb = sk->inurb[u];
@@ -739,11 +743,6 @@ int usb_stream_start(struct usb_stream_kernel *sk)
 		return -EFAULT;
 	}
 
-	sk->idle_inurb = sk->inurb[USB_STREAM_NURBS - 2];
-	sk->idle_outurb = sk->outurb[USB_STREAM_NURBS - 2];
-	sk->completed_inurb = sk->inurb[USB_STREAM_NURBS - 1];
-	sk->completed_outurb = sk->outurb[USB_STREAM_NURBS - 1];
-
 /* wait, check */
 	{
 		int wait_ms = 3000;
-- 
2.34.1

Re: [PATCH] ALSA: usx2y: Initialize idle URBs before submitting streams

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ALSA: usx2y: race in usb_stream_start() when URB 0 completes before URB 1
--

commit 136073d2c8fceba09fe79c9e8359e6b5813457a2
Author: YuanZhe Liu <25031212351@stu.xidian.edu.cn>

ALSA: usx2y: Initialize idle URBs before submitting streams

This commit moves the initialization of sk->idle_inurb, sk->idle_outurb,
sk->completed_inurb, and sk->completed_outurb before the URB submission loop
in usb_stream_start() so that early URB completion callbacks do not
dereference NULL pointers.

> diff --git a/sound/usb/usx2y/usb_stream.c b/sound/usb/usx2y/usb_stream.c
> index 3122cf653273d..7d621280fba33 100644
> --- a/sound/usb/usx2y/usb_stream.c
> +++ b/sound/usb/usx2y/usb_stream.c
> @@ -682,6 +682,10 @@ int usb_stream_start(struct usb_stream_kernel *sk)
>  	s->sync_packet = -1;
>  	s->inpacket_head = -1;
>  	sk->iso_frame_balance = 0;
> +	sk->idle_inurb = sk->inurb[USB_STREAM_NURBS - 2];
> +	sk->idle_outurb = sk->outurb[USB_STREAM_NURBS - 2];
> +	sk->completed_inurb = sk->inurb[USB_STREAM_NURBS - 1];
> +	sk->completed_outurb = sk->outurb[USB_STREAM_NURBS - 1];
>  	++try;
>  	for (u = 0; u < 2; u++) {
>  		struct urb *inurb = sk->inurb[u];

[Severity: Medium]
If both sk->inurb[0] and sk->outurb[0] complete in softirq context after
iteration u = 0 submits them, but before iteration u = 1 finishes in process
context, can stream_start() race with usb_stream_start() and submit URB 2
ahead of URB 1?

In sound/usb/usx2y/usb_stream.c, iteration u = 0 prepares sk->outurb[0] and
sk->inurb[0] with equal number_of_packets and submits both via
usb_submit_urb(). When both complete in softirq context via
i_capture_start() and i_playback_start(), balance_capture() and
balance_playback() bring sk->iso_frame_balance back to 0 and call
stream_start():

sound/usb/usx2y/usb_stream.c:stream_start() {
	...
	} else
		playback_prep_freqn(sk, sk->idle_outurb);

	if (submit_urbs(sk, inurb, outurb) < 0)
		return;
	...
}

While s->state is below usb_stream_sync1, stream_start() calls
playback_prep_freqn(sk, sk->idle_outurb) on sk->outurb[2] in softirq context
at the same time usb_stream_start() is running playback_prep_freqn(sk,
sk->outurb[1]) for u = 1 in process context:

sound/usb/usx2y/usb_stream.c:playback_prep_freqn() {
	struct usb_stream *s = sk->s;
	int pack, lb = 0;

	for (pack = 0; pack < sk->n_o_ps; pack++) {
		int l = usb_stream_next_packet_size(sk);

		if (s->idle_outsize + lb + l > s->period_size)
			goto check;

		sk->out_phase = sk->out_phase_peeked;
		urb->iso_frame_desc[pack].offset = lb;
		urb->iso_frame_desc[pack].length = l;
		lb += l;
	}

check:
	urb->number_of_packets = pack;
	urb->transfer_buffer_length = lb;
	s->idle_outsize += lb - s->period_size;
}

Because there is no synchronization between process context and the softirq
completion handler, both calls concurrently read and update sk->out_phase,
sk->out_phase_peeked, and s->idle_outsize. Immediately afterwards,
stream_start() calls submit_urbs(), which submits sk->inurb[2] and
sk->outurb[2] and rotates the idle/completed URB pointers before iteration
u = 1 has submitted sk->inurb[1] and sk->outurb[1].

Should both sk->outurb[0] and sk->outurb[1] be prepared before submitting the
initial URBs, or should softirqs be disabled around the initial preparation
and submission loop in usb_stream_start()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028103158.22996-1-25031212351@stu.xidian.edu.cn?part=1