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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 29 Apr 2022 09:09:08 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     David Gow <davidgow@...gle.com>
Cc:     Brendan Higgins <brendanhiggins@...gle.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Jonathan Corbet <corbet@....net>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Kees Cook <keescook@...omium.org>,
        Shuah Khan <skhan@...uxfoundation.org>,
        "Guilherme G . Piccoli" <gpiccoli@...lia.com>,
        Luis Chamberlain <mcgrof@...nel.org>,
        Sebastian Reichel <sre@...nel.org>,
        John Ogness <john.ogness@...utronix.de>,
        Joe Fradley <joefradley@...gle.com>,
        Daniel Latypov <dlatypov@...gle.com>,
        kunit-dev@...glegroups.com, linux-kselftest@...r.kernel.org,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] kunit: Taint kernel if any tests run

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@...gle.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