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

[Xu Yang <xu.yang_2@xxxxxxx>]

On Wed, May 22, 2024 at 08:21:24AM +0200, Takashi Iwai wrote:
> 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.

Yes.

> > 
> > > > (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.

Thanks in advance.

> 
> 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).

Yes, indeed true. I agree with you.

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

It's okay for me. I'll give a Tested-by when receive a formal patch.

Thanks,
Xu Yang

> -- 
> 2.43.0
>