Re: BUG: KASAN: use-after-free in snd_complete_urb+0x109e/0x1740 [snd_usb_audio] (5.11-rc6)
From: Takashi Iwai <hidden>
Date: 2021-02-06 08:33:57
Also in:
alsa-devel, lkml
On Sat, 06 Feb 2021 09:13:33 +0100, Hillf Danton wrote:
quoted hunk
On Sat, 6 Feb 2021 Takashi Iwai wrote:quoted
quoted
Due to the reconnecting key word mentioned, no fix to d0f09d1e4a88 ("ALSA: usb-audio: Refactoring endpoint URB deactivation") will be added. What is added is to capture EP_FLAG_STOPPING and remove the one second wait limit if the reconnecting acts may make it easier to repro the uaf. The diff is only for idea show.If my understanding is right, this won't change. The problem is rather the lack of this function call itself, i.e. the missing synchronization for the stream stop.Thanks for taking a look at it.quoted
It worked casually in the past because the endpoint resource is released at a later point that is after all streams are really closed. Now it's released earlier and hitting the UAF.And add it if I dont misread you. Hillf--- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c@@ -832,24 +832,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 (!test_bit(EP_FLAG_STOPPING, &ep->flags)) - return 0; - + WARN_ON_ONCE(!test_bit(EP_FLAG_STOPPING, &ep->flags));
EP_FLAG_STOPPING is normally zero, hence putting a WARN_ON() shows just a false-positive.
do {
- alive = bitmap_weight(&ep->active_mask, ep->nurbs);
- if (!alive)
+ if (!bitmap_weight(&ep->active_mask, ep->nurbs))
break;
schedule_timeout_uninterruptible(1);
- } while (time_before(jiffies, end_time));
+ } while (1);
- if (alive)
- usb_audio_err(ep->chip,
- "timeout: still %d active urbs on EP #%x\n",
- alive, ep->ep_num);The original report didn't show the error, so it's not about the timeout. You're likely scratching a wrong surface, I'm afraid.
quoted hunk
clear_bit(EP_FLAG_STOPPING, &ep->flags); ep->sync_sink = NULL;@@ -1433,7 +1423,7 @@ void snd_usb_endpoint_stop(struct snd_us WRITE_ONCE(ep->sync_source->sync_sink, NULL); if (!atomic_dec_return(&ep->running)) - stop_and_unlink_urbs(ep, false, false); + stop_and_unlink_urbs(ep, false, true);
Please don't. This will lead to a sleep-in-atomic Oops. Takashi