[PATCH] kunit: Taint kernel if any tests run

[PATCH] kunit: Taint kernel if any tests run

[David Gow]

KUnit tests are not supposed to run on production systems: they may do
deliberately illegal things to trigger errors, and have security
implications (assertions will often deliberately leak kernel addresses).

Add a new taint type, TAINT_KUNIT to signal that a KUnit test has been
run. This will be printed as 'N' (for kuNit, as K, U and T were already
taken).

This should discourage people from running KUnit tests on production
systems, and to make it easier to tell if tests have been run
accidentally (by loading the wrong configuration, etc.)

Signed-off-by: David Gow <davidgow@google.com>
---

This is something I'd been thinking about for a while, and it came up
again, so I'm finally giving it a go.

Two notes:
- I decided to add a new type of taint, as none of the existing ones
  really seemed to fit. We could live with considering KUnit tests as
  TAINT_WARN or TAINT_CRAP or something otherwise, but neither are quite
  right.
- The taint_flags table gives a couple of checkpatch.pl errors around
  bracket placement. I've kept the new entry consistent with what's
  there rather than reformatting the whole table, but be prepared for
  complaints about spaces.

Thoughts?
-- David

---
 Documentation/admin-guide/tainted-kernels.rst | 1 +
 include/linux/panic.h                         | 3 ++-
 kernel/panic.c                                | 1 +
 lib/kunit/test.c                              | 4 ++++
 4 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/tainted-kernels.rst b/Documentation/admin-guide/tainted-kernels.rst
index ceeed7b0798d..8f18fc4659d4 100644
--- a/Documentation/admin-guide/tainted-kernels.rst
+++ b/Documentation/admin-guide/tainted-kernels.rst
@@ -100,6 +100,7 @@ Bit  Log  Number  Reason that got the kernel tainted
  15  _/K   32768  kernel has been live patched
  16  _/X   65536  auxiliary taint, defined for and used by distros
  17  _/T  131072  kernel was built with the struct randomization plugin
+ 18  _/N  262144  a KUnit test has been run
 ===  ===  ======  ========================================================
 
 Note: The character ``_`` is representing a blank in this table to make reading
diff --git a/include/linux/panic.h b/include/linux/panic.h
index f5844908a089..1d316c26bf27 100644
--- a/include/linux/panic.h
+++ b/include/linux/panic.h
@@ -74,7 +74,8 @@ static inline void set_arch_panic_timeout(int timeout, int arch_default_timeout)
 #define TAINT_LIVEPATCH			15
 #define TAINT_AUX			16
 #define TAINT_RANDSTRUCT		17
