| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87mscat1tq.wl-tiwai@suse.de>
Date: Mon, 21 Apr 2025 09:23:45 +0200
From: Takashi Iwai <tiwai@...e.de>
To: Hillf Danton <hdanton@...a.com>
Cc: alsa-devel@...a-project.org,
Mikhail Gavrilov <mikhail.v.gavrilov@...il.com>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/9] ALSA: usb-audio: Fix possible race at sync of urb completions
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@...e.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);
Powered by blists - more mailing lists