diff mbox series

usb: gadget: u_audio: Fix race condition use of controls after free during gadget unbind.

Message ID CO1PR17MB54190B898057616EEB3F9E51E10E2@CO1PR17MB5419.namprd17.prod.outlook.com
State New
Headers show
Series usb: gadget: u_audio: Fix race condition use of controls after free during gadget unbind. | expand

Commit Message

Chris Wulff April 18, 2024, 4:35 p.m. UTC
Hang on to the control IDs instead of pointers since those are correctly handled with locks.
Prevent use of the uac data structure after it has been freed.
Mark the endpoint as disabled sooner so that freed requests aren't used.

Signed-off-by: Chris Wulff <chris.wulff@biamp.com>
---
 drivers/usb/gadget/function/u_audio.c | 31 ++++++++++++++++-----------
 1 file changed, 19 insertions(+), 12 deletions(-)

Comments

Chris Wulff April 24, 2024, 5:57 p.m. UTC | #1
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Tuesday, April 23, 2024 7:18 PM

> On Thu, Apr 18, 2024 at 04:35:07PM +0000, Chris Wulff wrote:
> > Hang on to the control IDs instead of pointers since those are correctly handled with locks.
> > Prevent use of the uac data structure after it has been freed.
> > Mark the endpoint as disabled sooner so that freed requests aren't used.
> 
> Nit, please wrap your changelog text at 72 columns.  running
> scripts/checkpatch.pl should show this.

Next version will be wrapped correctly.

> >
> > Signed-off-by: Chris Wulff <chris.wulff@biamp.com>
>
> What commit id does this fix?

Several (next version will have Fixes: and see comments below about separating fixes)

Modification to stored controls:
8fe9a03f4331 ("usb: gadget: u_audio: Rate ctl notifies about current srate (0=stopped)")
c565ad07ef35 ("usb: gadget: u_audio: Support multiple sampling rates")
02de698ca812 ("usb: gadget: u_audio: add bi-directional volume and mute support")

ep_enabled:
068fdad20454f81 ("usb: gadget: u_audio: fix race condition on endpoint stop")


> > @@ -447,6 +447,8 @@ static inline void free_ep(struct uac_rtd_params *prm, struct usb_ep *ep)
> >        if (!prm->ep_enabled)
> >                return;
> > 
> > +     prm->ep_enabled = false;
> > +
> >        audio_dev = uac->audio_dev;
> >        params = &audio_dev->params;
> > 
> > @@ -464,8 +466,6 @@ static inline void free_ep(struct uac_rtd_params *prm, struct usb_ep *ep)
> >                }
> >        }
> > 
> > -     prm->ep_enabled = false;
> > -
> >        if (usb_ep_disable(ep))
> >                dev_err(uac->card->dev, "%s:%d Error!\n", __func__, __LINE__);
> >  }

Looks like this actually is reverting part of 068fdad20454f81, which was put in to fix a different 
double-free problem but introduces a new race condition that I am running into. In my case,
u_audio_iso_complete is called during unbind and _doesn't_ exit early and goes forward to access
various things in the sound subsystem. I will split this off and see if I can better isolate what
is really going wrong. 

> > @@ -792,6 +791,9 @@ int u_audio_set_volume(struct g_audio *audio_dev, int playback, s16 val)
> >        unsigned long flags;
> >        int change = 0;
> > 
> > +     if (!uac)
> > +             return 0;
> > +
> >        if (playback)
> >                prm = &uac->p_prm;
> >        else
> > @@ -840,6 +842,9 @@ int u_audio_set_mute(struct g_audio *audio_dev, int playback, int val)
> >        int change = 0;
> >        int mute;
> > 
> > +     if (!uac)
> > +             return 0;
> 
> How can this happen?  Is this a separate fix?  Or the same issue?

This happens if we receive a URB callback after/during g_audio_cleanup (via out_rq_cur_complete 
in f_uac1/2.c) for a UAC mute or volume control. If that happens, these can access freed memory.

I think the higher-level race is that the dequeue for the request happens in composite_dev_cleanup
after the function unbind (which in f_uac1/2 calls g_audio_cleanup.)

I will make this a separate patch if you want as it is fixing a similar symptom as the others, but 
has it's own discrete cause. Presumably this can also happen for get of volume or mute controls
though I didn't run into that.

Here's a little timeline to better illustrate the race:

  Unbind                       URB

  composite_unbind
    __composite_unbind
      remove_config
        usb_remove_function
          f_audio_unbind
            g_audio_cleanup                        <-- uac is freed

                                                   <-- URB received from host
                               out_rq_cur_complete <-- set ctrl from host
                                 u_audio_set_mute  <-- uses freed uac

        usb_remove_function...                     <-- other function in config
                                                       may increase the window
      composite_dev_cleanup
        usb_ep_dequeue                             <-- EP0 req removed

It is possible there is a better way to avoid this by making sure to dequeue the req prior to
calling g_audio_cleanup in f_uac1/2.c. I will investigate that a bit and see what I can come up
with.

-- Chris Wulff
diff mbox series

Patch

diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c
index 4a42574b4a7f..bcae95472455 100644
--- a/drivers/usb/gadget/function/u_audio.c
+++ b/drivers/usb/gadget/function/u_audio.c
@@ -57,13 +57,13 @@  struct uac_rtd_params {
 
   /* Volume/Mute controls and their state */
   int fu_id; /* Feature Unit ID */
-  struct snd_kcontrol *snd_kctl_volume;
-  struct snd_kcontrol *snd_kctl_mute;
+  struct snd_ctl_elem_id snd_kctl_volume_id;
+  struct snd_ctl_elem_id snd_kctl_mute_id;
   s16 volume_min, volume_max, volume_res;
   s16 volume;
   int mute;
 
-	struct snd_kcontrol *snd_kctl_rate; /* read-only current rate */
+	struct snd_ctl_elem_id snd_kctl_rate_id; /* read-only current rate */
 	int srate; /* selected samplerate */
 	int active; /* playback/capture running */
 
@@ -447,6 +447,8 @@  static inline void free_ep(struct uac_rtd_params *prm, struct usb_ep *ep)
 	if (!prm->ep_enabled)
 		return;
 
+	prm->ep_enabled = false;
+
 	audio_dev = uac->audio_dev;
 	params = &audio_dev->params;
 
@@ -464,8 +466,6 @@  static inline void free_ep(struct uac_rtd_params *prm, struct usb_ep *ep)
 		}
 	}
 
-	prm->ep_enabled = false;
-
 	if (usb_ep_disable(ep))
 		dev_err(uac->card->dev, "%s:%d Error!\n", __func__, __LINE__);
 }
@@ -494,14 +494,13 @@  static inline void free_ep_fback(struct uac_rtd_params *prm, struct usb_ep *ep)
 static void set_active(struct uac_rtd_params *prm, bool active)
 {
 	// notifying through the Rate ctrl
-	struct snd_kcontrol *kctl = prm->snd_kctl_rate;
 	unsigned long flags;
 
 	spin_lock_irqsave(&prm->lock, flags);
 	if (prm->active != active) {
 		prm->active = active;
 		snd_ctl_notify(prm->uac->card, SNDRV_CTL_EVENT_MASK_VALUE,
-				&kctl->id);
+				&prm->snd_kctl_rate_id);
 	}
 	spin_unlock_irqrestore(&prm->lock, flags);
 }
@@ -792,6 +791,9 @@  int u_audio_set_volume(struct g_audio *audio_dev, int playback, s16 val)
 	unsigned long flags;
 	int change = 0;
 
+	if (!uac)
+		return 0;
+
 	if (playback)
 		prm = &uac->p_prm;
 	else
@@ -807,7 +809,7 @@  int u_audio_set_volume(struct g_audio *audio_dev, int playback, s16 val)
 
 	if (change)
 		snd_ctl_notify(uac->card, SNDRV_CTL_EVENT_MASK_VALUE,
-				&prm->snd_kctl_volume->id);
+				&prm->snd_kctl_volume_id);
 
 	return 0;
 }
@@ -840,6 +842,9 @@  int u_audio_set_mute(struct g_audio *audio_dev, int playback, int val)
 	int change = 0;
 	int mute;
 
+	if (!uac)
+		return 0;
+
 	if (playback)
 		prm = &uac->p_prm;
 	else
@@ -856,7 +861,7 @@  int u_audio_set_mute(struct g_audio *audio_dev, int playback, int val)
 
 	if (change)
 		snd_ctl_notify(uac->card, SNDRV_CTL_EVENT_MASK_VALUE,
-			       &prm->snd_kctl_mute->id);
+			       &prm->snd_kctl_mute_id);
 
 	return 0;
 }
@@ -1331,7 +1336,7 @@  int g_audio_setup(struct g_audio *g_audio, const char *pcm_name,
 			err = snd_ctl_add(card, kctl);
 			if (err < 0)
 				goto snd_fail;
-			prm->snd_kctl_mute = kctl;
+			prm->snd_kctl_mute_id = kctl->id;
 			prm->mute = 0;
 		}
 
@@ -1359,7 +1364,7 @@  int g_audio_setup(struct g_audio *g_audio, const char *pcm_name,
 			err = snd_ctl_add(card, kctl);
 			if (err < 0)
 				goto snd_fail;
-			prm->snd_kctl_volume = kctl;
+			prm->snd_kctl_volume_id = kctl->id;
 			prm->volume = fu->volume_max;
 			prm->volume_max = fu->volume_max;
 			prm->volume_min = fu->volume_min;
@@ -1383,7 +1388,7 @@  int g_audio_setup(struct g_audio *g_audio, const char *pcm_name,
 		err = snd_ctl_add(card, kctl);
 		if (err < 0)
 			goto snd_fail;
-		prm->snd_kctl_rate = kctl;
+		prm->snd_kctl_rate_id = kctl->id;
 	}
 
 	strscpy(card->driver, card_name, sizeof(card->driver));
@@ -1420,6 +1425,8 @@  void g_audio_cleanup(struct g_audio *g_audio)
 		return;
 
 	uac = g_audio->uac;
+	g_audio->uac = NULL;
+
 	card = uac->card;
 	if (card)
 		snd_card_free_when_closed(card);