-#define TAINT_FLAGS_COUNT		18
+#define TAINT_KUNIT			18
+#define TAINT_FLAGS_COUNT		19
 #define TAINT_FLAGS_MAX			((1UL << TAINT_FLAGS_COUNT) - 1)
 
 struct taint_flag {
diff --git a/kernel/panic.c b/kernel/panic.c
index eb4dfb932c85..b24ca63ed738 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -404,6 +404,7 @@ const struct taint_flag taint_flags[TAINT_FLAGS_COUNT] = {
 	[ TAINT_LIVEPATCH ]		= { 'K', ' ', true },
 	[ TAINT_AUX ]			= { 'X', ' ', true },
 	[ TAINT_RANDSTRUCT ]		= { 'T', ' ', true },
+	[ TAINT_KUNIT ]			= { 'N', ' ', false },
 };
 
 /**
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 0f66c13d126e..ea8e9162445d 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -11,6 +11,7 @@
 #include <kunit/test-bug.h>
 #include <linux/kernel.h>
 #include <linux/moduleparam.h>
+#include <linux/panic.h>
 #include <linux/sched/debug.h>
 #include <linux/sched.h>
 
@@ -498,6 +499,9 @@ int kunit_run_tests(struct kunit_suite *suite)
 	struct kunit_result_stats suite_stats = { 0 };
 	struct kunit_result_stats total_stats = { 0 };
 
+	/* Taint the kernel so we know we've run tests. */
+	add_taint(TAINT_KUNIT, LOCKDEP_STILL_OK);
+
 	kunit_print_subtest_start(suite);
 
 	kunit_suite_for_each_test_case(suite, test_case) {
-- 
2.36.0.464.gb9c8b46e94-goog

Re: [PATCH] kunit: Taint kernel if any tests run

[Greg KH]

On Fri, Apr 29, 2022 at 12:39:14PM +0800, David Gow wrote:
> KUnit tests are not supposed to run on production systems: they may do
> deliberately illegal things to trigger errors, and have security
> implications (assertions will often deliberately leak kernel addresses).
> 
> Add a new taint type, TAINT_KUNIT to signal that a KUnit test has been
> run. This will be printed as 'N' (for kuNit, as K, U and T were already
> taken).
> 
> This should discourage people from running KUnit tests on production
> systems, and to make it easier to tell if tests have been run
> accidentally (by loading the wrong configuration, etc.)
> 
> Signed-off-by: David Gow <davidgow@google.com>
> ---
> 
> This is something I'd been thinking about for a while, and it came up
> again, so I'm finally giving it a go.
> 
> Two notes:
> - I decided to add a new type of taint, as none of the existing ones
>   really seemed to fit. We could live with considering KUnit tests as
>   TAINT_WARN or TAINT_CRAP or something otherwise, but neither are quite
>   right.
> - The taint_flags table gives a couple of checkpatch.pl errors around
>   bracket placement. I've kept the new entry consistent with what's
>   there rather than reformatting the whole table, but be prepared for
>   complaints about spaces.
> 
> Thoughts?
> -- David
> 
> ---
>  Documentation/admin-guide/tainted-kernels.rst | 1 +
>  include/linux/panic.h                         | 3 ++-
>  kernel/panic.c                                | 1 +
>  lib/kunit/test.c                              | 4 ++++
>  4 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/admin-guide/tainted-kernels.rst b/Documentation/admin-guide/tainted-kernels.rst
> index ceeed7b0798d..8f18fc4659d4 100644
> --- a/Documentation/admin-guide/tainted-kernels.rst
> +++ b/Documentation/admin-guide/tainted-kernels.rst
> @@ -100,6 +100,7 @@ Bit  Log  Number  Reason that got the kernel tainted
>   15  _/K   32768  kernel has been live patched
>   16  _/X   65536  auxiliary taint, defined for and used by distros
>   17  _/T  131072  kernel was built with the struct randomization plugin
> + 18  _/N  262144  a KUnit test has been run
>  ===  ===  ======  ========================================================
>  
>  Note: The character ``_`` is representing a blank in this table to make reading
> diff --git a/include/linux/panic.h b/include/linux/panic.h
> index f5844908a089..1d316c26bf27 100644
> --- a/include/linux/panic.h
> +++ b/include/linux/panic.h
> @@ -74,7 +74,8 @@ static inline void set_arch_panic_timeout(int timeout, int arch_default_timeout)
>  #define TAINT_LIVEPATCH			15
>  #define TAINT_AUX			16
>  #define TAINT_RANDSTRUCT		17
> -#define TAINT_FLAGS_COUNT		18
> +#define TAINT_KUNIT			18
> +#define TAINT_FLAGS_COUNT		19
>  #define TAINT_FLAGS_MAX			((1UL << TAINT_FLAGS_COUNT) - 1)
>  
>  struct taint_flag {
> diff --git a/kernel/panic.c b/kernel/panic.c
> index eb4dfb932c85..b24ca63ed738 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -404,6 +404,7 @@ const struct taint_flag taint_flags[TAINT_FLAGS_COUNT] = {
>  	[ TAINT_LIVEPATCH ]		= { 'K', ' ', true },
>  	[ TAINT_AUX ]			= { 'X', ' ', true },
>  	[ TAINT_RANDSTRUCT ]		= { 'T', ' ', true },
> +	[ TAINT_KUNIT ]			= { 'N', ' ', false },

As kunit tests can be in modules, shouldn't this be "true" here?

Overall, I like it, makes sense to me.  The "N" will take some getting
used to, and I have no idea why "T" was for "struct randomization", that
would have allowed you to use "T" instead.  Oh well.

thanks,

greg k-h

Re: [PATCH] kunit: Taint kernel if any tests run

[Jani Nikula]

On Fri, 29 Apr 2022, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Fri, Apr 29, 2022 at 12:39:14PM +0800, David Gow wrote:
>> KUnit tests are not supposed to run on production systems: they may do
>> deliberately illegal things to trigger errors, and have security
>> implications (assertions will often deliberately leak kernel addresses).
>> 
>> Add a new taint type, TAINT_KUNIT to signal that a KUnit test has been
>> run. This will be printed as 'N' (for kuNit, as K, U and T were already
>> taken).
>> 
>> This should discourage people from running KUnit tests on production
>> systems, and to make it easier to tell if tests have been run
>> accidentally (by loading the wrong configuration, etc.)
>> 
>> Signed-off-by: David Gow <davidgow@google.com>
>> ---
>> 
>> This is something I'd been thinking about for a while, and it came up
>> again, so I'm finally giving it a go.
>> 
>> Two notes:
>> - I decided to add a new type of taint, as none of the existing ones
>>   really seemed to fit. We could live with considering KUnit tests as
>>   TAINT_WARN or TAINT_CRAP or something otherwise, but neither are quite
>>   right.
>> - The taint_flags table gives a couple of checkpatch.pl errors around
>>   bracket placement. I've kept the new entry consistent with what's
>>   there rather than reformatting the whole table, but be prepared for
>>   complaints about spaces.
>> 
>> Thoughts?
>> -- David
>> 
>> ---
>>  Documentation/admin-guide/tainted-kernels.rst | 1 +
>>  include/linux/panic.h                         | 3 ++-
>>  kernel/panic.c                                | 1 +
>>  lib/kunit/test.c                              | 4 ++++
>>  4 files changed, 8 insertions(+), 1 deletion(-)
>> 
>> diff --git a/Documentation/admin-guide/tainted-kernels.rst b/Documentation/admin-guide/tainted-kernels.rst
>> index ceeed7b0798d..8f18fc4659d4 100644
>> --- a/Documentation/admin-guide/tainted-kernels.rst
>> +++ b/Documentation/admin-guide/tainted-kernels.rst
>> @@ -100,6 +100,7 @@ Bit  Log  Number  Reason that got the kernel tainted
>>   15  _/K   32768  kernel has been live patched
>>   16  _/X   65536  auxiliary taint, defined for and used by distros
>>   17  _/T  131072  kernel was built with the struct randomization plugin
>> + 18  _/N  262144  a KUnit test has been run
>>  ===  ===  ======  ========================================================
>>  
>>  Note: The character ``_`` is representing a blank in this table to make reading
>> diff --git a/include/linux/panic.h b/include/linux/panic.h
>> index f5844908a089..1d316c26bf27 100644
>> --- a/include/linux/panic.h
>> +++ b/include/linux/panic.h
>> @@ -74,7 +74,8 @@ static inline void set_arch_panic_timeout(int timeout, int arch_default_timeout)
>>  #define TAINT_LIVEPATCH			15
>>  #define TAINT_AUX			16
>>  #define TAINT_RANDSTRUCT		17
>> -#define TAINT_FLAGS_COUNT		18
>> +#define TAINT_KUNIT			18
>> +#define TAINT_FLAGS_COUNT		19
>>  #define TAINT_FLAGS_MAX			((1UL << TAINT_FLAGS_COUNT) - 1)
>>  
>>  struct taint_flag {
>> diff --git a/kernel/panic.c b/kernel/panic.c
>> index eb4dfb932c85..b24ca63ed738 100644
>> --- a/kernel/panic.c
>> +++ b/kernel/panic.c
>> @@ -404,6 +404,7 @@ const struct taint_flag taint_flags[TAINT_FLAGS_COUNT] = {
>>  	[ TAINT_LIVEPATCH ]		= { 'K', ' ', true },
>>  	[ TAINT_AUX ]			= { 'X', ' ', true },
>>  	[ TAINT_RANDSTRUCT ]		= { 'T', ' ', true },
>> +	[ TAINT_KUNIT ]			= { 'N', ' ', false },
>
> As kunit tests can be in modules, shouldn't this be "true" here?
>
> Overall, I like it, makes sense to me.  The "N" will take some getting
> used to, and I have no idea why "T" was for "struct randomization", that
> would have allowed you to use "T" instead.  Oh well.

Would you consider a patch adding more self-explanatory taint flag
strings to the output?

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH] kunit: Taint kernel if any tests run

[Greg KH]

On Fri, Apr 29, 2022 at 02:21:26PM +0300, Jani Nikula wrote:
> On Fri, 29 Apr 2022, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Fri, Apr 29, 2022 at 12:39:14PM +0800, David Gow wrote:
> >> KUnit tests are not supposed to run on production systems: they may do
> >> deliberately illegal things to trigger errors, and have security
> >> implications (assertions will often deliberately leak kernel addresses).
> >> 
> >> Add a new taint type, TAINT_KUNIT to signal that a KUnit test has been
> >> run. This will be printed as 'N' (for kuNit, as K, U and T were already
> >> taken).
> >> 
> >> This should discourage people from running KUnit tests on production
> >> systems, and to make it easier to tell if tests have been run
> >> accidentally (by loading the wrong configuration, etc.)
> >> 
> >> Signed-off-by: David Gow <davidgow@google.com>
> >> ---
> >> 
> >> This is something I'd been thinking about for a while, and it came up
> >> again, so I'm finally giving it a go.
> >> 
> >> Two notes:
> >> - I decided to add a new type of taint, as none of the existing ones
> >>   really seemed to fit. We could live with considering KUnit tests as
> >>   TAINT_WARN or TAINT_CRAP or something otherwise, but neither are quite
> >>   right.
> >> - The taint_flags table gives a couple of checkpatch.pl errors around
> >>   bracket placement. I've kept the new entry consistent with what's
> >>   there rather than reformatting the whole table, but be prepared for
> >>   complaints about spaces.
> >> 
> >> Thoughts?
> >> -- David
> >> 
> >> ---
> >>  Documentation/admin-guide/tainted-kernels.rst | 1 +
> >>  include/linux/panic.h                         | 3 ++-
> >>  kernel/panic.c                                | 1 +
> >>  lib/kunit/test.c                              | 4 ++++
> >>  4 files changed, 8 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/Documentation/admin-guide/tainted-kernels.rst b/Documentation/admin-guide/tainted-kernels.rst
> >> index ceeed7b0798d..8f18fc4659d4 100644
> >> --- a/Documentation/admin-guide/tainted-kernels.rst
> >> +++ b/Documentation/admin-guide/tainted-kernels.rst
> >> @@ -100,6 +100,7 @@ Bit  Log  Number  Reason that got the kernel tainted
> >>   15  _/K   32768  kernel has been live patched
> >>   16  _/X   65536  auxiliary taint, defined for and used by distros
> >>   17  _/T  131072  kernel was built with the struct randomization plugin
> >> + 18  _/N  262144  a KUnit test has been run
> >>  ===  ===  ======  ========================================================
> >>  
> >>  Note: The character ``_`` is representing a blank in this table to make reading
> >> diff --git a/include/linux/panic.h b/include/linux/panic.h
> >> index f5844908a089..1d316c26bf27 100644
> >> --- a/include/linux/panic.h
> >> +++ b/include/linux/panic.h
> >> @@ -74,7 +74,8 @@ static inline void set_arch_panic_timeout(int timeout, int arch_default_timeout)
> >>  #define TAINT_LIVEPATCH			15
> >>  #define TAINT_AUX			16
> >>  #define TAINT_RANDSTRUCT		17
> >> -#define TAINT_FLAGS_COUNT		18
> >> +#define TAINT_KUNIT			18
> >> +#define TAINT_FLAGS_COUNT		19
> >>  #define TAINT_FLAGS_MAX			((1UL << TAINT_FLAGS_COUNT) - 1)
> >>  
> >>  struct taint_flag {
> >> diff --git a/kernel/panic.c b/kernel/panic.c
> >> index eb4dfb932c85..b24ca63ed738 100644
> >> --- a/kernel/panic.c
> >> +++ b/kernel/panic.c
> >> @@ -404,6 +404,7 @@ const struct taint_flag taint_flags[TAINT_FLAGS_COUNT] = {
> >>  	[ TAINT_LIVEPATCH ]		= { 'K', ' ', true },
> >>  	[ TAINT_AUX ]			= { 'X', ' ', true },
> >>  	[ TAINT_RANDSTRUCT ]		= { 'T', ' ', true },
> >> +	[ TAINT_KUNIT ]			= { 'N', ' ', false },
> >
> > As kunit tests can be in modules, shouldn't this be "true" here?
> >
> > Overall, I like it, makes sense to me.  The "N" will take some getting
> > used to, and I have no idea why "T" was for "struct randomization", that
> > would have allowed you to use "T" instead.  Oh well.
> 
> Would you consider a patch adding more self-explanatory taint flag
> strings to the output?

Where would those strings go?  In the oops report?  Or somewhere else?

thanks,

greg k-h

Re: [PATCH] kunit: Taint kernel if any tests run

[Jani Nikula]

On Fri, 29 Apr 2022, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Fri, Apr 29, 2022 at 02:21:26PM +0300, Jani Nikula wrote:
>> On Fri, 29 Apr 2022, Greg KH <gregkh@linuxfoundation.org> wrote:
>> > On Fri, Apr 29, 2022 at 12:39:14PM +0800, David Gow wrote:
>> >> KUnit tests are not supposed to run on production systems: they may do
>> >> deliberately illegal things to trigger errors, and have security
>> >> implications (assertions will often deliberately leak kernel addresses).
>> >> 
>> >> Add a new taint type, TAINT_KUNIT to signal that a KUnit test has been
>> >> run. This will be printed as 'N' (for kuNit, as K, U and T were already
>> >> taken).
>> >> 
>> >> This should discourage people from running KUnit tests on production
>> >> systems, and to make it easier to tell if tests have been run
>> >> accidentally (by loading the wrong configuration, etc.)
>> >> 
>> >> Signed-off-by: David Gow <davidgow@google.com>
>> >> ---
>> >> 
>> >> This is something I'd been thinking about for a while, and it came up
>> >> again, so I'm finally giving it a go.
>> >> 
>> >> Two notes:
>> >> - I decided to add a new type of taint, as none of the existing ones
>> >>   really seemed to fit. We could live with considering KUnit tests as
>> >>   TAINT_WARN or TAINT_CRAP or something otherwise, but neither are quite
>> >>   right.
>> >> - The taint_flags table gives a couple of checkpatch.pl errors around
>> >>   bracket placement. I've kept the new entry consistent with what's
>> >>   there rather than reformatting the whole table, but be prepared for
>> >>   complaints about spaces.
>> >> 
>> >> Thoughts?
>> >> -- David
>> >> 
>> >> ---
>> >>  Documentation/admin-guide/tainted-kernels.rst | 1 +
>> >>  include/linux/panic.h                         | 3 ++-
>> >>  kernel/panic.c                                | 1 +
>> >>  lib/kunit/test.c                              | 4 ++++
>> >>  4 files changed, 8 insertions(+), 1 deletion(-)
>> >> 
>> >> diff --git a/Documentation/admin-guide/tainted-kernels.rst b/Documentation/admin-guide/tainted-kernels.rst
>> >> index ceeed7b0798d..8f18fc4659d4 100644
>> >> --- a/Documentation/admin-guide/tainted-kernels.rst
>> >> +++ b/Documentation/admin-guide/tainted-kernels.rst
>> >> @@ -100,6 +100,7 @@ Bit  Log  Number  Reason that got the kernel tainted
>> >>   15  _/K   32768  kernel has been live patched
>> >>   16  _/X   65536  auxiliary taint, defined for and used by distros
>> >>   17  _/T  131072  kernel was built with the struct randomization plugin
>> >> + 18  _/N  262144  a KUnit test has been run
>> >>  ===  ===  ======  ========================================================
>> >>  
>> >>  Note: The character ``_`` is representing a blank in this table to make reading
>> >> diff --git a/include/linux/panic.h b/include/linux/panic.h
>> >> index f5844908a089..1d316c26bf27 100644
>> >> --- a/include/linux/panic.h
>> >> +++ b/include/linux/panic.h
>> >> @@ -74,7 +74,8 @@ static inline void set_arch_panic_timeout(int timeout, int arch_default_timeout)
>> >>  #define TAINT_LIVEPATCH			15
>> >>  #define TAINT_AUX			16
>> >>  #define TAINT_RANDSTRUCT		17
>> >> -#define TAINT_FLAGS_COUNT		18
>> >> +#define TAINT_KUNIT			18
>> >> +#define TAINT_FLAGS_COUNT		19
>> >>  #define TAINT_FLAGS_MAX			((1UL << TAINT_FLAGS_COUNT) - 1)
>> >>  
>> >>  struct taint_flag {
>> >> diff --git a/kernel/panic.c b/kernel/panic.c
>> >> index eb4dfb932c85..b24ca63ed738 100644
>> >> --- a/kernel/panic.c
>> >> +++ b/kernel/panic.c
>> >> @@ -404,6 +404,7 @@ const struct taint_flag taint_flags[TAINT_FLAGS_COUNT] = {
>> >>  	[ TAINT_LIVEPATCH ]		= { 'K', ' ', true },
>> >>  	[ TAINT_AUX ]			= { 'X', ' ', true },
>> >>  	[ TAINT_RANDSTRUCT ]		= { 'T', ' ', true },
>> >> +	[ TAINT_KUNIT ]			= { 'N', ' ', false },
>> >
>> > As kunit tests can be in modules, shouldn't this be "true" here?
>> >
>> > Overall, I like it, makes sense to me.  The "N" will take some getting
>> > used to, and I have no idea why "T" was for "struct randomization", that
>> > would have allowed you to use "T" instead.  Oh well.
>> 
>> Would you consider a patch adding more self-explanatory taint flag
>> strings to the output?
>
> Where would those strings go?  In the oops report?  Or somewhere else?

I was thinking the oops report. Basically most times I look at an oops
with taint, I need to double check what the flags mean. There are soon
19 of them, you need to look at a lot of oops to remember them all.

Currently we also print ' ' (or 'G' in case of non-properietary module)
for every unset taint flag. If we stopped doing that we wouldn't even
need that much more horizontal space for the strings, unless several
flags were set. (I assume people who do remember all the flags by heart
would still want to keep them too.)

BR,
Jani.


>
> thanks,
>
> greg k-h

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH] kunit: Taint kernel if any tests run

[Greg KH]

On Fri, Apr 29, 2022 at 02:54:25PM +0300, Jani Nikula wrote:
> On Fri, 29 Apr 2022, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Fri, Apr 29, 2022 at 02:21:26PM +0300, Jani Nikula wrote:
> >> On Fri, 29 Apr 2022, Greg KH <gregkh@linuxfoundation.org> wrote:
> >> > On Fri, Apr 29, 2022 at 12:39:14PM +0800, David Gow wrote:
> >> >> KUnit tests are not supposed to run on production systems: they may do
> >> >> deliberately illegal things to trigger errors, and have security
> >> >> implications (assertions will often deliberately leak kernel addresses).
> >> >> 
> >> >> Add a new taint type, TAINT_KUNIT to signal that a KUnit test has been
> >> >> run. This will be printed as 'N' (for kuNit, as K, U and T were already
> >> >> taken).
> >> >> 
> >> >> This should discourage people from running KUnit tests on production
> >> >> systems, and to make it easier to tell if tests have been run
> >> >> accidentally (by loading the wrong configuration, etc.)
> >> >> 
> >> >> Signed-off-by: David Gow <davidgow@google.com>
> >> >> ---
> >> >> 
> >> >> This is something I'd been thinking about for a while, and it came up
> >> >> again, so I'm finally giving it a go.
> >> >> 
> >> >> Two notes:
> >> >> - I decided to add a new type of taint, as none of the existing ones
> >> >>   really seemed to fit. We could live with considering KUnit tests as
> >> >>   TAINT_WARN or TAINT_CRAP or something otherwise, but neither are quite
> >> >>   right.
> >> >> - The taint_flags table gives a couple of checkpatch.pl errors around
> >> >>   bracket placement. I've kept the new entry consistent with what's
> >> >>   there rather than reformatting the whole table, but be prepared for
> >> >>   complaints about spaces.
> >> >> 
> >> >> Thoughts?
> >> >> -- David
> >> >> 
> >> >> ---
> >> >>  Documentation/admin-guide/tainted-kernels.rst | 1 +
> >> >>  include/linux/panic.h                         | 3 ++-
> >> >>  kernel/panic.c                                | 1 +
> >> >>  lib/kunit/test.c                              | 4 ++++
> >> >>  4 files changed, 8 insertions(+), 1 deletion(-)
> >> >> 
> >> >> diff --git a/Documentation/admin-guide/tainted-kernels.rst b/Documentation/admin-guide/tainted-kernels.rst
> >> >> index ceeed7b0798d..8f18fc4659d4 100644
> >> >> --- a/Documentation/admin-guide/tainted-kernels.rst
> >> >> +++ b/Documentation/admin-guide/tainted-kernels.rst
> >> >> @@ -100,6 +100,7 @@ Bit  Log  Number  Reason that got the kernel tainted
> >> >>   15  _/K   32768  kernel has been live patched
> >> >>   16  _/X   65536  auxiliary taint, defined for and used by distros
> >> >>   17  _/T  131072  kernel was built with the struct randomization plugin
> >> >> + 18  _/N  262144  a KUnit test has been run
> >> >>  ===  ===  ======  ========================================================
> >> >>  
> >> >>  Note: The character ``_`` is representing a blank in this table to make reading
> >> >> diff --git a/include/linux/panic.h b/include/linux/panic.h
> >> >> index f5844908a089..1d316c26bf27 100644
> >> >> --- a/include/linux/panic.h
> >> >> +++ b/include/linux/panic.h
> >> >> @@ -74,7 +74,8 @@ static inline void set_arch_panic_timeout(int timeout, int arch_default_timeout)
> >> >>  #define TAINT_LIVEPATCH			15
> >> >>  #define TAINT_AUX			16
> >> >>  #define TAINT_RANDSTRUCT		17
> >> >> -#define TAINT_FLAGS_COUNT		18
> >> >> +#define TAINT_KUNIT			18
> >> >> +#define TAINT_FLAGS_COUNT		19
> >> >>  #define TAINT_FLAGS_MAX			((1UL << TAINT_FLAGS_COUNT) - 1)
> >> >>  
> >> >>  struct taint_flag {
> >> >> diff --git a/kernel/panic.c b/kernel/panic.c
> >> >> index eb4dfb932c85..b24ca63ed738 100644
> >> >> --- a/kernel/panic.c
> >> >> +++ b/kernel/panic.c
> >> >> @@ -404,6 +404,7 @@ const struct taint_flag taint_flags[TAINT_FLAGS_COUNT] = {
> >> >>  	[ TAINT_LIVEPATCH ]		= { 'K', ' ', true },
> >> >>  	[ TAINT_AUX ]			= { 'X', ' ', true },
> >> >>  	[ TAINT_RANDSTRUCT ]		= { 'T', ' ', true },
> >> >> +	[ TAINT_KUNIT ]			= { 'N', ' ', false },
> >> >
> >> > As kunit tests can be in modules, shouldn't this be "true" here?
> >> >
> >> > Overall, I like it, makes sense to me.  The "N" will take some getting
> >> > used to, and I have no idea why "T" was for "struct randomization", that
> >> > would have allowed you to use "T" instead.  Oh well.
> >> 
> >> Would you consider a patch adding more self-explanatory taint flag
> >> strings to the output?
> >
> > Where would those strings go?  In the oops report?  Or somewhere else?
> 
> I was thinking the oops report. Basically most times I look at an oops
> with taint, I need to double check what the flags mean. There are soon
> 19 of them, you need to look at a lot of oops to remember them all.

I agree, it isn't easy to remember.

> Currently we also print ' ' (or 'G' in case of non-properietary module)
> for every unset taint flag. If we stopped doing that we wouldn't even
> need that much more horizontal space for the strings, unless several
> flags were set. (I assume people who do remember all the flags by heart
> would still want to keep them too.)

I recommend keeping the current layout, but maybe adding a new line that
gives the "key" for what the current taint flags mean?

For example, the oops report here:
	https://lore.kernel.org/r/20220413033425.GM16799@magnolia
Has the lines:
	kernel BUG at mm/filemap.c:1653!
	invalid opcode: 0000 [#1] PREEMPT SMP
	CPU: 0 PID: 1349866 Comm: 0:116 Tainted: G        W         5.18.0-rc2-djwx #rc2 19cc48221d47ada6c8e5859639b6a0946c9a3777
	Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20171121_152543-x86-ol7-builder-01.us.oracle.com-4.el7.1 04/01/2014
	Workqueue: xfs-conv/sda4 xfs_end_io [xfs]
	RIP: 0010:folio_end_writeback+0x79/0x80

Perhaps we add another line right before or after "Hardware name:" that
lists the flags that are set at the moment and what they mean:

	Taint flags: [G]=PROPRIETARY_MODULE, [W]=WARN

Or something like that (format was a first guess only).

Anyway, might be helpful?

thanks,

greg k-h

Re: [PATCH] kunit: Taint kernel if any tests run

[David Gow]

On Fri, Apr 29, 2022 at 3:09 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Fri, Apr 29, 2022 at 12:39:14PM +0800, David Gow wrote:
> > KUnit tests are not supposed to run on production systems: they may do
> > deliberately illegal things to trigger errors, and have security
> > implications (assertions will often deliberately leak kernel addresses).
> >
> > Add a new taint type, TAINT_KUNIT to signal that a KUnit test has been
> > run. This will be printed as 'N' (for kuNit, as K, U and T were already
> > taken).
> >
> > This should discourage people from running KUnit tests on production
> > systems, and to make it easier to tell if tests have been run
> > accidentally (by loading the wrong configuration, etc.)
> >
> > Signed-off-by: David Gow <davidgow@google.com>

< snip >

> > +     [ TAINT_KUNIT ]                 = { 'N', ' ', false },
>
> As kunit tests can be in modules, shouldn't this be "true" here?

Ah, good catch. While I tend to use either built-in tests (or modules
which are immediately unloaded), there are definitely some cases where
the tests are part of long-lasting modules.

I'll send out v2 with that changed.

> Overall, I like it, makes sense to me.  The "N" will take some getting
> used to, and I have no idea why "T" was for "struct randomization", that
> would have allowed you to use "T" instead.  Oh well.

Yeah, 'T' would've been nice, but I doubt it'd be worth trying to
change it now. At least we haven't had to resort to emoji...

Adding an actual name as Jani suggested would be a good idea, IMHO,
though obviously best done in a separate patch.


Cheers,
-- David

[PATCH v2] kunit: Taint kernel if any tests run

[David Gow]

KUnit tests are not supposed to run on production systems: they may do
deliberately illegal things to trigger errors, and have security
implications (assertions will often deliberately leak kernel addresses).

Add a new taint type, TAINT_KUNIT to signal that a KUnit test has been
run. This will be printed as 'N' (for kuNit, as K, U and T were already
taken).

This should discourage people from running KUnit tests on production
systems, and to make it easier to tell if tests have been run
accidentally (by loading the wrong configuration, etc.)

Signed-off-by: David Gow <davidgow@google.com>
---

Changes since v1:
https://lore.kernel.org/linux-kselftest/20220429043913.626647-1-davidgow@google.com/
- Make the taint per-module, to handle the case when tests are in
  (longer lasting) modules. (Thanks Greg KH).

Note that this still has checkpatch.pl warnings around bracket
placement, which are intentional as part of matching the surrounding
code.

---
 Documentation/admin-guide/tainted-kernels.rst | 1 +
 include/linux/panic.h                         | 3 ++-
 kernel/panic.c                                | 1 +
 lib/kunit/test.c                              | 4 ++++
 4 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/tainted-kernels.rst b/Documentation/admin-guide/tainted-kernels.rst
index ceeed7b0798d..8f18fc4659d4 100644
--- a/Documentation/admin-guide/tainted-kernels.rst
+++ b/Documentation/admin-guide/tainted-kernels.rst
@@ -100,6 +100,7 @@ Bit  Log  Number  Reason that got the kernel tainted
  15  _/K   32768  kernel has been live patched
  16  _/X   65536  auxiliary taint, defined for and used by distros
  17  _/T  131072  kernel was built with the struct randomization plugin
+ 18  _/N  262144  a KUnit test has been run
 ===  ===  ======  ========================================================
 
 Note: The character ``_`` is representing a blank in this table to make reading
diff --git a/include/linux/panic.h b/include/linux/panic.h
index f5844908a089..1d316c26bf27 100644
--- a/include/linux/panic.h
+++ b/include/linux/panic.h
@@ -74,7 +74,8 @@ static inline void set_arch_panic_timeout(int timeout, int arch_default_timeout)
 #define TAINT_LIVEPATCH			15
 #define TAINT_AUX			16
 #define TAINT_RANDSTRUCT		17
-#define TAINT_FLAGS_COUNT		18
+#define TAINT_KUNIT			18
+#define TAINT_FLAGS_COUNT		19
 #define TAINT_FLAGS_MAX			((1UL << TAINT_FLAGS_COUNT) - 1)
 
 struct taint_flag {
diff --git a/kernel/panic.c b/kernel/panic.c
index eb4dfb932c85..9a026d98a00c 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -404,6 +404,7 @@ const struct taint_flag taint_flags[TAINT_FLAGS_COUNT] = {
 	[ TAINT_LIVEPATCH ]		= { 'K', ' ', true },
 	[ TAINT_AUX ]			= { 'X', ' ', true },
 	[ TAINT_RANDSTRUCT ]		= { 'T', ' ', true },
+	[ TAINT_KUNIT ]			= { 'N', ' ', true },
 };
 
 /**
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 0f66c13d126e..ea8e9162445d 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -11,6 +11,7 @@
 #include <kunit/test-bug.h>
 #include <linux/kernel.h>
 #include <linux/moduleparam.h>
+#include <linux/panic.h>
 #include <linux/sched/debug.h>
 #include <linux/sched.h>
 
@@ -498,6 +499,9 @@ int kunit_run_tests(struct kunit_suite *suite)
 	struct kunit_result_stats suite_stats = { 0 };
 	struct kunit_result_stats total_stats = { 0 };
 
+	/* Taint the kernel so we know we've run tests. */
+	add_taint(TAINT_KUNIT, LOCKDEP_STILL_OK);
+
 	kunit_print_subtest_start(suite);
 
 	kunit_suite_for_each_test_case(suite, test_case) {
-- 
2.36.0.464.gb9c8b46e94-goog

Re: [PATCH v2] kunit: Taint kernel if any tests run

[Greg KH]

On Sat, Apr 30, 2022 at 11:00:19AM +0800, David Gow wrote:
> KUnit tests are not supposed to run on production systems: they may do
> deliberately illegal things to trigger errors, and have security
> implications (assertions will often deliberately leak kernel addresses).
> 
> Add a new taint type, TAINT_KUNIT to signal that a KUnit test has been
> run. This will be printed as 'N' (for kuNit, as K, U and T were already
> taken).
> 
> This should discourage people from running KUnit tests on production
> systems, and to make it easier to tell if tests have been run
> accidentally (by loading the wrong configuration, etc.)
> 
> Signed-off-by: David Gow <davidgow@google.com>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH v2] kunit: Taint kernel if any tests run

[Luis Chamberlain]

On Sat, Apr 30, 2022 at 11:00:19AM +0800, David Gow wrote:
> KUnit tests are not supposed to run on production systems: they may do
> deliberately illegal things to trigger errors, and have security
> implications (assertions will often deliberately leak kernel addresses).
> 
> Add a new taint type, TAINT_KUNIT to signal that a KUnit test has been
> run. This will be printed as 'N' (for kuNit, as K, U and T were already
> taken).
> 
> This should discourage people from running KUnit tests on production
> systems, and to make it easier to tell if tests have been run
> accidentally (by loading the wrong configuration, etc.)
> 
> Signed-off-by: David Gow <davidgow@google.com>

There is no reason to distinguish kunit from selftests if the result is
the same: really make the kernel try really insane stupid things which
may crash it or put it into a bad state.

So no, this should be renamed to "TEST_BREAK" as I think outside of
selftest and kunit we may grow the kernel to do stupid things outside
of that domain and this gives us the flexilibilty to use that in other
places as well.

It begs the question if we *should* allow userspace to volunterally say
"hey, we are doing really insane things, brace yourself." Why ? Well
because selftest has tons of modules. We either then define a macro
that adds the taint for them and wrap the module declaration for it,
or we expose a syctl to let userspace volunteer to opt-in to seggest
we are about to try something stupid with the kernel including loading
some dangeerous modules which may not have macros which taint the kernel.
That would let selftest taint on *any* selftest. Because we can run all
selftests or run one selftest.

Then, if such sysctl is exposed, maybe we should then also use this for
example for blktests, fstests, fio tests, etc.

  Luis

Re: [PATCH v2] kunit: Taint kernel if any tests run

[Luis Chamberlain]

On Sun, May 01, 2022 at 11:22:38AM -0700, Luis Chamberlain wrote:
> On Sat, Apr 30, 2022 at 11:00:19AM +0800, David Gow wrote:
> > KUnit tests are not supposed to run on production systems: they may do
> > deliberately illegal things to trigger errors, and have security
> > implications (assertions will often deliberately leak kernel addresses).
> > 
> > Add a new taint type, TAINT_KUNIT to signal that a KUnit test has been
> > run. This will be printed as 'N' (for kuNit, as K, U and T were already
> > taken).
> > 
> > This should discourage people from running KUnit tests on production
> > systems, and to make it easier to tell if tests have been run
> > accidentally (by loading the wrong configuration, etc.)
> > 
> > Signed-off-by: David Gow <davidgow@google.com>
> 
> There is no reason to distinguish kunit from selftests if the result is
> the same: really make the kernel try really insane stupid things which
> may crash it or put it into a bad state.
> 
> So no, this should be renamed to "TEST_BREAK" as I think outside of
> selftest and kunit we may grow the kernel to do stupid things outside
> of that domain and this gives us the flexilibilty to use that in other
> places as well.
> 
> It begs the question if we *should* allow userspace to volunterally say
> "hey, we are doing really insane things, brace yourself." Why ? Well
> because selftest has tons of modules. We either then define a macro
> that adds the taint for them and wrap the module declaration for it,
> or we expose a syctl to let userspace volunteer to opt-in to seggest
> we are about to try something stupid with the kernel including loading
> some dangeerous modules which may not have macros which taint the kernel.
> That would let selftest taint on *any* selftest. Because we can run all
> selftests or run one selftest.
> 
> Then, if such sysctl is exposed, maybe we should then also use this for
> example for blktests, fstests, fio tests, etc.

For got to expand to fsdevel and linux-block.

  Luis

Re: [PATCH v2] kunit: Taint kernel if any tests run

[David Gow]

On Mon, May 2, 2022 at 2:24 AM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Sun, May 01, 2022 at 11:22:38AM -0700, Luis Chamberlain wrote:
> > On Sat, Apr 30, 2022 at 11:00:19AM +0800, David Gow wrote:
> > > KUnit tests are not supposed to run on production systems: they may do
> > > deliberately illegal things to trigger errors, and have security
> > > implications (assertions will often deliberately leak kernel addresses).
> > >
> > > Add a new taint type, TAINT_KUNIT to signal that a KUnit test has been
> > > run. This will be printed as 'N' (for kuNit, as K, U and T were already
> > > taken).
> > >
> > > This should discourage people from running KUnit tests on production
> > > systems, and to make it easier to tell if tests have been run
> > > accidentally (by loading the wrong configuration, etc.)
> > >
> > > Signed-off-by: David Gow <davidgow@google.com>
> >
> > There is no reason to distinguish kunit from selftests if the result is
> > the same: really make the kernel try really insane stupid things which
> > may crash it or put it into a bad state.
> >
My initial thought is that KUnit is explicitly in-kernel testing,
whereas kselftest is (at least somewhat) user-space based. My personal
feeling is that "doing weird stuff from userspace" is fundamentally
different from "doing weird stuff in the kernel". That being said, in
practice many kselftest tests load modules which do strange things,
and those could be in scope for something like that. I'd still err on
the side of only having those tests (or specifically those modules)
add the taint, rather than all selftests, but could be conveniced.

The other thing of note is that KUnit tests do often leak pointer
addresses, which again is something that's a worry in the kernel, and
harmless in userspace.

> > So no, this should be renamed to "TEST_BREAK" as I think outside of
> > selftest and kunit we may grow the kernel to do stupid things outside
> > of that domain and this gives us the flexilibilty to use that in other
> > places as well.
> >
> > It begs the question if we *should* allow userspace to volunterally say
> > "hey, we are doing really insane things, brace yourself." Why ? Well
> > because selftest has tons of modules. We either then define a macro
> > that adds the taint for them and wrap the module declaration for it,
> > or we expose a syctl to let userspace volunteer to opt-in to seggest
> > we are about to try something stupid with the kernel including loading
> > some dangeerous modules which may not have macros which taint the kernel.
> > That would let selftest taint on *any* selftest. Because we can run all
> > selftests or run one selftest.
> >
> > Then, if such sysctl is exposed, maybe we should then also use this for
> > example for blktests, fstests, fio tests, etc.

Is this what TAINT_USER is for?

I think (though haven't actually tried) writing to
/proc/sys/kernel/tainted will add whatever taint flags are needed from
userspace.

That being said, I'm not _against_ making this more general, or
standardising on the existing TAINT_USER, or similar. Personally,
though, I quite like having KUnit separate (but I am, admittedly,
biased :-) ).

Cheers,
-- David

Re: [PATCH v2] kunit: Taint kernel if any tests run

[Luis Chamberlain]

On Tue, May 03, 2022 at 02:49:58PM +0800, David Gow wrote:
> On Mon, May 2, 2022 at 2:24 AM Luis Chamberlain <mcgrof@kernel.org> wrote:
> >
> > On Sun, May 01, 2022 at 11:22:38AM -0700, Luis Chamberlain wrote:
> > > On Sat, Apr 30, 2022 at 11:00:19AM +0800, David Gow wrote:
> > > > KUnit tests are not supposed to run on production systems: they may do
> > > > deliberately illegal things to trigger errors, and have security
> > > > implications (assertions will often deliberately leak kernel addresses).
> > > >
> > > > Add a new taint type, TAINT_KUNIT to signal that a KUnit test has been
> > > > run. This will be printed as 'N' (for kuNit, as K, U and T were already
> > > > taken).
> > > >
> > > > This should discourage people from running KUnit tests on production
> > > > systems, and to make it easier to tell if tests have been run
> > > > accidentally (by loading the wrong configuration, etc.)
> > > >
> > > > Signed-off-by: David Gow <davidgow@google.com>
> > >
> > > There is no reason to distinguish kunit from selftests if the result is
> > > the same: really make the kernel try really insane stupid things which
> > > may crash it or put it into a bad state.
> > >
> My initial thought is that KUnit is explicitly in-kernel testing,
> whereas kselftest is (at least somewhat) user-space based.

selftests has modules, although I am not sure if there are selftests
which do not load modules. Shuah?

> My personal
> feeling is that "doing weird stuff from userspace" is fundamentally
> different from "doing weird stuff in the kernel".

True.

> That being said, in
> practice many kselftest tests load modules which do strange things,
> and those could be in scope for something like that. I'd still err on
> the side of only having those tests (or specifically those modules)
> add the taint, rather than all selftests, but could be conveniced.

Yeah I think now that this can easily be added by having a special
new module info, MODULE_TAINTS(taint_flag). Then in check_modinfo()
you'd get_modinfo(info, "taints") to then add_taint_module() if set.

We can ignore the userspace thing I mentioned earlier as I thought
at first we could not add the taint to selftest modules easily but
we can.

  Luis

Re: [PATCH v2] kunit: Taint kernel if any tests run

[Daniel Latypov]

On Wed, May 4, 2022 at 9:51 AM Luis Chamberlain <mcgrof@kernel.org> wrote:
> selftests has modules, although I am not sure if there are selftests
> which do not load modules. Shuah?

I'm not Shuah, but there are indeed selftests that don't load modules.

I went looking for an example and found
tools/testing/selftests/bpf/test_doc_build.sh, which runs entirely in
userspace (basically just `make docs`).

Re: [PATCH v2] kunit: Taint kernel if any tests run

[Luis Chamberlain]

On Wed, May 04, 2022 at 11:25:14AM -0500, Daniel Latypov wrote:
> On Wed, May 4, 2022 at 9:51 AM Luis Chamberlain <mcgrof@kernel.org> wrote:
> > selftests has modules, although I am not sure if there are selftests
> > which do not load modules. Shuah?
> 
> I'm not Shuah, but there are indeed selftests that don't load modules.
> 
> I went looking for an example and found
> tools/testing/selftests/bpf/test_doc_build.sh, which runs entirely in
> userspace (basically just `make docs`).

OK so, we can just skip tainting considerations for selftests which
don't use modules for now. There may be selftests which do wonky
things in userspace but indeed I agree the userspace taint would
be better for those but I don't think it may be worth bother
worrying about those at this point in time.

But my point in that sharing a taint between kunit / selftests modules
does make sense and is easily possible. The unfortunate aspect is just
that selftests don't have a centralized runner, because I can just
run tools/testing/selftests/sysctl/sysctl.sh for example and that's it.
So I think we have no other option but to just add the module info
manually for selftests at this time.

  Luis

Re: [PATCH v2] kunit: Taint kernel if any tests run

[Daniel Latypov]

On Wed, May 4, 2022 at 1:46 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> OK so, we can just skip tainting considerations for selftests which
> don't use modules for now. There may be selftests which do wonky
> things in userspace but indeed I agree the userspace taint would
> be better for those but I don't think it may be worth bother
> worrying about those at this point in time.
>
> But my point in that sharing a taint between kunit / selftests modules
> does make sense and is easily possible. The unfortunate aspect is just

Yes, I 100% agree that we should share a taint for kernelspace testing
from both kunit/kselftest.
Someone running the system won't care what framework was used.

> that selftests don't have a centralized runner, because I can just
> run tools/testing/selftests/sysctl/sysctl.sh for example and that's it.
> So I think we have no other option but to just add the module info
> manually for selftests at this time.

Somewhat tangential: there's a number of other test modules that
aren't explicitly part of kselftest.
Long-term, I think most of them should be converted to kselftest or
kunit as appropriate, so they'll get taken care of eventually.

A number of these modules depend on CONFIG_DEBUG_KERNEL=y, but we
can't pre-emptively set this new taint flag by checking for it as it's
too widely used :\
E.g. the debian-based distro I'm using right now has it set
$ grep 'CONFIG_DEBUG_KERNEL=y' /boot/config-$(uname -r)
CONFIG_DEBUG_KERNEL=y

-Daniel

Re: [PATCH v2] kunit: Taint kernel if any tests run

[Luis Chamberlain]

On Wed, May 04, 2022 at 02:19:59PM -0500, Daniel Latypov wrote:
> On Wed, May 4, 2022 at 1:46 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> > OK so, we can just skip tainting considerations for selftests which
> > don't use modules for now. There may be selftests which do wonky
> > things in userspace but indeed I agree the userspace taint would
> > be better for those but I don't think it may be worth bother
> > worrying about those at this point in time.
> >
> > But my point in that sharing a taint between kunit / selftests modules
> > does make sense and is easily possible. The unfortunate aspect is just
> 
> Yes, I 100% agree that we should share a taint for kernelspace testing
> from both kunit/kselftest.
> Someone running the system won't care what framework was used.

OK do you mind doing the nasty work of manually adding the new
MODULE_TAINT() to the selftests as part of your effort?

*Alternatively*, if we *moved* all sefltests modules to a new
lib/debug/selftests/ directory or something like that then t would
seem modpost *could* add the taint flag automagically for us without
having to edit or require it on new drivers. We have similar type of
taint for staging, see add_staging_flag().

I would *highly* prefer this approach, event though it is more work,
because I think this is a step we should take anyway.

However, I just checked modules on lib/ and well, some of them are
already in their own directory, like lib/math/test_div64.c. So not
sure, maybe just move a few modules which are just in lib/*.c for now
and then just sprinkle the MODULE_TAINT() to the others?

> > that selftests don't have a centralized runner, because I can just
> > run tools/testing/selftests/sysctl/sysctl.sh for example and that's it.
> > So I think we have no other option but to just add the module info
> > manually for selftests at this time.
> 
> Somewhat tangential: there's a number of other test modules that
> aren't explicitly part of kselftest.

Oh interesting, like which one?

> Long-term, I think most of them should be converted to kselftest or
> kunit as appropriate, so they'll get taken care of eventually.

Makes sense.

  Luis

Re: [PATCH v2] kunit: Taint kernel if any tests run

[Luis Chamberlain]

On Wed, May 04, 2022 at 02:12:30PM -0700, Luis Chamberlain wrote:
> On Wed, May 04, 2022 at 02:19:59PM -0500, Daniel Latypov wrote:
> > On Wed, May 4, 2022 at 1:46 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> > > OK so, we can just skip tainting considerations for selftests which
> > > don't use modules for now. There may be selftests which do wonky
> > > things in userspace but indeed I agree the userspace taint would
> > > be better for those but I don't think it may be worth bother
> > > worrying about those at this point in time.
> > >
> > > But my point in that sharing a taint between kunit / selftests modules
> > > does make sense and is easily possible. The unfortunate aspect is just
> > 
> > Yes, I 100% agree that we should share a taint for kernelspace testing
> > from both kunit/kselftest.
> > Someone running the system won't care what framework was used.
> 
> OK do you mind doing the nasty work of manually adding the new
> MODULE_TAINT() to the selftests as part of your effort?
> 
> *Alternatively*, if we *moved* all sefltests modules to a new
> lib/debug/selftests/ directory or something like that then t would
> seem modpost *could* add the taint flag automagically for us without
> having to edit or require it on new drivers. We have similar type of
> taint for staging, see add_staging_flag().
> 
> I would *highly* prefer this approach, event though it is more work,
> because I think this is a step we should take anyway.
> 
> However, I just checked modules on lib/ and well, some of them are
> already in their own directory, like lib/math/test_div64.c. So not
> sure, maybe just move a few modules which are just in lib/*.c for now
> and then just sprinkle the MODULE_TAINT() to the others?

I *think* we could just pull this off with a much easier approach,
simply looking for the substrings in the module name in modpost.c:

  * "_test." || "-test."
  * ^"test_" || ^"test-"

An issue with this of course is a vendor $FOO with an out of tree
test driver may end up with the taint. Perhaps we don't care.

That means moving selftests to its own directory is not needed at this
point in time.

  Luis

Re: [PATCH v2] kunit: Taint kernel if any tests run

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 3161 bytes --]

On Thu, May 5, 2022 at 1:57 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Wed, May 04, 2022 at 02:12:30PM -0700, Luis Chamberlain wrote:
> > On Wed, May 04, 2022 at 02:19:59PM -0500, Daniel Latypov wrote:
> > > On Wed, May 4, 2022 at 1:46 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> > > > OK so, we can just skip tainting considerations for selftests which
> > > > don't use modules for now. There may be selftests which do wonky
> > > > things in userspace but indeed I agree the userspace taint would
> > > > be better for those but I don't think it may be worth bother
> > > > worrying about those at this point in time.
> > > >
> > > > But my point in that sharing a taint between kunit / selftests modules
> > > > does make sense and is easily possible. The unfortunate aspect is just
> > >
> > > Yes, I 100% agree that we should share a taint for kernelspace testing
> > > from both kunit/kselftest.
> > > Someone running the system won't care what framework was used.
> >
> > OK do you mind doing the nasty work of manually adding the new
> > MODULE_TAINT() to the selftests as part of your effort?
> >
> > *Alternatively*, if we *moved* all sefltests modules to a new
> > lib/debug/selftests/ directory or something like that then t would
> > seem modpost *could* add the taint flag automagically for us without
> > having to edit or require it on new drivers. We have similar type of
> > taint for staging, see add_staging_flag().
> >
> > I would *highly* prefer this approach, event though it is more work,
> > because I think this is a step we should take anyway.
> >
> > However, I just checked modules on lib/ and well, some of them are
> > already in their own directory, like lib/math/test_div64.c. So not
> > sure, maybe just move a few modules which are just in lib/*.c for now
> > and then just sprinkle the MODULE_TAINT() to the others?
>
> I *think* we could just pull this off with a much easier approach,
> simply looking for the substrings in the module name in modpost.c:
>
>   * "_test." || "-test."
>   * ^"test_" || ^"test-"
>
> An issue with this of course is a vendor $FOO with an out of tree
> test driver may end up with the taint. Perhaps we don't care.
>
> That means moving selftests to its own directory is not needed at this
> point in time.

I can't say I'm thrilled with the idea of just doing name comparisons,
particularly since not all of them match this pattern, for example:
bpf_testmod.ko. (Though, frankly, more of them do than I'd've
guessed.)

Maybe adding a taint call to the selftest helper module framework in
kselftest_module.h, though again, there are several tests which don't
use it.

I _suspect_ we'd be able to hit most of them by tainting in frameworks
like the above, and patch the remaining modules manually. There's also
definitely a grey area with things like netdevsim, which are used a
lot as helper modules by selftests, but may have other uses as well.

(The advantage of the KUnit tainting is that, due to KUnit's
centralised executor, we can be sure all KUnit tests will correctly
trigger the taint. But maybe it doesn't matter as much if one or two
selftests miss out.)

-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH v2] kunit: Taint kernel if any tests run

[Luis Chamberlain]

On Fri, May 06, 2022 at 03:01:34PM +0800, David Gow wrote:
> On Thu, May 5, 2022 at 1:57 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> >
> > On Wed, May 04, 2022 at 02:12:30PM -0700, Luis Chamberlain wrote:
> > > On Wed, May 04, 2022 at 02:19:59PM -0500, Daniel Latypov wrote:
> > > > On Wed, May 4, 2022 at 1:46 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> > > > > OK so, we can just skip tainting considerations for selftests which
> > > > > don't use modules for now. There may be selftests which do wonky
> > > > > things in userspace but indeed I agree the userspace taint would
> > > > > be better for those but I don't think it may be worth bother
> > > > > worrying about those at this point in time.
> > > > >
> > > > > But my point in that sharing a taint between kunit / selftests modules
> > > > > does make sense and is easily possible. The unfortunate aspect is just
> > > >
> > > > Yes, I 100% agree that we should share a taint for kernelspace testing
> > > > from both kunit/kselftest.
> > > > Someone running the system won't care what framework was used.
> > >
> > > OK do you mind doing the nasty work of manually adding the new
> > > MODULE_TAINT() to the selftests as part of your effort?
> > >
> > > *Alternatively*, if we *moved* all sefltests modules to a new
> > > lib/debug/selftests/ directory or something like that then t would
> > > seem modpost *could* add the taint flag automagically for us without
> > > having to edit or require it on new drivers. We have similar type of
> > > taint for staging, see add_staging_flag().
> > >
> > > I would *highly* prefer this approach, event though it is more work,
> > > because I think this is a step we should take anyway.
> > >
> > > However, I just checked modules on lib/ and well, some of them are
> > > already in their own directory, like lib/math/test_div64.c. So not
> > > sure, maybe just move a few modules which are just in lib/*.c for now
> > > and then just sprinkle the MODULE_TAINT() to the others?
> >
> > I *think* we could just pull this off with a much easier approach,
> > simply looking for the substrings in the module name in modpost.c:
> >
> >   * "_test." || "-test."
> >   * ^"test_" || ^"test-"
> >
> > An issue with this of course is a vendor $FOO with an out of tree
> > test driver may end up with the taint. Perhaps we don't care.
> >
> > That means moving selftests to its own directory is not needed at this
> > point in time.
> 
> I can't say I'm thrilled with the idea of just doing name comparisons,
> particularly since not all of them match this pattern, for example:
> bpf_testmod.ko. (Though, frankly, more of them do than I'd've
> guessed.)
> 
> Maybe adding a taint call to the selftest helper module framework in
> kselftest_module.h, though again, there are several tests which don't
> use it.

Right, I can't think of a generic way to peg this. I think long term
we do stand to gain to move all selftests under a lib/debug/selftests/
or something like that, but for now what I suggested is the only thing
I can come up with.

> I _suspect_ we'd be able to hit most of them by tainting in frameworks
> like the above, and patch the remaining modules manually.

Works with me.

> There's also
> definitely a grey area with things like netdevsim, which are used a
> lot as helper modules by selftests, but may have other uses as well.

They can peg the module if they want the taint.

> (The advantage of the KUnit tainting is that, due to KUnit's
> centralised executor, we can be sure all KUnit tests will correctly
> trigger the taint. But maybe it doesn't matter as much if one or two
> selftests miss out.)

That is what I was thinking.

I'm convinced we *should* move selftests to a one directory. The
amount of stuff in lib/ is getting out of hand.

  Luis

[PATCH v3 1/3] panic: Taint kernel if tests are run

[David Gow]

Most in-kernel tests (such as KUnit tests) are not supposed to run on
production systems: they may do deliberately illegal things to trigger
errors, and have security implications (for example, KUnit assertions
will often deliberately leak kernel addresses).

Add a new taint type, TAINT_TEST to signal that a test has been run.
This will be printed as 'N' (originally for kuNit, as every other
sensible letter was taken.)

This should discourage people from running these tests on production
systems, and to make it easier to tell if tests have been run
accidentally (by loading the wrong configuration, etc.)

Signed-off-by: David Gow <davidgow@google.com>
---

Updated this to handle the most common case of selftest modules, in
addition to KUnit tests. There's room for other tests or test frameworks
to use this as well, either with a call to add_taint() from within the
kernel, or by writing to /proc/sys/kernel/tainted.

The 'N' character for the taint is even less useful now that it's no
longer short for kuNit, but all the letters in TEST are taken. :-(

Changes since v2:
https://lore.kernel.org/linux-kselftest/20220430030019.803481-1-davidgow@google.com/
- Rename TAINT_KUNIT -> TAINT_TEST.
- Split into separate patches for adding the taint, and triggering it.
- Taint on a kselftest_module being loaded (patch 3/3)

Changes since v1:
https://lore.kernel.org/linux-kselftest/20220429043913.626647-1-davidgow@google.com/
- Make the taint per-module, to handle the case when tests are in
  (longer lasting) modules. (Thanks Greg KH).

Note that this still has checkpatch.pl warnings around bracket
placement, which are intentional as part of matching the surrounding
code.

---
 Documentation/admin-guide/tainted-kernels.rst | 1 +
 include/linux/panic.h                         | 3 ++-
 kernel/panic.c                                | 1 +
 3 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/tainted-kernels.rst b/Documentation/admin-guide/tainted-kernels.rst
index ceeed7b0798d..546f3071940d 100644
--- a/Documentation/admin-guide/tainted-kernels.rst
+++ b/Documentation/admin-guide/tainted-kernels.rst
@@ -100,6 +100,7 @@ Bit  Log  Number  Reason that got the kernel tainted
  15  _/K   32768  kernel has been live patched
  16  _/X   65536  auxiliary taint, defined for and used by distros
  17  _/T  131072  kernel was built with the struct randomization plugin
+ 18  _/N  262144  an in-kernel test (such as a KUnit test) has been run
 ===  ===  ======  ========================================================
 
 Note: The character ``_`` is representing a blank in this table to make reading
diff --git a/include/linux/panic.h b/include/linux/panic.h
index f5844908a089..2f5f2a9ecaf7 100644
--- a/include/linux/panic.h
+++ b/include/linux/panic.h
@@ -74,7 +74,8 @@ static inline void set_arch_panic_timeout(int timeout, int arch_default_timeout)
 #define TAINT_LIVEPATCH			15
 #define TAINT_AUX			16
 #define TAINT_RANDSTRUCT		17
-#define TAINT_FLAGS_COUNT		18
+#define TAINT_TEST			18
+#define TAINT_FLAGS_COUNT		19
 #define TAINT_FLAGS_MAX			((1UL << TAINT_FLAGS_COUNT) - 1)
 
 struct taint_flag {
diff --git a/kernel/panic.c b/kernel/panic.c
index eb4dfb932c85..1cf707e3bacd 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -404,6 +404,7 @@ const struct taint_flag taint_flags[TAINT_FLAGS_COUNT] = {
 	[ TAINT_LIVEPATCH ]		= { 'K', ' ', true },
 	[ TAINT_AUX ]			= { 'X', ' ', true },
 	[ TAINT_RANDSTRUCT ]		= { 'T', ' ', true },
+	[ TAINT_TEST ]			= { 'N', ' ', true },
 };
 
 /**
-- 
2.36.0.550.gb090851708-goog

[PATCH v3 2/3] kunit: Taint the kernel when KUnit tests are run

[David Gow]

Make KUnit trigger the new TAINT_TEST taint when any KUnit test is run.
Due to KUnit tests not being intended to run on production systems, and
potentially causing problems (or security issues like leaking kernel
addresses), the kernel's state should not be considered safe for
production use after KUnit tests are run.

Signed-off-by: David Gow <davidgow@google.com>
---
 lib/kunit/test.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 0f66c13d126e..2b808117bd4a 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -11,6 +11,7 @@
 #include <kunit/test-bug.h>
 #include <linux/kernel.h>
 #include <linux/moduleparam.h>
+#include <linux/panic.h>
 #include <linux/sched/debug.h>
 #include <linux/sched.h>
 
@@ -498,6 +499,9 @@ int kunit_run_tests(struct kunit_suite *suite)
 	struct kunit_result_stats suite_stats = { 0 };
 	struct kunit_result_stats total_stats = { 0 };
 
+	/* Taint the kernel so we know we've run tests. */
+	add_taint(TAINT_TEST, LOCKDEP_STILL_OK);
+
 	kunit_print_subtest_start(suite);
 
 	kunit_suite_for_each_test_case(suite, test_case) {
-- 
2.36.0.550.gb090851708-goog

[PATCH v3 3/3] selftest: Taint kernel when test module loaded

[David Gow]

Make any kselftest test module (using the kselftest_module framework)
taint the kernel with TAINT_TEST on module load.

Note that several selftests use kernel modules which are not based on
the kselftest_module framework, and so will not automatically taint the
kernel. These modules will have to be manually modified if they should
taint the kernel this way.

Similarly, selftests which do not load modules into the kernel generally
should not taint the kernel (or possibly should only do so on failure),
as it's assumed that testing from user-space should be safe. Regardless,
they can write to /proc/sys/kernel/tainted if required.

Signed-off-by: David Gow <davidgow@google.com>
---
 tools/testing/selftests/kselftest_module.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/testing/selftests/kselftest_module.h b/tools/testing/selftests/kselftest_module.h
index e2ea41de3f35..226e616b82e0 100644
--- a/tools/testing/selftests/kselftest_module.h
+++ b/tools/testing/selftests/kselftest_module.h
@@ -3,6 +3,7 @@
 #define __KSELFTEST_MODULE_H
 
 #include <linux/module.h>
+#include <linux/panic.h>
 
 /*
  * Test framework for writing test modules to be loaded by kselftest.
@@ -41,6 +42,7 @@ static inline int kstm_report(unsigned int total_tests, unsigned int failed_test
 static int __init __module##_init(void)			\
 {							\
 	pr_info("loaded.\n");				\
+	add_taint(TAINT_KUNIT, LOCKDEP_STILL_OK);	\
 	selftest();					\
 	return kstm_report(total_tests, failed_tests, skipped_tests);	\
 }							\
-- 
2.36.0.550.gb090851708-goog

Re: [PATCH v3 1/3] panic: Taint kernel if tests are run

[Luis Chamberlain]

On Fri, May 13, 2022 at 04:32:11PM +0800, David Gow wrote:
> Most in-kernel tests (such as KUnit tests) are not supposed to run on
> production systems: they may do deliberately illegal things to trigger
> errors, and have security implications (for example, KUnit assertions
> will often deliberately leak kernel addresses).
> 
> Add a new taint type, TAINT_TEST to signal that a test has been run.
> This will be printed as 'N' (originally for kuNit, as every other
> sensible letter was taken.)
> 
> This should discourage people from running these tests on production
> systems, and to make it easier to tell if tests have been run
> accidentally (by loading the wrong configuration, etc.)
> 
> Signed-off-by: David Gow <davidgow@google.com>
> ---
> 
> Updated this to handle the most common case of selftest modules, in
> addition to KUnit tests. There's room for other tests or test frameworks
> to use this as well, either with a call to add_taint() from within the
> kernel, or by writing to /proc/sys/kernel/tainted.
> 
> The 'N' character for the taint is even less useful now that it's no
> longer short for kuNit, but all the letters in TEST are taken. :-(
> 
> Changes since v2:
> https://lore.kernel.org/linux-kselftest/20220430030019.803481-1-davidgow@google.com/
> - Rename TAINT_KUNIT -> TAINT_TEST.
> - Split into separate patches for adding the taint, and triggering it.
> - Taint on a kselftest_module being loaded (patch 3/3)
> 
> Changes since v1:
> https://lore.kernel.org/linux-kselftest/20220429043913.626647-1-davidgow@google.com/
> - Make the taint per-module, to handle the case when tests are in
>   (longer lasting) modules. (Thanks Greg KH).
> 
> Note that this still has checkpatch.pl warnings around bracket
> placement, which are intentional as part of matching the surrounding
> code.
> 
> ---
>  Documentation/admin-guide/tainted-kernels.rst | 1 +
>  include/linux/panic.h                         | 3 ++-
>  kernel/panic.c                                | 1 +
>  3 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/admin-guide/tainted-kernels.rst b/Documentation/admin-guide/tainted-kernels.rst
> index ceeed7b0798d..546f3071940d 100644
> --- a/Documentation/admin-guide/tainted-kernels.rst
> +++ b/Documentation/admin-guide/tainted-kernels.rst
> @@ -100,6 +100,7 @@ Bit  Log  Number  Reason that got the kernel tainted
>   15  _/K   32768  kernel has been live patched
>   16  _/X   65536  auxiliary taint, defined for and used by distros
>   17  _/T  131072  kernel was built with the struct randomization plugin
> + 18  _/N  262144  an in-kernel test (such as a KUnit test) has been run

I think mentioning just kunit fuzzes its interpretation here.
Best to keep that out.

Other than that:

Acked-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis

Re: [PATCH v3 2/3] kunit: Taint the kernel when KUnit tests are run

[Luis Chamberlain]

On Fri, May 13, 2022 at 04:32:12PM +0800, David Gow wrote:
> Make KUnit trigger the new TAINT_TEST taint when any KUnit test is run.
> Due to KUnit tests not being intended to run on production systems, and
> potentially causing problems (or security issues like leaking kernel
> addresses), the kernel's state should not be considered safe for
> production use after KUnit tests are run.
> 
> Signed-off-by: David Gow <davidgow@google.com>

Acked-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis

Re: [PATCH v3 3/3] selftest: Taint kernel when test module loaded

[Luis Chamberlain]

On Fri, May 13, 2022 at 04:32:13PM +0800, David Gow wrote:
> Make any kselftest test module (using the kselftest_module framework)
> taint the kernel with TAINT_TEST on module load.
> 
> Note that several selftests use kernel modules which are not based on
> the kselftest_module framework, and so will not automatically taint the
> kernel. These modules will have to be manually modified if they should
> taint the kernel this way.
> 
> Similarly, selftests which do not load modules into the kernel generally
> should not taint the kernel (or possibly should only do so on failure),
> as it's assumed that testing from user-space should be safe. Regardless,
> they can write to /proc/sys/kernel/tainted if required.
> 
> Signed-off-by: David Gow <davidgow@google.com>

Not all selftest modules use KSTM_MODULE_LOADERS() so I'd like to see a
modpost target as well, otherwise this just covers a sliver of
selftests.

  Luis

Re: [PATCH v3 2/3] kunit: Taint the kernel when KUnit tests are run

[Daniel Latypov]

On Fri, May 13, 2022 at 1:32 AM David Gow <davidgow@google.com> wrote:
>
> Make KUnit trigger the new TAINT_TEST taint when any KUnit test is run.
> Due to KUnit tests not being intended to run on production systems, and
> potentially causing problems (or security issues like leaking kernel
> addresses), the kernel's state should not be considered safe for
> production use after KUnit tests are run.
>
> Signed-off-by: David Gow <davidgow@google.com>

Tested-by: Daniel Latypov <dlatypov@google.com>

Looks good to me.

There's an edge case where we might have 0 suites or 0 tests and we
still taint the kernel, but I don't think we need to deal with that.
At the start of kunit_run_tests() is the cleanest place to do this.

I wasn't quite sure where this applied, but I manually applied the changes here.
Without this patch, this command exits fine:
$ ./tools/testing/kunit/kunit.py run --kernel_args=panic_on_taint=0x40000

With it, I get
[12:03:31] Kernel panic - not syncing: panic_on_taint set ...
[12:03:31] CPU: 0 PID: 1 Comm: swapper Tainted: G                 N
5.17.0-00001-gea9ee5e7aed8-dirty #60

I'm a bit surprised that it prints 'G' and not 'N', but this does seem
to be the right mask
$ python3 -c 'print(hex(1<<18))'
0x40000
and it only takes effect when this patch is applied.
I'll chalk that up to my ignorance of how taint works.

Re: [PATCH v3 2/3] kunit: Taint the kernel when KUnit tests are run

[David Gow]

On Sat, May 14, 2022 at 3:09 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> On Fri, May 13, 2022 at 1:32 AM David Gow <davidgow@google.com> wrote:
> >
> > Make KUnit trigger the new TAINT_TEST taint when any KUnit test is run.
> > Due to KUnit tests not being intended to run on production systems, and
> > potentially causing problems (or security issues like leaking kernel
> > addresses), the kernel's state should not be considered safe for
> > production use after KUnit tests are run.
> >
> > Signed-off-by: David Gow <davidgow@google.com>
>
> Tested-by: Daniel Latypov <dlatypov@google.com>
>
> Looks good to me.
>
> There's an edge case where we might have 0 suites or 0 tests and we
> still taint the kernel, but I don't think we need to deal with that.
> At the start of kunit_run_tests() is the cleanest place to do this.

Hmm... thinking about it, I think it might be worth not tainting if 0
suites run, but tainting if 0 tests run.

If we taint even if there are no suites present, that'll make things
awkward for the "build KUnit in, but not any tests" case: the kernel
would be tainted regardless. Given Android might be having the KUnit
execution stuff built-in (but using modules for tests), it's probably
worth not tainting there. (Though I think they have a separate way of
disabling KUnit as well, so it's probably not a complete
deal-breaker).

The case of having suites but no tests should still taint the kernel,
as suite_init functions could still run.

Assuming that seems sensible, I'll send out a v4 with that changed.

> I wasn't quite sure where this applied, but I manually applied the changes here.
> Without this patch, this command exits fine:
> $ ./tools/testing/kunit/kunit.py run --kernel_args=panic_on_taint=0x40000
>
> With it, I get
> [12:03:31] Kernel panic - not syncing: panic_on_taint set ...
> [12:03:31] CPU: 0 PID: 1 Comm: swapper Tainted: G                 N

This is showing both 'G' and 'N' ('G' being the character for GPL --
i.e. the kernel is not tainted by proprietary modules: 'P').

Jani did suggest a better way of printing these in the v1 discussion
(printing the actual names of taints present), which I might do in a
follow-up.

> 5.17.0-00001-gea9ee5e7aed8-dirty #60
>
> I'm a bit surprised that it prints 'G' and not 'N', but this does seem
> to be the right mask
> $ python3 -c 'print(hex(1<<18))'
> 0x40000
> and it only takes effect when this patch is applied.
> I'll chalk that up to my ignorance of how taint works.

-- David

Re: [PATCH v3 3/3] selftest: Taint kernel when test module loaded

[David Gow]

On Fri, May 13, 2022 at 11:38 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Fri, May 13, 2022 at 04:32:13PM +0800, David Gow wrote:
> > Make any kselftest test module (using the kselftest_module framework)
> > taint the kernel with TAINT_TEST on module load.
> >
> > Note that several selftests use kernel modules which are not based on
> > the kselftest_module framework, and so will not automatically taint the
> > kernel. These modules will have to be manually modified if they should
> > taint the kernel this way.
> >
> > Similarly, selftests which do not load modules into the kernel generally
> > should not taint the kernel (or possibly should only do so on failure),
> > as it's assumed that testing from user-space should be safe. Regardless,
> > they can write to /proc/sys/kernel/tainted if required.
> >
> > Signed-off-by: David Gow <davidgow@google.com>
>
> Not all selftest modules use KSTM_MODULE_LOADERS() so I'd like to see a
> modpost target as well, otherwise this just covers a sliver of
> selftests.
>

My personal feeling is that the ideal way of solving this is actually
to port those modules which aren't using KSTM_MODULE_LOADERS() (or
KUnit, or some other system) to do so, or to otherwise manually tag
them as selftests and/or make them taint the kernel.

That being said, we can gain a bit my making the module-loading
helpers in kselftest/module.sh manually taint the kernel with
/proc/sys/kernel/tainted, which will catch quite a few of them (even
if tainting from userspace before they're loaded is suboptimal).

I've also started experimenting with a "test" MODULE_INFO field, which
modpost would add with the -t option. That still requires sprinkling
MODULE_INFO() everwhere, or the '-t' option to a bunch of makefiles,
or doing something more drastic to set it automatically for modules in
a given directory / makefile. Or the staging thing of checking the
directory / prefix in modpost.

I'll play around some more and have something to show in v4. (If we
have a MODULE_INFO field, we should use it for KUnit modules, but we'd
still have to taint the kernel manually for built-in tests anyway, so
it'd be redundant...)

-- David

Re: [PATCH v3 2/3] kunit: Taint the kernel when KUnit tests are run

[Daniel Latypov]

On Fri, May 13, 2022 at 8:04 PM David Gow <davidgow@google.com> wrote:
> Hmm... thinking about it, I think it might be worth not tainting if 0
> suites run, but tainting if 0 tests run.
>
> If we taint even if there are no suites present, that'll make things
> awkward for the "build KUnit in, but not any tests" case: the kernel
> would be tainted regardless. Given Android might be having the KUnit

Actually, this is what the code does right now. I was wrong.
If there are 0 suites => not tainted.
If there are 0 tests in the suites => tainted.

For kunit being built in, it first goes through this func
   206  static void kunit_exec_run_tests(struct suite_set *suite_set)
   207  {
   208          struct kunit_suite * const * const *suites;
   209
   210          kunit_print_tap_header(suite_set);
   211
   212          for (suites = suite_set->start; suites <
suite_set->end; suites++)
   213                  __kunit_test_suites_init(*suites);
   214  }

So for the "build KUnit in, but not any tests" case, you'll never
enter that for-loop.
Thus you'll never call __kunit_test_suites_init() => kunit_run_tests().

For module-based tests, we have the same behavior.
If there's 0 test suites, we never enter the second loop, so we never taint.
But if there's >0 suites, then we will, regardless of the # of test cases.

   570  int __kunit_test_suites_init(struct kunit_suite * const * const suites)
   571  {
   572          unsigned int i;
   573
   574          for (i = 0; suites[i] != NULL; i++) {
   575                  kunit_init_suite(suites[i]);
   576                  kunit_run_tests(suites[i]);
   577          }
   578          return 0;
   579  }

So this change should already work as intended.

> execution stuff built-in (but using modules for tests), it's probably
> worth not tainting there. (Though I think they have a separate way of
> disabling KUnit as well, so it's probably not a complete
> deal-breaker).
>
> The case of having suites but no tests should still taint the kernel,
> as suite_init functions could still run.

Yes, suite_init functions are the concern. I agree we should taint if
there are >0 suites but 0 test cases.

I don't think it's worth trying to be fancy and tainting iff there >0
test cases or a suite_init/exit function ran.

>
> Assuming that seems sensible, I'll send out a v4 with that changed.

Just to be clear: that shouldn't be necessary.

>
> > I wasn't quite sure where this applied, but I manually applied the changes here.
> > Without this patch, this command exits fine:
> > $ ./tools/testing/kunit/kunit.py run --kernel_args=panic_on_taint=0x40000
> >
> > With it, I get
> > [12:03:31] Kernel panic - not syncing: panic_on_taint set ...
> > [12:03:31] CPU: 0 PID: 1 Comm: swapper Tainted: G                 N
>
> This is showing both 'G' and 'N' ('G' being the character for GPL --

I just somehow missed the fact there was an 'N' at the end there.
Thanks, I thought I was going crazy. I guess I was just going blind.


Daniel

Re: [PATCH v3 1/3] panic: Taint kernel if tests are run

[Brendan Higgins]

On Fri, May 13, 2022 at 4:32 AM David Gow <davidgow@google.com> wrote:
>
> Most in-kernel tests (such as KUnit tests) are not supposed to run on
> production systems: they may do deliberately illegal things to trigger
> errors, and have security implications (for example, KUnit assertions
> will often deliberately leak kernel addresses).
>
> Add a new taint type, TAINT_TEST to signal that a test has been run.
> This will be printed as 'N' (originally for kuNit, as every other
> sensible letter was taken.)
>
> This should discourage people from running these tests on production
> systems, and to make it easier to tell if tests have been run
> accidentally (by loading the wrong configuration, etc.)
>
> Signed-off-by: David Gow <davidgow@google.com>

Aside from Luis' comment (which I agree with), this looks good. I am
not an expert on the taint mechanism, but this seems pretty
straightforward.

Reviewed-by: Brendan Higgins <brendanhiggins@google.com>

Re: [PATCH v3 2/3] kunit: Taint the kernel when KUnit tests are run

[Brendan Higgins]

On Sat, May 14, 2022 at 3:25 PM Daniel Latypov <dlatypov@google.com> wrote:
>
> On Fri, May 13, 2022 at 8:04 PM David Gow <davidgow@google.com> wrote:
> > Hmm... thinking about it, I think it might be worth not tainting if 0
> > suites run, but tainting if 0 tests run.
> >
> > If we taint even if there are no suites present, that'll make things
> > awkward for the "build KUnit in, but not any tests" case: the kernel
> > would be tainted regardless. Given Android might be having the KUnit
>
> Actually, this is what the code does right now. I was wrong.
> If there are 0 suites => not tainted.
> If there are 0 tests in the suites => tainted.
>
> For kunit being built in, it first goes through this func
>    206  static void kunit_exec_run_tests(struct suite_set *suite_set)
>    207  {
>    208          struct kunit_suite * const * const *suites;
>    209
>    210          kunit_print_tap_header(suite_set);
>    211
>    212          for (suites = suite_set->start; suites <
> suite_set->end; suites++)
>    213                  __kunit_test_suites_init(*suites);
>    214  }
>
> So for the "build KUnit in, but not any tests" case, you'll never
> enter that for-loop.
> Thus you'll never call __kunit_test_suites_init() => kunit_run_tests().
>
> For module-based tests, we have the same behavior.
> If there's 0 test suites, we never enter the second loop, so we never taint.
> But if there's >0 suites, then we will, regardless of the # of test cases.
>
>    570  int __kunit_test_suites_init(struct kunit_suite * const * const suites)
>    571  {
>    572          unsigned int i;
>    573
>    574          for (i = 0; suites[i] != NULL; i++) {
>    575                  kunit_init_suite(suites[i]);
>    576                  kunit_run_tests(suites[i]);
>    577          }
>    578          return 0;
>    579  }
>
> So this change should already work as intended.
>
> > execution stuff built-in (but using modules for tests), it's probably
> > worth not tainting there. (Though I think they have a separate way of
> > disabling KUnit as well, so it's probably not a complete
> > deal-breaker).
> >
> > The case of having suites but no tests should still taint the kernel,
> > as suite_init functions could still run.
>
> Yes, suite_init functions are the concern. I agree we should taint if
> there are >0 suites but 0 test cases.
>
> I don't think it's worth trying to be fancy and tainting iff there >0
> test cases or a suite_init/exit function ran.
>
> >
> > Assuming that seems sensible, I'll send out a v4 with that changed.
>
> Just to be clear: that shouldn't be necessary.

Agreed. I think the current behavior is acceptable, and should be
unobtrusive to Android: Joe has a patch that introduces a kernel param
which disables running KUnit tests at the suite level which would
happen before this taint occurs. So the only way the taint happens is
if we actually try to execute some test cases (whether or not the
cases actually run).

> > > I wasn't quite sure where this applied, but I manually applied the changes here.
> > > Without this patch, this command exits fine:
> > > $ ./tools/testing/kunit/kunit.py run --kernel_args=panic_on_taint=0x40000
> > >
> > > With it, I get
> > > [12:03:31] Kernel panic - not syncing: panic_on_taint set ...
> > > [12:03:31] CPU: 0 PID: 1 Comm: swapper Tainted: G                 N
> >
> > This is showing both 'G' and 'N' ('G' being the character for GPL --
>
> I just somehow missed the fact there was an 'N' at the end there.
> Thanks, I thought I was going crazy. I guess I was just going blind.
>
>
> Daniel

Re: [PATCH v3 2/3] kunit: Taint the kernel when KUnit tests are run

[Brendan Higgins]

On Fri, May 13, 2022 at 4:32 AM David Gow <davidgow@google.com> wrote:
>
> Make KUnit trigger the new TAINT_TEST taint when any KUnit test is run.
> Due to KUnit tests not being intended to run on production systems, and
> potentially causing problems (or security issues like leaking kernel
> addresses), the kernel's state should not be considered safe for
> production use after KUnit tests are run.
>
> Signed-off-by: David Gow <davidgow@google.com>

Reviewed-by: Brendan Higgins <brendanhiggins@google.com>