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

[Xu Yang <xu.yang_2@xxxxxxx>]

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.

> 
> (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? ^_^

Thanks,
Xu Yang