[PATCH 1/2] ALSA: core: Fix NULL module pointer assignment at card init

[PATCH 1/2] ALSA: core: Fix NULL module pointer assignment at card init

[Takashi Iwai]

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 (CONFIG_SND_USB_AUDIO=m),
the passed module pointer is ignored even if it's non-NULL, and
card->module remains as NULL.  This would result 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 we keep 'ifdef MODULE' for WARN_ON(), otherwise it would
lead to a false-positive NULL module check.  Admittedly it won't catch
perfectly, i.e. no check is performed when CONFIG_SND=y.  But, it's no
real problem as it's only for debugging, and the condition is pretty
rare.

Fixes: 81033c6b584b ("ALSA: core: Warn on empty module")
Reported-by: Xu Yang <xu.yang_2@nxp.com>
Closes: https://lore.kernel.org/r/20240520170349.2417900-1-xu.yang_2@nxp.com
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 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

[PATCH 2/2] ALSA: core: Enable proc module when CONFIG_MODULES=y

[Takashi Iwai]

We used '#ifdef MODULE' for judging whether the system supports the
sound module or not, and /proc/asound/modules is created only when
'#ifdef MODULE' is true.  The check is not really appropriate, though,
because the flag means only for the sound core and the drivers are
still allowed to be built as modules even if 'MODULE' is not set in
sound/core/init.c.

For fixing the inconsistency, replace those ifdefs with 'ifdef
CONFIG_MODULES'.  One place for a NULL module check is rewritten with
IS_MODULE(CONFIG_SND) to be more intuitive.  It can't be changed to
CONFIG_MODULES; otherwise it would hit a WARN_ON() incorrectly.

This is a slight behavior change; the modules proc entry appears now
no matter whether the sound core is built-in or not as long as modules
are enabled on the kernel in general.  This can't be avoided due to
the nature of kernel builds.

Link: https://lore.kernel.org/r/20240520170349.2417900-1-xu.yang_2@nxp.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/init.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/sound/core/init.c b/sound/core/init.c
index ac072614d1ea..4e52bbe32786 100644
--- 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,9 +311,7 @@ static int snd_card_init(struct snd_card *card, struct device *parent,
 	}
 	card->dev = parent;
 	card->number = idx;
-#ifdef MODULE
-	WARN_ON(!module);
-#endif
+	WARN_ON(IS_MODULE(CONFIG_SND) && !module);
 	card->module = module;
 	INIT_LIST_HEAD(&card->devices);
 	init_rwsem(&card->controls_rwsem);
@@ -969,7 +967,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 +995,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;
-- 
2.43.0

Re: [PATCH 1/2] ALSA: core: Fix NULL module pointer assignment at card init

[Xu Yang]

On Wed, May 22, 2024 at 09:04:39AM +0200, Takashi Iwai wrote:
> 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 (CONFIG_SND_USB_AUDIO=m),
> the passed module pointer is ignored even if it's non-NULL, and
> card->module remains as NULL.  This would result 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 we keep 'ifdef MODULE' for WARN_ON(), otherwise it would
> lead to a false-positive NULL module check.  Admittedly it won't catch
> perfectly, i.e. no check is performed when CONFIG_SND=y.  But, it's no
> real problem as it's only for debugging, and the condition is pretty
> rare.
> 
> Fixes: 81033c6b584b ("ALSA: core: Warn on empty module")
> Reported-by: Xu Yang <xu.yang_2@nxp.com>
> Closes: https://lore.kernel.org/r/20240520170349.2417900-1-xu.yang_2@nxp.com
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

For this patchset:
Tested-by: Xu Yang <xu.yang_2@nxp.com>

Thanks,
Xu Yang

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