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

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

[Takashi Iwai]

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.


Takashi

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

[Takashi Iwai]

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...?



Takashi

> 
> [1] https://lore.kernel.org/lkml/CABXGCsOposU1A_HavA_jmtkJMKhDZgh5m1b_YJK1Es5wyE-hZw@mail.gmail.com/

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

[Takashi Iwai]

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?

And more importantly, does it actually help at all?  If yes, I'd
happily take the patch, of course.


thanks,

Takashi

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

[Takashi Iwai]

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);

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

[Takashi Iwai]

On Mon, 21 Apr 2025 12:43:42 +0200,
Hillf Danton wrote:
> 
> On Mon, 21 Apr 2025 09:23:45 +0200 Takashi Iwai wrote:
> >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.
> >
> 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.

> 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.


Takashi

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

[Takashi Iwai]

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;

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

[Takashi Iwai]

On Tue, 22 Apr 2025 12:29:31 +0200,
Hillf Danton wrote:
> 
> On Tue, 22 Apr 2025 09:03:20 +0200 Takashi Iwai wrote:
> > 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.
> > 
> I'd like to test the change proposed locally, so please tippoint me to
> the test programs that could trigger the break.

Well, you need to follow the code logic.  The function isn't called
only from release / disconnect, but also from the code pattern:
 trigger(STOP) -> prepare -> sync_stop
and the URBs aren't released / re-initialized in this case.

For the problem you raised, I suppose it's better to stick with your
first approach with the manual barriers.  But you'd need to describe
exactly what it does and why it needed as a proper patch.


thanks,

Takashi