Message-ID: <zsk54zosv6tht4j4nma4ix7lq2knxi3ylqadd2foznc72nkaa3@xbc7gpozx6ai>
Date: Sat, 6 Jul 2024 00:15:01 -0300
From: Ágatha Isabelle Chris Moreira Guedes <code@...tha.dev>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	Luis Chamberlain <mcgrof@...nel.org>, linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org, 
	linux-modules@...r.kernel.org
Cc: Uwe Kleine-König <u.kleine-koenig@...libre.com>, 
	Dan Carpenter <dan.carpenter@...aro.org>, patch-reply@...tha.dev
Subject: [PATCH v3] init: staging: Fix missing warning/taint on builtin code

Fix the absence of warning message and kernel tainting when initializing
drivers from the `drivers/staging` subtree from initcalls (when
configured as built-in).

When such a driver is built as module and the module is loaded, the
`load_module()` function taints the kernel to signal code of unknown
quality is loaded, and produces a warning like this:

[    8.076352] rts5208: module is from the staging directory, the
quality is unknown, you have been warned.

The same behaviour is absent, however, when a staging driver is compiled
as built-in on the kernel image, since loading it happens through
initcalls and not through load_module().

This might prevent relevant information of being available on a bug
report (i.e. on a panic log) among other possible problems.

NOTES:
- The patch is written in such a way that all non-staging drivers are
  kept the way they were, except for staging drivers built with
  `-DSTAGING_CODE`.
- Since it changes some macros related to clang LTO as well, I tested it
  and it works properly in kernels compiled with both clang and gcc.
- Some `checkpatch.pl` errors, warnings and checks (with `--strict`) are
  present. Some were already there, some I introduced but I think
  they're unavoidable. Some IMHO don´t make sense at all, I think they
  would apply for most regular macros but initcall macros are just way
  different.

Fixes: 061b1bd394ca ("Staging: add TAINT_CRAP for all drivers/staging code")
Signed-off-by: Ágatha Isabelle Chris Moreira Guedes <code@...tha.dev>
---
CHANGELOG
v3:
- Added some missing suggestions from Uwe's suggestions after I detected
  the oversight (the Fixes: tag).
- I also noticed a possible build break, so I removed the
  `#ifdef CONFIG_STAGING` enclosuring the definition of
  `staging_taint()`.
- Improved again the string details after further explanation of Dan's
  original suggestion in v2.

v2:
- Changed the way we hook into the initcalls as suggested by Uwe, and
  moved the logic from `include/linux/module.h` to
  `include/linux/init.h`.
- Adjusted accordingly to work with both GCC and clang with
  `CONFIG_LTO_CLANG=y`, since some init scripts required that.
- Fixed the missing space pointed out by Dan & other minor string
  details.
- Changed the subject, since it became somewhat more of an init thing
  than a staging thing.

 drivers/staging/Makefile |  2 ++
 include/linux/init.h     | 76 ++++++++++++++++++++++++++++++++++------
 init/main.c              | 18 ++++++++++
 kernel/module/main.c     |  4 +--
 4 files changed, 86 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/Makefile b/drivers/staging/Makefile
index 5390879b5d1b..7cea13436426 100644
--- a/drivers/staging/Makefile
+++ b/drivers/staging/Makefile
@@ -1,6 +1,8 @@
 # SPDX-License-Identifier: GPL-2.0
 # Makefile for staging directory
 
+subdir-ccflags-y += -DSTAGING_CODE
+
 obj-y				+= media/
 obj-$(CONFIG_FB_OLPC_DCON)	+= olpc_dcon/
 obj-$(CONFIG_RTL8192E)		+= rtl8192e/
diff --git a/include/linux/init.h b/include/linux/init.h
index 58cef4c2e59a..c7afee978b42 100644
--- a/include/linux/init.h
+++ b/include/linux/init.h
@@ -188,6 +188,8 @@ extern struct module __this_module;
 
 #ifndef __ASSEMBLY__
 
