[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