[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.
Takashi
---
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);
--
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
