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

[Takashi Iwai <tiwai@suse.de>]

On Mon, 21 Apr 2025 07:18:31 +0200,
Hillf Danton wrote:
> 
> On Sun, 20 Apr 2025 09:49:46 +0200 Takashi Iwai wrote:
> >On Sat, 19 Apr 2025 10:04:08 +0200, Hillf Danton wrote:
> >> On Sat, 19 Apr 2025 08:50:46 +0200 Takashi Iwai wrote:
> >> >On Fri, 18 Apr 2025 16:45:17 +0200, Hillf Danton wrote:
> >> >> On Fri, 18 Apr 2025 13:08:32 +0200 Takashi Iwai wrote:
> >> >> > On Fri, 18 Apr 2025 12:35:32 +0200 Hillf Danton wrote:
> >> >> > > On Wed, 29 Sep 2021 10:08:37 +0200 Takashi Iwai wrote:
> >> >> > > > USB-audio driver tries to sync with the clear of all pending URBs in
> >> >> > > > wait_clear_urbs(), and it waits for all bits in active_mask getting
> >> >> > > > cleared.  This works fine for the normal operations, but when a stream
> >> >> > > > is managed in the implicit feedback mode, there is still a very thin
> >> >> > > > race window: namely, in snd_complete_usb(), the active_mask bit for
> >> >> > > > the current URB is once cleared before re-submitted in
> >> >> > > > queue_pending_output_urbs().  If wait_clear_urbs() is called during
> >> >> > > > that period, it may pass the test and go forward even though there may
> >> >> > > > be a still pending URB.
> >> >> > > > 
> >> >> > > > For covering it, this patch adds a new counter to each endpoint to
> >> >> > > > keep the number of in-flight URBs, and changes wait_clear_urbs()
> >> >> > > > checking this number instead.  The counter is decremented at the end
> >> >> > > > of URB complete, hence the reference is kept as long as the URB
> >> >> > > > complete is in process.
> >> >> > > > 
> >> >> > > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> >> >> > > > ---
> >> >> > > >  sound/usb/card.h     | 1 +
> >> >> > > >  sound/usb/endpoint.c | 7 ++++++-
> >> >> > > >  2 files changed, 7 insertions(+), 1 deletion(-)
> >> >> > > > 
> >> >> > > > diff --git a/sound/usb/card.h b/sound/usb/card.h
> >> >> > > > index 3329ce710cb9..746a765b2437 100644
> >> >> > > > --- a/sound/usb/card.h
> >> >> > > > +++ b/sound/usb/card.h
> >> >> > > > @@ -97,6 +97,7 @@ struct snd_usb_endpoint {
> >> >> > > >  	unsigned int nominal_queue_size; /* total buffer sizes in URBs */
> >> >> > > >  	unsigned long active_mask;	/* bitmask of active urbs */
> >> >> > > >  	unsigned long unlink_mask;	/* bitmask of unlinked urbs */
> >> >> > > > +	atomic_t submitted_urbs;	/* currently submitted urbs */
> >> >> > > >  	char *syncbuf;			/* sync buffer for all sync URBs */
> >> >> > > >  	dma_addr_t sync_dma;		/* DMA address of syncbuf */
> >> >> > > >  
> >> >> > > > diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
> >> >> > > > index 29c4865966f5..06241568abf7 100644
> >> >> > > > --- a/sound/usb/endpoint.c
> >> >> > > > +++ b/sound/usb/endpoint.c
> >> >> > > > @@ -451,6 +451,7 @@ static void queue_pending_output_urbs(struct snd_usb_endpoint *ep)
> >> >> > > >  		}
> >> >> > > >  
> >> >> > > >  		set_bit(ctx->index, &ep->active_mask);
> >> >> > > > +		atomic_inc(&ep->submitted_urbs);
> >> >> > > >  	}
> >> >> > > >  }
> >> >> > > >  
> >> >> > > > @@ -488,6 +489,7 @@ static void snd_complete_urb(struct urb *urb)
> >> >> > > >  			clear_bit(ctx->index, &ep->active_mask);
> >> >> > > >  			spin_unlock_irqrestore(&ep->lock, flags);
> >> >> > > >  			queue_pending_output_urbs(ep);
> >> >> > > 
> >> >> > > 			smp_mb();
> >> >> > > 
> >> >> > > > +			atomic_dec(&ep->submitted_urbs); /* decrement at last */
> >> >> > > 
> >> >> > > Does it match the comment to add a mb?
> >> >> > 
> >> >> > How...?  I don't understand your intention.
> >> >> > 
> >> >> In addition to the UAF report [1], I saw a customer report of list
> >> >> corruption of linux-6.1.99 on arm64 this week without reproducer.
> >> >> 
> >> >> 	list corruption
> >> >> 	list_add_tail();
> >> >> 	push_back_to_ready_list();
> >> >> 	snd_complete_urb();
> >> >> 
> >> >> And after another look at this patch I wonder if the race can not be
> >> >> erased without the certainty that ep will be no longer used after the
> >> >> atomic decrement.
> >> >
> >> > But why adding more barrier if you perform the atomic op...?
> >> >
> >> Because atomic op != ordering, see 26fbe9772b8c ("USB: core: Fix hang 
> >> in usb_kill_urb by adding memory barriers") for detail for example.
> >> And c5b2cbdbdac5 ("ipc/mqueue.c: update/document memory barriers") as well.
> >
> > Still don't get it.  Which reads and writes are you trying to solve?
> >
> See a simpler UAF case IIUIC.
> 
> 	cpu1		cpu2
> 			atomic_dec(&ep->submitted_urbs);
> 
> 	if (!atomic_read(&ep->submitted_urbs))
> 		kfree(ep);
> 
> 			a = ep->xxx; // UAF

That's what I don't get it.  Which place does this UAF happen, more
specifically?  In the whole conversations, the context is missing, and
you provided only a snippet of the patch.

> > And more importantly, does it actually help at all?  If yes, I'd
> 
> Yes and No. Yes because of the fix mentioned in ipc wrt mb.
> 
> > happily take the patch, of course.
> >
> Provided a repro, a fix like the following diff should have been posted.

No idea about repro and the report itself, so hard to judge.


Takashi

> 
> 	Hillf
> 
> --- x/sound/usb/endpoint.c
> +++ y/sound/usb/endpoint.c
> @@ -569,8 +569,11 @@ static void snd_complete_urb(struct urb
>  			snd_usb_queue_pending_output_urbs(ep, false);
>  			/* decrement at last, and check xrun */
>  			if (atomic_dec_and_test(&ep->submitted_urbs) &&
> -			    !snd_usb_endpoint_implicit_feedback_sink(ep))
> +			    !snd_usb_endpoint_implicit_feedback_sink(ep)) {
>  				notify_xrun(ep);
> +				smp_mb();
> +				atomic_dec(&ep->submitted_urbs);
> +			}
>  			return;
>  		}
>  
> @@ -602,7 +605,8 @@ static void snd_complete_urb(struct urb
>  
>  exit_clear:
>  	clear_bit(ctx->index, &ep->active_mask);
> -	atomic_dec(&ep->submitted_urbs);
> +	if (atomic_dec_and_test(&ep->submitted_urbs))
> +		atomic_dec(&ep->submitted_urbs);
>  }
>  
>  /*
> @@ -1004,17 +1008,17 @@ static int wait_clear_urbs(struct snd_us
>  
>  	do {
>  		alive = atomic_read(&ep->submitted_urbs);
> -		if (!alive)
> -			break;
> +		if (alive < 0)
> +			goto update;
>  
>  		schedule_timeout_uninterruptible(1);
>  	} while (time_before(jiffies, end_time));
>  
> -	if (alive)
> -		usb_audio_err(ep->chip,
> +	usb_audio_err(ep->chip,
>  			"timeout: still %d active urbs on EP #%x\n",
>  			alive, ep->ep_num);
>  
> +update:
>  	if (ep_state_update(ep, EP_STATE_STOPPING, EP_STATE_STOPPED)) {
>  		ep->sync_sink = NULL;
>  		snd_usb_endpoint_set_callback(ep, NULL, NULL, NULL);