At Wed, 25 Jun 2014 10:59:29 -0400 (EDT),
Alan Stern wrote:
>
> On Wed, 25 Jun 2014, Takashi Iwai wrote:
>
> > [Adding Daniel to Cc]
> >
> > At Tue, 24 Jun 2014 13:52:14 -0400 (EDT),
> > Alan Stern wrote:
> > >
> > > Takashi and Clemens:
> > >
> > > The snd-usb-audio driver has a race between close and disconnect. A
> > > patch I have been testing reliably triggers this race on my machine and
> > > reveals a use-after-free bug.
> > >
> > > This happens when a device is disconnected while being used for audio
> > > I/O. Simply unplugging the device doesn't seem to trigger the problem,
> > > probably because it involves different timing.
> > >
> > > In detail: When the device is unplugged, the USB core calls
> > > usb_audio_disconnect() -> snd_usb_audio_disconnect().
> > >
> > > At the same time, the user program gets a poll failure and closes the
> > > device: snd_usb_playback_close() -> snd_usb_pcm_close() ->
> > > stop_endpoints() -> snd_usb_endpoint_sync_pending_stop() ->
> > > wait_clear_urbs(). This routine waits until the endpoint's
> > > outstanding URBs have completed, testing the snd_usb_endpoint structure
> > > in a loop.
> > >
> > > Meanwhile, back in the disconnect thread, snd_usb_audio_disconnect()
> > > calls snd_usb_endpoint_free() for each endpoint on the device. This
> > > routine unlinks the endpoint's URBs, calls wait_clear_urbs(), and then
> > > deallocates the snd_usb_endpoint structure.
> > >
> > > So there are two threads both running wait_clear_urbs() for the same
> > > endpoint, and one of them will deallocate the endpoint afterward. If
> > > that thread happens to finish first, the other thread will dereference
> > > the deallocated structure.
> > >
> > > Obviously there needs to be some sort of mutual exclusion between the
> > > I/O pathways and the disconnect pathway. I don't know what the right
> > > solution is. The patch below at least avoids this particular failure
> > > scenario, but probably not correctly.
> > >
> > > Can either of you write a proper fix?
> >
> > If I understand correctly, calling wait_clear_urbs() simultaneously
> > doesn't do anything harmful. Also calling wait_clear_urbs() and
> > release_urbs() at the same time would be OK, too. The problem is
> > rather that snd_usb_endpoint_free() actually frees the ep object that
> > is being referenced. Moving kfree() into the later point should solve
> > it.
> >
> > Below is an untested patch. Give it a try.
>
> Thanks for the patch. I confirm that it fixed the bug.
>
> Tested-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
>
> Don't forget to tag it for -stable when you merge it!
Thanks for a quick test! Below is the complete patch with the
changelog.
Takashi
-- 8< --
From: Takashi Iwai <tiwai@xxxxxxx>
Subject: [PATCH] ALSA: usb-audio: Fix races at disconnection and PCM closing
When a USB-audio device is disconnected while PCM is still running, we
still see some race: the disconnect callback calls
snd_usb_endpoint_free() that calls release_urbs() and then kfree()
while a PCM stream would be closed at the same time and calls
stop_endpoints() that leads to wait_clear_urbs(). That is, the EP
object might be deallocated while a PCM stream is syncing with
wait_clear_urbs() with the same EP.
Basically calling multiple wait_clear_urbs() would work fine, also
calling wait_clear_urbs() and release_urbs() would work, too, as
wait_clear_urbs() just reads some fields in ep. The problem is the
succeeding kfree() in snd_pcm_endpoint_free().
This patch moves out the EP deallocation into the later point, the
destructor callback. At this stage, all PCMs must have been already
closed, so it's safe to free the objects.
Reported-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
Tested-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
Cc: <stable@xxxxxxxxxxxxxxx>
Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
---
sound/usb/card.c | 13 ++++++++++---
sound/usb/endpoint.c | 17 ++++++++++++++---
sound/usb/endpoint.h | 1 +
3 files changed, 25 insertions(+), 6 deletions(-)
diff --git a/sound/usb/card.c b/sound/usb/card.c
index c3b5b7dca1c3..a09e5f3519e3 100644
--- a/sound/usb/card.c
+++ b/sound/usb/card.c
@@ -307,6 +307,11 @@ static int snd_usb_create_streams(struct snd_usb_audio *chip, int ctrlif)
static int snd_usb_audio_free(struct snd_usb_audio *chip)
{
+ struct list_head *p, *n;
+
+ list_for_each_safe(p, n, &chip->ep_list)
+ snd_usb_endpoint_free(p);
+
mutex_destroy(&chip->mutex);
kfree(chip);
return 0;
@@ -585,7 +590,7 @@ static void snd_usb_audio_disconnect(struct usb_device *dev,
struct snd_usb_audio *chip)
{
struct snd_card *card;
- struct list_head *p, *n;
+ struct list_head *p;
if (chip == (void *)-1L)
return;
@@ -598,14 +603,16 @@ static void snd_usb_audio_disconnect(struct usb_device *dev,
mutex_lock(®ister_mutex);
chip->num_interfaces--;
if (chip->num_interfaces <= 0) {
+ struct snd_usb_endpoint *ep;
+
snd_card_disconnect(card);
/* release the pcm resources */
list_for_each(p, &chip->pcm_list) {
snd_usb_stream_disconnect(p);
}
/* release the endpoint resources */
- list_for_each_safe(p, n, &chip->ep_list) {
- snd_usb_endpoint_free(p);
+ list_for_each_entry(ep, &chip->ep_list, list) {
+ snd_usb_endpoint_release(ep);
}
/* release the midi resources */
list_for_each(p, &chip->midi_list) {
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
index 289f582c9130..114e3e7ff511 100644
--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -987,19 +987,30 @@ void snd_usb_endpoint_deactivate(struct snd_usb_endpoint *ep)
}
/**
+ * snd_usb_endpoint_release: Tear down an snd_usb_endpoint
+ *
+ * @ep: the endpoint to release
+ *
+ * This function does not care for the endpoint's use count but will tear
+ * down all the streaming URBs immediately.
+ */
+void snd_usb_endpoint_release(struct snd_usb_endpoint *ep)
+{
+ release_urbs(ep, 1);
+}
+
+/**
* snd_usb_endpoint_free: Free the resources of an snd_usb_endpoint
*
* @ep: the list header of the endpoint to free
*
- * This function does not care for the endpoint's use count but will tear
- * down all the streaming URBs immediately and free all resources.
+ * This free all resources of the given ep.
*/
void snd_usb_endpoint_free(struct list_head *head)
{
struct snd_usb_endpoint *ep;
ep = list_entry(head, struct snd_usb_endpoint, list);
- release_urbs(ep, 1);
kfree(ep);
}
diff --git a/sound/usb/endpoint.h b/sound/usb/endpoint.h
index 1c7e8ee48abc..e61ee5c356a3 100644
--- a/sound/usb/endpoint.h
+++ b/sound/usb/endpoint.h
@@ -23,6 +23,7 @@ void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep);
void snd_usb_endpoint_sync_pending_stop(struct snd_usb_endpoint *ep);
int snd_usb_endpoint_activate(struct snd_usb_endpoint *ep);
void snd_usb_endpoint_deactivate(struct snd_usb_endpoint *ep);
+void snd_usb_endpoint_release(struct snd_usb_endpoint *ep);
void snd_usb_endpoint_free(struct list_head *head);
int snd_usb_endpoint_implicit_feedback_sink(struct snd_usb_endpoint *ep);
--
2.0.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
