From: Takashi Iwai <tiwai@suse.de>
To: Xu Yang <xu.yang_2@nxp.com>
Cc: perex@perex.cz, tiwai@suse.com, linux-sound@vger.kernel.org,
imx@lists.linux.dev
Subject: Re: [PATCH] ALSA: usb-audio: fix potential use after free issue when remove module snd-usb-audioy
Date: Wed, 22 May 2024 08:21:24 +0200 [thread overview]
Message-ID: <87h6eqz7rv.wl-tiwai@suse.de> (raw)
In-Reply-To: <87jzjmz8vv.wl-tiwai@suse.de>
On Wed, 22 May 2024 07:57:24 +0200,
Takashi Iwai wrote:
>
> On Wed, 22 May 2024 05:34:25 +0200,
> Xu Yang wrote:
> >
> > On Tue, May 21, 2024 at 01:32:37PM +0200, Takashi Iwai wrote:
> > > On Tue, 21 May 2024 12:56:05 +0200,
> > > Xu Yang wrote:
> > > >
> > > > On Mon, May 20, 2024 at 12:29:15PM +0200, Takashi Iwai wrote:
> > > > > On Mon, 20 May 2024 19:03:49 +0200,
> > > > > Xu Yang wrote:
> > > > > >
> > > > > > When remove module snd-usb-audio, snd_card_free_when_closed() will not
> > > > > > release the card resource if the card_dev refcount > 0 and
> > > >
> > > > [...]
> > > >
> > > > > > Then, even the userspace trying to cleanup the resources, kernel will not
> > > > > > touch the released code memory.
> > > > >
> > > > > Hm, it's an interesting report. Could you verify whether it's really
> > > > > hitting a module unload race? The module refcount should have been
> > > > > non-zero when the device is still in use, and it should have prevented
> > > > > the module unloading.
> > > >
> > > > Yes, the race does exist. I enable trace and got below output:
> > > > It seems that snd_usb_audio module refcnt is 0 after insmod completed. So
> > > > it can continue to be removed even it's still in use.
> > >
> > > If no device is opened, it's not really "used", and the driver module
> > > can be unloaded at any time. That's the intended behavior.
> >
> > Hh, I see wireplumber did open the card_dev when it scan card devices.
> > But wireplumber didn't close the card_dev when the scan process completed.
>
> Then rmmod shouldn't have been allowed, it's a consequence of the NULL
> module pointer, I suppose.
>
> > > (snip)
> > > > Then I take some time to check why snd_usb_audio module refcnt is 0
> > > > even though the card_dev is in use. Finally I got below finding:
> > > >
> > > > I build kernel and module with below configuration:
> > > >
> > > > CONFIG_SOUND=y
> > > > CONFIG_SND=y
> > > > CONFIG_SND_USB=y
> > > > CONFIG_SND_USB_AUDIO=m
> > > >
> > > > Then GCC will add -DMODULE when build snd-usb-audio as module, but will
> > > > not add -DMODULE when build sound/core/*.c.
> > > >
> > > > When insmod snd-usb-audio.ko, it will create a snd card device and call:
> > > >
> > > > snd_card_init() // sound/core/init.c
> > > >
> > > > #ifdef MODULE
> > > > WARN_ON(!module);
> > > > card->module = module;
> > > > #endif
> > > >
> > > > However, MODULE is not defined for sound/core/init.c, then card->module
> > > > will keep NULL pointer. With this results, snd-usb-audio module refcnt
> > > > will not be a non-zero value.
> > >
> > > Ah, it's a good finding! That explains.
> > >
> > > > > Practically seen, replacing snd_card_free_when_closed() with
> > > > > snd_card_free() shouldn't be a big problem, and it'll work in most
> > > > > cases. But there are always some corner cases that might lead to
> > > > > unexpected behavior. So, let's try to analyze more exactly what's
> > > > > happening there at first.
> > > >
> > > > With above finding, we needn't to replace snd_card_free_when_closed()
> > > > with snd_card_free(). We need to find a way to correctly handle module
> > > > refcnt since this should be a normal usecase.
> > >
> > > Right, I guess a simple fix below to replace '#ifdef MODULE' with
> > > '#ifdef CONFIG_MODULES' should work instead?
> >
> > Yeah, it works for me.
> > Will you send a fix for the issue or suggest me send it? ^_^
>
> I'm going to submit a proper fix patch later.
Looking at the git log again, I noticed that it's rather a regression
in commit 81033c6b584b. So at best we should address only this
regression by the patch below. Then the rest '#ifdef MODULE' can be
replaced with '#ifdef CONFIG_MODULES' in another patch as a different
fix / cleanup. The latter changes the behavior slightly (now the proc
entry appears even if you build the sound core into kernel).
Takashi
-- 8< --
Subject: [PATCH] ALSA: core: Fix NULL module pointer assignment at card init
The commit 81033c6b584b ("ALSA: core: Warn on empty module")
introduced a WARN_ON() for a NULL module pointer passed at snd_card
object creation, and it also wraps the code around it with '#ifdef
MODULE'. This works in most cases, but the devils are always in
details. "MODULE" is defined when the target code (i.e. the sound
core) is built as a module; but this doesn't mean that the caller is
also built-in or not. Namely, when only the sound core is built-in
(CONFIG_SND=y) while the driver is a module
(e.g. CONFIG_SND_USB_AUDIO=m), the passed module pointer is ignored
even if it's non-NULL and NULL is kept in card->module as consequence.
This resulted in the missing module reference up/down at the device
open/close, leading to a race with the code execution after the module
removal.
For addressing the bug, move the assignment of card->module again out
of ifdef. The WARN_ON() is still wrapped with ifdef, because the
module can be really NULL when all sound drivers are built-in.
Note that a NULL module pointer won't caught by ifdef MODULE, but it's
only for special kconfigs and this warning is only for debugging,
hence it shouldn't be any serious problem.
Fixes: 81033c6b584b ("ALSA: core: Warn on empty module")
Reported-by: Xu Yang <xu.yang_2@nxp.com>
Closes: https://lore.kernel.org/r/20240520170349.2417900-1-xu.yang_2@nxp.com
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
sound/core/init.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/core/init.c b/sound/core/init.c
index 6b127864a1a3..ac072614d1ea 100644
--- a/sound/core/init.c
+++ b/sound/core/init.c
@@ -313,8 +313,8 @@ static int snd_card_init(struct snd_card *card, struct device *parent,
card->number = idx;
#ifdef MODULE
WARN_ON(!module);
- card->module = module;
#endif
+ card->module = module;
INIT_LIST_HEAD(&card->devices);
init_rwsem(&card->controls_rwsem);
rwlock_init(&card->ctl_files_rwlock);
--
2.43.0
next prev parent reply other threads:[~2024-05-22 6:21 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-20 17:03 [PATCH] ALSA: usb-audio: fix potential use after free issue when remove module snd-usb-audio Xu Yang
2024-05-20 10:29 ` Takashi Iwai
2024-05-21 10:56 ` Xu Yang
2024-05-21 11:32 ` Takashi Iwai
2024-05-22 3:34 ` [PATCH] ALSA: usb-audio: fix potential use after free issue when remove module snd-usb-audioy Xu Yang
2024-05-22 5:57 ` Takashi Iwai
2024-05-22 6:21 ` Takashi Iwai [this message]
2024-05-22 6:36 ` Xu Yang
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=87h6eqz7rv.wl-tiwai@suse.de \
--to=tiwai@suse.de \
--cc=imx@lists.linux.dev \
--cc=linux-sound@vger.kernel.org \
--cc=perex@perex.cz \
--cc=tiwai@suse.com \
--cc=xu.yang_2@nxp.com \
/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).