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


thanks,

Takashi