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

[Takashi Iwai <tiwai@xxxxxxx>]

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.

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


thanks,

Takashi

-- 8< --
--- a/sound/core/init.c
+++ b/sound/core/init.c
@@ -50,7 +50,7 @@ MODULE_PARM_DESC(slots, "Module names assigned to the slots.");
 static int module_slot_match(struct module *module, int idx)
 {
 	int match = 1;
-#ifdef MODULE
+#ifdef CONFIG_MODULES
 	const char *s1, *s2;
 
 	if (!module || !*module->name || !slots[idx])
@@ -77,7 +77,7 @@ static int module_slot_match(struct module *module, int idx)
 		if (!c1)
 			break;
 	}
-#endif /* MODULE */
+#endif /* CONFIG_MODULES */
 	return match;
 }
 
@@ -311,7 +311,7 @@ static int snd_card_init(struct snd_card *card, struct device *parent,
 	}
 	card->dev = parent;
 	card->number = idx;
-#ifdef MODULE
+#ifdef CONFIG_MODULES
 	WARN_ON(!module);
 	card->module = module;
 #endif
@@ -969,7 +969,7 @@ void snd_card_info_read_oss(struct snd_info_buffer *buffer)
 
 #endif
 
-#ifdef MODULE
+#ifdef CONFIG_MODULES
 static void snd_card_module_info_read(struct snd_info_entry *entry,
 				      struct snd_info_buffer *buffer)
 {
@@ -997,7 +997,7 @@ int __init snd_card_info_init(void)
 	if (snd_info_register(entry) < 0)
 		return -ENOMEM; /* freed in error path */
 
-#ifdef MODULE
+#ifdef CONFIG_MODULES
 	entry = snd_info_create_module_entry(THIS_MODULE, "modules", NULL);
 	if (!entry)
 		return -ENOMEM;