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@xxxxxxxxxxxxxx>
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@xxxxxxxxxxxx>
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
