Re: [PATCH] ALSA: usb-audio: fix potential use after free issue when remove module snd-usb-audioy

[Takashi Iwai <tiwai@xxxxxxx>]

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@xxxxxxx>
Closes: https://lore.kernel.org/r/20240520170349.2417900-1-xu.yang_2@xxxxxxx
Cc: <stable@xxxxxxxxxxxxxxx>
Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
---
 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