On Sat, 6 Feb 2021 Takashi Iwai wrote:
> > 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.
>
> 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));
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);
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);
}
/**
