Re: [PATCH 2/9] ALSA: usb-audio: Fix possible race at sync of urb completions

[Takashi Iwai <tiwai@suse.de>]

On Tue, 22 Apr 2025 01:38:59 +0200,
Hillf Danton wrote:
> 
> On Mon, 21 Apr 2025 16:36:30 +0200 Takashi Iwai wrote:
> > On Mon, 21 Apr 2025 12:43:42 +0200, Hillf Danton wrote:
> > > I misread "Which reads and writes are you trying to solve?" though I
> > > showed the read/write, but it is a bad case particulay with UAF.
> > > 
> > > Could you tell us what will happen if the race is not fixed? Could ep
> > > be freed with in-flight urbs for example?
> > 
> > Before the patch, wait_clear_urbs() might return earlier than actually
> > all pending eps are finished, so it can be UAF.
> > 
> Got it.
> 
> > > Is it still race if the wait loop in wait_clear_urbs() ends before the
> > > urb complete callbace completes, given the last sentence in your commit
> > > message? If nope, igore my noise please.
> > 
> > Well, your concern about the missing barrier -- that would
> > wait_clear_urbs() missing the refcount decrement, hence it would be
> > rather to make the return delayed.  So it shouldn't lead to further
> > UAF, but at most it might lead to an unnecessary delay.
> > 
> > That said, I'm willing to take a fix even for a theoretical issue if
> > it clarifies what it really fixes.  But scratching a random surface
> > isn't what we want.
> > 
> Thank you for shedding light on the race, given
> a) the mb in 26fbe9772b8c ("USB: core: Fix hang in usb_kill_urb by adding memory barriers")
> b) the urb complete callback is invoked in giveback, see __usb_hcd_giveback_urb()
> 
> use the urb routines instead to close the race.

I'm afraid that it can break things as of this form; the stopped
stream might be restarted without reinitializing URBs.  That is, this
isn't called only from disconnect or close, but also just for stopping
the stream in the middle, too.


Takashi

> 
> Note the one second timeout is also cut.
> 
> 	Hillf
> 
> --- x/sound/usb/endpoint.c
> +++ y/sound/usb/endpoint.c
> @@ -996,24 +996,14 @@ void snd_usb_endpoint_suspend(struct snd
>   */
>  static int wait_clear_urbs(struct snd_usb_endpoint *ep)
>  {
> -	unsigned long end_time = jiffies + msecs_to_jiffies(1000);
> -	int alive;
> -
>  	if (atomic_read(&ep->state) != EP_STATE_STOPPING)
>  		return 0;
>  
> -	do {
> -		alive = atomic_read(&ep->submitted_urbs);
> -		if (!alive)
> -			break;
> -
> -		schedule_timeout_uninterruptible(1);
> -	} while (time_before(jiffies, end_time));
> +	for (int i = 0; i < ep->nurbs; i++) {
> +		struct snd_urb_ctx *u = &ep->urb[i];
>  
> -	if (alive)
> -		usb_audio_err(ep->chip,
> -			"timeout: still %d active urbs on EP #%x\n",
> -			alive, ep->ep_num);
> +		usb_kill_urb(u->urb);
> +	}
>  
>  	if (ep_state_update(ep, EP_STATE_STOPPING, EP_STATE_STOPPED)) {
>  		ep->sync_sink = NULL;