Re: [PATCH] usb: gadget: u_audio: Fix race condition use of controls after free during gadget unbind.

[Greg KH <gregkh@linuxfoundation.org>]

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.

> 
> Signed-off-by: Chris Wulff <chris.wulff@biamp.com>

What commit id does this fix?

> ---
>  drivers/usb/gadget/function/u_audio.c | 31 ++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 12 deletions(-)
> 
> 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;

No tabs?  Odd.  Not your fault, just a meta-comment.

>  
> -	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;

How can this happen?  Is this a separate fix?  Or the same issue?

thanks,

greg k-h