From: Chris Wulff <Chris.Wulff@biamp.com>
To: Greg KH <gregkh@linuxfoundation.org>
Cc: "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>
Subject: Re: [PATCH] usb: gadget: u_audio: Fix race condition use of controls after free during gadget unbind.
Date: Wed, 24 Apr 2024 17:57:08 +0000 [thread overview]
Message-ID: <CO1PR17MB5419BB151E902045A7E98FF6E1102@CO1PR17MB5419.namprd17.prod.outlook.com> (raw)
In-Reply-To: <2024042346-pastime-platonic-7e28@gregkh>
> 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
prev parent reply other threads:[~2024-04-24 17:57 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-18 16:35 [PATCH] usb: gadget: u_audio: Fix race condition use of controls after free during gadget unbind Chris Wulff
2024-04-23 23:18 ` Greg KH
2024-04-24 17:57 ` Chris Wulff [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CO1PR17MB5419BB151E902045A7E98FF6E1102@CO1PR17MB5419.namprd17.prod.outlook.com \
--to=chris.wulff@biamp.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-usb@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).