+extern void staging_taint(const char *code_id, bool module);
+
 /*
  * initcalls are now grouped by functionality into separate
  * subsections. Ordering inside the subsections is determined
@@ -220,6 +222,14 @@ extern struct module __this_module;
 	__PASTE(__,						\
 	__PASTE(__iid, id))))
 
+#define __define_stub_fn(___stub, fn, __taintcall)		\
+	int __init ___stub(void);				\
+	int __init ___stub(void)				\
+	{							\
+		__taintcall();					\
+		return fn();					\
+	}							\
+
 #ifdef CONFIG_LTO_CLANG
 /*
  * With LTO, the compiler doesn't necessarily obey link order for
@@ -230,31 +240,75 @@ extern struct module __this_module;
 #define __initcall_section(__sec, __iid)			\
 	#__sec ".init.." #__iid
 
+#define ___define_initcall_stub(__stub, fn, __taintcall)	\
+	__define_stub_fn(__stub, fn, __taintcall)		\
+	__ADDRESSABLE(__stub)
+#else
+#define __initcall_section(__sec, __iid)			\
+	#__sec ".init"
+
+#define ___define_initcall_stub(__stub, fn, __taintcall)	\
+	__ADDRESSABLE(fn)
+#endif /* CONFIG_LTO_CLANG */
+
+#ifdef STAGING_CODE
+
 /*
  * With LTO, the compiler can rename static functions to avoid
  * global naming collisions. We use a global stub function for
  * initcalls to create a stable symbol name whose address can be
  * taken in inline assembly when PREL32 relocations are used.
+ *
+ * Moreover, when there's staging code, regardless of LTO, we
+ * need to wrap the function inside a new one to taint the
+ * kernel and warn about it in the log. So we need this special
+ * symbol name for the wrapper regardless of that.
  */
 #define __initcall_stub(fn, __iid, id)				\
 	__initcall_name(initstub, __iid, id)
 
+#define __staging_taint_fn()					\
+	staging_taint(KBUILD_MODNAME, false)
+
+#ifdef CONFIG_LTO_CLANG
+
 #define __define_initcall_stub(__stub, fn)			\
-	int __init __stub(void);				\
-	int __init __stub(void)					\
-	{ 							\
-		return fn();					\
-	}							\
-	__ADDRESSABLE(__stub)
-#else
-#define __initcall_section(__sec, __iid)			\
-	#__sec ".init"
+	___define_initcall_stub(__stub, fn, __staging_taint_fn)
+
+#else /* no CONFIG_LTO_CLANG */
+
+#define __define_initcall_stub(_stub, fn)			\
+	__define_stub_fn(_stub, fn, __staging_taint_fn)		\
+	__ADDRESSABLE(_stub)
+
+#endif /* CONFIG_LTO_CLANG */
+
+#else /* no STAGING_CODE */
+
+#ifdef CONFIG_LTO_CLANG
+
+/*
+ * Same case as before, it's not staging code but there's LTO
+ */
+#define __initcall_stub(fn, __iid, id)				\
+	__initcall_name(initstub, __iid, id)
 
+#else /* no CONFIG_LTO_CLANG*/
+
+/* For no LTO outside staging code, the vast majority of drivers
+ * can just be built with their regular symbol names as they
+ * just have been built all the time
+ */
 #define __initcall_stub(fn, __iid, id)	fn
 
+#endif /* CONFIG_LTO_CLANG */
+
+#define __staging_taint_fn()
+
 #define __define_initcall_stub(__stub, fn)			\
-	__ADDRESSABLE(fn)
-#endif
+	___define_initcall_stub(__stub, fn, __staging_taint_fn)
+
+#endif /* STAGING_CODE */
 
 #ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
 #define ____define_initcall(fn, __stub, __name, __sec)		\
diff --git a/init/main.c b/init/main.c
index 206acdde51f5..e35eeec1fbe4 100644
--- a/init/main.c
+++ b/init/main.c
@@ -1602,3 +1602,21 @@ static noinline void __init kernel_init_freeable(void)
 
 	integrity_load_keys();
 }
+
+/**
+ * staging_init_taint() - We need to taint the kernel whenever staging code
+ * is initialized (from built-in drivers) or loaded (as modules) and issue
+ * a warning the first time it happens.
+ */
+void staging_taint(const char *code_id, bool module)
+{
+	char *code_type = module ? "module" : "builtin driver";
+
+	pr_warn("%s %s: The kernel contains code from staging directory, "
+		"the quality is unknown, you have been warned.\n",
+		code_type, code_id);
+
+	add_taint(TAINT_CRAP, LOCKDEP_STILL_OK);
+}
+EXPORT_SYMBOL(staging_taint);
+
diff --git a/kernel/module/main.c b/kernel/module/main.c
index d18a94b973e1..d7d33336ab43 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2039,9 +2039,7 @@ static void module_augment_kernel_taints(struct module *mod, struct load_info *i
 	check_modinfo_retpoline(mod, info);
 
 	if (get_modinfo(info, "staging")) {
-		add_taint_module(mod, TAINT_CRAP, LOCKDEP_STILL_OK);
-		pr_warn("%s: module is from the staging directory, the quality "
-			"is unknown, you have been warned.\n", mod->name);
+		staging_taint(mod->name, true);
 	}
 
 	if (is_livepatch_module(mod)) {
-- 
2.45.2


Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists