linux-kernel - Re: [PATCH 2/2] kernel: add sysctl kernel.nr

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]

Message-ID: <201911300823.69EAF975E9@keescook>
Date:   Sat, 30 Nov 2019 08:33:44 -0800
From:   Kees Cook <keescook@...omium.org>
To:     Konstantin Khlebnikov <khlebnikov@...dex-team.ru>
Cc:     linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
        Sasha Levin <sashal@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        rcu@...r.kernel.org, Tejun Heo <tj@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH 2/2] kernel: add sysctl kernel.nr_taints

On Fri, Nov 29, 2019 at 04:21:48PM +0300, Konstantin Khlebnikov wrote:
> Once taint flag is set it's never cleared. Following taint could be detected
> only via parsing kernel log messages which are different for each occasion.
> For repeatable taints like TAINT_MACHINE_CHECK, TAINT_BAD_PAGE, TAINT_DIE,
> TAINT_WARN, TAINT_LOCKUP it would be good to know count to see their rate.
> 
> This patch adds sysctl with vector of counters. One for each taint flag.
> Counters are non-atomic in favor of simplicity. Exact count doesn't matter.
> Writing vector of zeroes resets counters.
> 
> This is useful for detecting frequent problems with automatic monitoring.
> 
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@...dex-team.ru>

I like this, yeah. This would let LKDTM users reset taint counts to
re-check the same kernel, etc, without explicitly clearing the taint
flags which always seemed like a bad idea. :)

Reviewed-by: Kees Cook <keescook@...omium.org>

One nit below...

> ---
>  include/linux/kernel.h |    1 +
>  kernel/panic.c         |    2 ++
>  kernel/sysctl.c        |    9 +++++++++
>  3 files changed, 12 insertions(+)
> 
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index e8a6808e4f2f..900d02167bbd 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -604,6 +604,7 @@ struct taint_flag {
>  };
>  
>  extern const struct taint_flag taint_flags[TAINT_FLAGS_COUNT];
> +extern int sysctl_nr_taints[TAINT_FLAGS_COUNT];
>  
>  extern const char hex_asc[];
>  #define hex_asc_lo(x)	hex_asc[((x) & 0x0f)]
> diff --git a/kernel/panic.c b/kernel/panic.c
> index d7750a45ca8d..a3df00ebcba2 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -39,6 +39,7 @@
>  int panic_on_oops = CONFIG_PANIC_ON_OOPS_VALUE;
>  static unsigned long tainted_mask =
>  	IS_ENABLED(CONFIG_GCC_PLUGIN_RANDSTRUCT) ? (1 << TAINT_RANDSTRUCT) : 0;
> +int sysctl_nr_taints[TAINT_FLAGS_COUNT];
>  static int pause_on_oops;
>  static int pause_on_oops_flag;
>  static DEFINE_SPINLOCK(pause_on_oops_lock);
> @@ -434,6 +435,7 @@ void add_taint(unsigned flag, enum lockdep_ok lockdep_ok)
>  		pr_warn("Disabling lock debugging due to kernel taint\n");
>  
>  	set_bit(flag, &tainted_mask);
> +	sysctl_nr_taints[flag]++;

As long as we're changing this code, how about adding an explicit check
of "flag" against either ARRAY_SIZE(sysctl_nr_tains) or TAINT_FLAGS_COUNT?

It looks like only 1 caller isn't using a static value, though:
proc_taint(), so it would catch "overflows" there (it's already bounded
to the size of tainted_mask, but proc could set "unknown" taint flags).

-Kees

>  }
>  EXPORT_SYMBOL(add_taint);
>  
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index b6f2f35d0bcf..5d9727556cef 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -553,6 +553,15 @@ static struct ctl_table kern_table[] = {
>  		.mode		= 0644,
>  		.proc_handler	= proc_taint,
>  	},
> +	{
> +		.procname	= "nr_taints",
> +		.data		= &sysctl_nr_taints,
> +		.maxlen		= sizeof(sysctl_nr_taints),
> +		.mode		= 0644,
> +		.proc_handler	= proc_dointvec_minmax,
> +		.extra1		= SYSCTL_ZERO,
> +		.extra2		= SYSCTL_ZERO,
> +	},
>  	{
>  		.procname	= "sysctl_writes_strict",
>  		.data		= &sysctl_writes_strict,
> 

-- 
Kees Cook