[PATCH v3] kernel: add panic_on_taint

[PATCH v3] kernel: add panic_on_taint

[Rafael Aquini]

Analogously to the introduction of panic_on_warn, this patch
introduces a kernel option named panic_on_taint in order to
provide a simple and generic way to stop execution and catch
a coredump when the kernel gets tainted by any given taint flag.

This is useful for debugging sessions as it avoids rebuilding
the kernel to explicitly add calls to panic() or BUG() into
code sites that introduce the taint flags of interest.
Another, perhaps less frequent, use for this option would be
as a mean for assuring a security policy (in paranoid mode)
case where no single taint is allowed for the running system.

Suggested-by: Qian Cai <cai@lca.pw>
Signed-off-by: Rafael Aquini <aquini@redhat.com>
---
Changelog:
* v2: get rid of unnecessary/misguided compiler hints		(Luis)
* v2: enhance documentation text for the new kernel parameter	(Randy)
* v3: drop sysctl interface, keep it only as a kernel parameter (Luis)

 Documentation/admin-guide/kdump/kdump.rst     | 10 +++++
 .../admin-guide/kernel-parameters.txt         | 15 +++++++
 include/linux/kernel.h                        |  2 +
 kernel/panic.c                                | 40 +++++++++++++++++++
 kernel/sysctl.c                               |  9 ++++-
 5 files changed, 75 insertions(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/kdump/kdump.rst b/Documentation/admin-guide/kdump/kdump.rst
index ac7e131d2935..de3cf6d377cc 100644
--- a/Documentation/admin-guide/kdump/kdump.rst
+++ b/Documentation/admin-guide/kdump/kdump.rst
@@ -521,6 +521,16 @@ will cause a kdump to occur at the panic() call.  In cases where a user wants
 to specify this during runtime, /proc/sys/kernel/panic_on_warn can be set to 1
 to achieve the same behaviour.
 
+Trigger Kdump on add_taint()
+============================
+
+The kernel parameter, panic_on_taint, calls panic() from within add_taint(),
+whenever the value set in this bitmask matches with the bit flag being set
+by add_taint(). This will cause a kdump to occur at the panic() call.
+In cases where a user wants to specify this during runtime,
+/proc/sys/kernel/panic_on_taint can be set to a respective bitmask value
+to achieve the same behaviour.
+
 Contact
 =======
 
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 7bc83f3d9bdf..4a69fe49a70d 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3404,6 +3404,21 @@
 	panic_on_warn	panic() instead of WARN().  Useful to cause kdump
 			on a WARN().
 
+	panic_on_taint=	[KNL] conditionally panic() in add_taint()
+			Format: <str>
+			Specifies, as a string, the TAINT flag set that will
+			compose a bitmask for calling panic() when the kernel
+			gets tainted.
+			See Documentation/admin-guide/tainted-kernels.rst for
+			details on the taint flags that users can pick to
+			compose the bitmask to assign to panic_on_taint.
+			When the string is prefixed with a '-' the bitmask
+			set in panic_on_taint will be mutually exclusive
+			with the sysctl knob kernel.tainted, and any attempt
+			to write to that sysctl will fail with -EINVAL for
+			any taint value that masks with the flags set for
+			this option.
+
 	crash_kexec_post_notifiers
 			Run kdump after running panic-notifiers and dumping
 			kmsg. This only for the users who doubt kdump always
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 9b7a8d74a9d6..66bc102cb59a 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -528,6 +528,8 @@ extern int panic_on_oops;
 extern int panic_on_unrecovered_nmi;
 extern int panic_on_io_nmi;
 extern int panic_on_warn;
+extern unsigned long panic_on_taint;
+extern bool panic_on_taint_exclusive;
 extern int sysctl_panic_on_rcu_stall;
 extern int sysctl_panic_on_stackoverflow;
 
diff --git a/kernel/panic.c b/kernel/panic.c
index b69ee9e76cb2..65c62f8a1de8 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -25,6 +25,7 @@
 #include <linux/kexec.h>
 #include <linux/sched.h>
 #include <linux/sysrq.h>
+#include <linux/ctype.h>
 #include <linux/init.h>
 #include <linux/nmi.h>
 #include <linux/console.h>
@@ -44,6 +45,8 @@ static int pause_on_oops_flag;
 static DEFINE_SPINLOCK(pause_on_oops_lock);
 bool crash_kexec_post_notifiers;
 int panic_on_warn __read_mostly;
+unsigned long panic_on_taint;
+bool panic_on_taint_exclusive = false;
 
 int panic_timeout = CONFIG_PANIC_TIMEOUT;
 EXPORT_SYMBOL_GPL(panic_timeout);
@@ -434,6 +437,11 @@ 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);
+
+	if (tainted_mask & panic_on_taint) {
+		panic_on_taint = 0;
+		panic("panic_on_taint set ...");
+	}
 }
 EXPORT_SYMBOL(add_taint);
 
@@ -686,3 +694,35 @@ static int __init oops_setup(char *s)
 	return 0;
 }
 early_param("oops", oops_setup);
+
+static int __init panic_on_taint_setup(char *s)
+{
+	/* we just ignore panic_on_taint if passed without flags */
+	if (!s)
+		goto out;
+
+	for (; *s; s++) {
+		int i;
+
+		if (*s == '-') {
+			panic_on_taint_exclusive = true;
+			continue;
+		}
+
+		for (i = 0; i < TAINT_FLAGS_COUNT; i++) {
+			if (toupper(*s) == taint_flags[i].c_true) {
+				set_bit(i, &panic_on_taint);
+				break;
+			}
+		}
+	}
+
+	/* unset exclusive mode if no taint flags were passed on */
+	if (panic_on_taint_exclusive &&
+	    !(panic_on_taint & ((1UL << TAINT_FLAGS_COUNT) - 1)))
+		panic_on_taint_exclusive = false;
+
+out:
+	return 0;
+}
+early_param("panic_on_taint", panic_on_taint_setup);
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 8a176d8727a3..d361ec0420f6 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2623,11 +2623,18 @@ static int proc_taint(struct ctl_table *table, int write,
 		return err;
 
 	if (write) {
+		int i;
+		/*
+		 * If we are relying on panic_on_taint not producing
+		 * false positives due to userland input, bail out
+		 * before setting the requested taint flags.
+		 */
+		if (panic_on_taint_exclusive && (tmptaint & panic_on_taint))
+			return -EINVAL;
 		/*
 		 * Poor man's atomic or. Not worth adding a primitive
 		 * to everyone's atomic.h for this
 		 */
-		int i;
 		for (i = 0; i < BITS_PER_LONG && tmptaint >> i; i++) {
 			if ((tmptaint >> i) & 1)
 				add_taint(i, LOCKDEP_STILL_OK);
-- 
2.25.4

Re: [PATCH v3] kernel: add panic_on_taint

[Kees Cook]

On Sat, May 09, 2020 at 09:57:37AM -0400, Rafael Aquini wrote:
> Analogously to the introduction of panic_on_warn, this patch
> introduces a kernel option named panic_on_taint in order to
> provide a simple and generic way to stop execution and catch
> a coredump when the kernel gets tainted by any given taint flag.
> 
> This is useful for debugging sessions as it avoids rebuilding
> the kernel to explicitly add calls to panic() or BUG() into
> code sites that introduce the taint flags of interest.
> Another, perhaps less frequent, use for this option would be
> as a mean for assuring a security policy (in paranoid mode)
> case where no single taint is allowed for the running system.
> 
> Suggested-by: Qian Cai <cai@lca.pw>
> Signed-off-by: Rafael Aquini <aquini@redhat.com>

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

-- 
Kees Cook

Re: [PATCH v3] kernel: add panic_on_taint

[Baoquan He]

On 05/09/20 at 09:57am, Rafael Aquini wrote:
> Analogously to the introduction of panic_on_warn, this patch
> introduces a kernel option named panic_on_taint in order to
> provide a simple and generic way to stop execution and catch
> a coredump when the kernel gets tainted by any given taint flag.
> 
> This is useful for debugging sessions as it avoids rebuilding
> the kernel to explicitly add calls to panic() or BUG() into
> code sites that introduce the taint flags of interest.
> Another, perhaps less frequent, use for this option would be
> as a mean for assuring a security policy (in paranoid mode)
> case where no single taint is allowed for the running system.
> 
> Suggested-by: Qian Cai <cai@lca.pw>
> Signed-off-by: Rafael Aquini <aquini@redhat.com>
> ---
> Changelog:
> * v2: get rid of unnecessary/misguided compiler hints		(Luis)
> * v2: enhance documentation text for the new kernel parameter	(Randy)
> * v3: drop sysctl interface, keep it only as a kernel parameter (Luis)
> 
>  Documentation/admin-guide/kdump/kdump.rst     | 10 +++++
>  .../admin-guide/kernel-parameters.txt         | 15 +++++++
>  include/linux/kernel.h                        |  2 +
>  kernel/panic.c                                | 40 +++++++++++++++++++
>  kernel/sysctl.c                               |  9 ++++-
>  5 files changed, 75 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/admin-guide/kdump/kdump.rst b/Documentation/admin-guide/kdump/kdump.rst
> index ac7e131d2935..de3cf6d377cc 100644
> --- a/Documentation/admin-guide/kdump/kdump.rst
> +++ b/Documentation/admin-guide/kdump/kdump.rst
> @@ -521,6 +521,16 @@ will cause a kdump to occur at the panic() call.  In cases where a user wants
>  to specify this during runtime, /proc/sys/kernel/panic_on_warn can be set to 1
>  to achieve the same behaviour.
>  
> +Trigger Kdump on add_taint()
> +============================
> +
> +The kernel parameter, panic_on_taint, calls panic() from within add_taint(),
> +whenever the value set in this bitmask matches with the bit flag being set
> +by add_taint(). This will cause a kdump to occur at the panic() call.
> +In cases where a user wants to specify this during runtime,
> +/proc/sys/kernel/panic_on_taint can be set to a respective bitmask value
> +to achieve the same behaviour.
> +
>  Contact
>  =======
>  
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 7bc83f3d9bdf..4a69fe49a70d 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -3404,6 +3404,21 @@
>  	panic_on_warn	panic() instead of WARN().  Useful to cause kdump
>  			on a WARN().
>  
> +	panic_on_taint=	[KNL] conditionally panic() in add_taint()
> +			Format: <str>
			Changed it as 'Format: <string>' to be
consistent with the existing other options?
> +			Specifies, as a string, the TAINT flag set that will
> +			compose a bitmask for calling panic() when the kernel
> +			gets tainted.
> +			See Documentation/admin-guide/tainted-kernels.rst for
> +			details on the taint flags that users can pick to
> +			compose the bitmask to assign to panic_on_taint.
> +			When the string is prefixed with a '-' the bitmask
> +			set in panic_on_taint will be mutually exclusive
> +			with the sysctl knob kernel.tainted, and any attempt
> +			to write to that sysctl will fail with -EINVAL for
> +			any taint value that masks with the flags set for
> +			this option.
> +
>  	crash_kexec_post_notifiers
>  			Run kdump after running panic-notifiers and dumping
>  			kmsg. This only for the users who doubt kdump always
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 9b7a8d74a9d6..66bc102cb59a 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -528,6 +528,8 @@ extern int panic_on_oops;
>  extern int panic_on_unrecovered_nmi;
>  extern int panic_on_io_nmi;
>  extern int panic_on_warn;
> +extern unsigned long panic_on_taint;
> +extern bool panic_on_taint_exclusive;
>  extern int sysctl_panic_on_rcu_stall;
>  extern int sysctl_panic_on_stackoverflow;
>  
> diff --git a/kernel/panic.c b/kernel/panic.c
> index b69ee9e76cb2..65c62f8a1de8 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -25,6 +25,7 @@
>  #include <linux/kexec.h>
>  #include <linux/sched.h>
>  #include <linux/sysrq.h>
> +#include <linux/ctype.h>
>  #include <linux/init.h>
>  #include <linux/nmi.h>
>  #include <linux/console.h>
> @@ -44,6 +45,8 @@ static int pause_on_oops_flag;
>  static DEFINE_SPINLOCK(pause_on_oops_lock);
>  bool crash_kexec_post_notifiers;
>  int panic_on_warn __read_mostly;
> +unsigned long panic_on_taint;
> +bool panic_on_taint_exclusive = false;
>  
>  int panic_timeout = CONFIG_PANIC_TIMEOUT;
>  EXPORT_SYMBOL_GPL(panic_timeout);
> @@ -434,6 +437,11 @@ 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);
> +
> +	if (tainted_mask & panic_on_taint) {
> +		panic_on_taint = 0;

This panic_on_taint resetting is redundant? It will trigger crash, do we
need care if it's 0 or not?

> +		panic("panic_on_taint set ...");
> +	}
>  }
>  EXPORT_SYMBOL(add_taint);
>  
> @@ -686,3 +694,35 @@ static int __init oops_setup(char *s)
>  	return 0;
>  }
>  early_param("oops", oops_setup);
> +
> +static int __init panic_on_taint_setup(char *s)
> +{
> +	/* we just ignore panic_on_taint if passed without flags */
> +	if (!s)
> +		goto out;
> +
> +	for (; *s; s++) {
> +		int i;
> +
> +		if (*s == '-') {
> +			panic_on_taint_exclusive = true;
> +			continue;
> +		}
> +
> +		for (i = 0; i < TAINT_FLAGS_COUNT; i++) {
> +			if (toupper(*s) == taint_flags[i].c_true) {
> +				set_bit(i, &panic_on_taint);
> +				break;
> +			}
> +		}

Read admin-guide/tainted-kernels.rst, but still do not get what 'G' means.
If I specify 'panic_on_taint="G"' or 'panic_on_taint="-G"' in cmdline,
what is expected for this customer behaviour?

Except of above minor nitpicks, this patch looks good to me, thanks.

Reviewed-by: Baoquan He <bhe@redhat.com>

Thanks
Baoquan

Re: [PATCH v3] kernel: add panic_on_taint

[Randy Dunlap]

On 5/9/20 7:59 PM, Baoquan He wrote:
> Read admin-guide/tainted-kernels.rst, but still do not get what 'G' means.

I interpret 'G' as GPL (strictly it means that no proprietary module has
been loaded).  But I don't see why TAINT_PROPRIETARY_MODULE is the only
taint flag that has a non-blank c_false character.  It could just be blank
also AFAICT.  Then the 'G' would not be there to confuse us.  :)

-- 
~Randy

Re: [PATCH v3] kernel: add panic_on_taint

[Baoquan He]

On 05/09/20 at 09:10pm, Randy Dunlap wrote:
> On 5/9/20 7:59 PM, Baoquan He wrote:
> > Read admin-guide/tainted-kernels.rst, but still do not get what 'G' means.
> 
> I interpret 'G' as GPL (strictly it means that no proprietary module has
> been loaded).  But I don't see why TAINT_PROPRIETARY_MODULE is the only
> taint flag that has a non-blank c_false character.  It could just be blank
> also AFAICT.  Then the 'G' would not be there to confuse us.  :)

Yeah, seems c_false character is not so necessary. If no 'P' set, then
it means no proprietary modules loaded naturally. We may need clean up
the c_false in struct taint_flag, since c_true is enough to indicate
what want to check.

Re: [PATCH v3] kernel: add panic_on_taint

[Rafael Aquini]

On Sun, May 10, 2020 at 10:59:21AM +0800, Baoquan He wrote:
> On 05/09/20 at 09:57am, Rafael Aquini wrote:
> > Analogously to the introduction of panic_on_warn, this patch
> > introduces a kernel option named panic_on_taint in order to
> > provide a simple and generic way to stop execution and catch
> > a coredump when the kernel gets tainted by any given taint flag.
> > 
> > This is useful for debugging sessions as it avoids rebuilding
> > the kernel to explicitly add calls to panic() or BUG() into
> > code sites that introduce the taint flags of interest.
> > Another, perhaps less frequent, use for this option would be
> > as a mean for assuring a security policy (in paranoid mode)
> > case where no single taint is allowed for the running system.
> > 
> > Suggested-by: Qian Cai <cai@lca.pw>
> > Signed-off-by: Rafael Aquini <aquini@redhat.com>
> > ---
> > Changelog:
> > * v2: get rid of unnecessary/misguided compiler hints		(Luis)
> > * v2: enhance documentation text for the new kernel parameter	(Randy)
> > * v3: drop sysctl interface, keep it only as a kernel parameter (Luis)
> > 
> >  Documentation/admin-guide/kdump/kdump.rst     | 10 +++++
> >  .../admin-guide/kernel-parameters.txt         | 15 +++++++
> >  include/linux/kernel.h                        |  2 +
> >  kernel/panic.c                                | 40 +++++++++++++++++++
> >  kernel/sysctl.c                               |  9 ++++-
> >  5 files changed, 75 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/admin-guide/kdump/kdump.rst b/Documentation/admin-guide/kdump/kdump.rst
> > index ac7e131d2935..de3cf6d377cc 100644
> > --- a/Documentation/admin-guide/kdump/kdump.rst
> > +++ b/Documentation/admin-guide/kdump/kdump.rst
> > @@ -521,6 +521,16 @@ will cause a kdump to occur at the panic() call.  In cases where a user wants
> >  to specify this during runtime, /proc/sys/kernel/panic_on_warn can be set to 1
> >  to achieve the same behaviour.
> >  
> > +Trigger Kdump on add_taint()
> > +============================
> > +
> > +The kernel parameter, panic_on_taint, calls panic() from within add_taint(),
> > +whenever the value set in this bitmask matches with the bit flag being set
> > +by add_taint(). This will cause a kdump to occur at the panic() call.
> > +In cases where a user wants to specify this during runtime,
> > +/proc/sys/kernel/panic_on_taint can be set to a respective bitmask value
> > +to achieve the same behaviour.
> > +
> >  Contact
> >  =======
> >  
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index 7bc83f3d9bdf..4a69fe49a70d 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -3404,6 +3404,21 @@
> >  	panic_on_warn	panic() instead of WARN().  Useful to cause kdump
> >  			on a WARN().
> >  
> > +	panic_on_taint=	[KNL] conditionally panic() in add_taint()
> > +			Format: <str>
> 			Changed it as 'Format: <string>' to be
> consistent with the existing other options?

I can resubmit with the change, if it's a strong req and the surgery
cannot be done at merge time.


> > +			Specifies, as a string, the TAINT flag set that will
> > +			compose a bitmask for calling panic() when the kernel
> > +			gets tainted.
> > +			See Documentation/admin-guide/tainted-kernels.rst for
> > +			details on the taint flags that users can pick to
> > +			compose the bitmask to assign to panic_on_taint.
> > +			When the string is prefixed with a '-' the bitmask
> > +			set in panic_on_taint will be mutually exclusive
> > +			with the sysctl knob kernel.tainted, and any attempt
> > +			to write to that sysctl will fail with -EINVAL for
> > +			any taint value that masks with the flags set for
> > +			this option.
> > +
> >  	crash_kexec_post_notifiers
> >  			Run kdump after running panic-notifiers and dumping
> >  			kmsg. This only for the users who doubt kdump always
> > diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> > index 9b7a8d74a9d6..66bc102cb59a 100644
> > --- a/include/linux/kernel.h
> > +++ b/include/linux/kernel.h
> > @@ -528,6 +528,8 @@ extern int panic_on_oops;
> >  extern int panic_on_unrecovered_nmi;
> >  extern int panic_on_io_nmi;
> >  extern int panic_on_warn;
> > +extern unsigned long panic_on_taint;
> > +extern bool panic_on_taint_exclusive;
> >  extern int sysctl_panic_on_rcu_stall;
> >  extern int sysctl_panic_on_stackoverflow;
> >  
> > diff --git a/kernel/panic.c b/kernel/panic.c
> > index b69ee9e76cb2..65c62f8a1de8 100644
> > --- a/kernel/panic.c
> > +++ b/kernel/panic.c
> > @@ -25,6 +25,7 @@
> >  #include <linux/kexec.h>
> >  #include <linux/sched.h>
> >  #include <linux/sysrq.h>
> > +#include <linux/ctype.h>
> >  #include <linux/init.h>
> >  #include <linux/nmi.h>
> >  #include <linux/console.h>
> > @@ -44,6 +45,8 @@ static int pause_on_oops_flag;
> >  static DEFINE_SPINLOCK(pause_on_oops_lock);
> >  bool crash_kexec_post_notifiers;
> >  int panic_on_warn __read_mostly;
> > +unsigned long panic_on_taint;
> > +bool panic_on_taint_exclusive = false;
> >  
> >  int panic_timeout = CONFIG_PANIC_TIMEOUT;
> >  EXPORT_SYMBOL_GPL(panic_timeout);
> > @@ -434,6 +437,11 @@ 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);
> > +
> > +	if (tainted_mask & panic_on_taint) {
> > +		panic_on_taint = 0;
> 
> This panic_on_taint resetting is redundant? It will trigger crash, do we
> need care if it's 0 or not?
>

We might still get more than one CPU hitting a taint adding code path after 
the one that tripped here called panic. To avoid multiple calls to panic, 
in that particular scenario, we clear the panic_on_taint bitmask out. 
Also, albeit non-frequent, we might be tracking TAINT_WARN, and still hit 
a WARN_ON() in the panic / kdump path, thus incurring in a second 
(and unwanted) call to panic here.  

 
> > +		panic("panic_on_taint set ...");
> > +	}
> >  }
> >  EXPORT_SYMBOL(add_taint);
> >  
> > @@ -686,3 +694,35 @@ static int __init oops_setup(char *s)
> >  	return 0;
> >  }
> >  early_param("oops", oops_setup);
> > +
> > +static int __init panic_on_taint_setup(char *s)
> > +{
> > +	/* we just ignore panic_on_taint if passed without flags */
> > +	if (!s)
> > +		goto out;
> > +
> > +	for (; *s; s++) {
> > +		int i;
> > +
> > +		if (*s == '-') {
> > +			panic_on_taint_exclusive = true;
> > +			continue;
> > +		}
> > +
> > +		for (i = 0; i < TAINT_FLAGS_COUNT; i++) {
> > +			if (toupper(*s) == taint_flags[i].c_true) {
> > +				set_bit(i, &panic_on_taint);
> > +				break;
> > +			}
> > +		}
> 
> Read admin-guide/tainted-kernels.rst, but still do not get what 'G' means.
> If I specify 'panic_on_taint="G"' or 'panic_on_taint="-G"' in cmdline,
> what is expected for this customer behaviour?
> 

This will not panic the system as no taint flag gets actually set in 
panic_on_taint bitmask for G.

G is the counterpart of P, and appears on print_tainted() whenever
TAINT_PROPRIETARY_MODULE is not set. panic_on_taint doesn't set
anything for G, as it doesn't represent any taint, but the lack
of one particular taint, instead.

(apparently, TAINT_PROPRIETARY_MODULE is the only taint flag
that follows that pattern of having an extra assigned letter 
that means its absence, and perhaps it should be removed)

> Except of above minor nitpicks, this patch looks good to me, thanks.
> 
> Reviewed-by: Baoquan He <bhe@redhat.com>
> 
> Thanks
> Baoquan

Re: [PATCH v3] kernel: add panic_on_taint

[Baoquan He]

On 05/10/20 at 02:22pm, Rafael Aquini wrote:
> > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > > index 7bc83f3d9bdf..4a69fe49a70d 100644
> > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > @@ -3404,6 +3404,21 @@
> > >  	panic_on_warn	panic() instead of WARN().  Useful to cause kdump
> > >  			on a WARN().
> > >  
> > > +	panic_on_taint=	[KNL] conditionally panic() in add_taint()
> > > +			Format: <str>
> > 			Changed it as 'Format: <string>' to be
> > consistent with the existing other options?
> 
> I can resubmit with the change, if it's a strong req and the surgery
> cannot be done at merge time.

Yeah, maybe maintainer can help adjust this, not sure who will pick it.
No, it's not a strong request, people might get a little bit confusion
about which format should be referred to when a new kernel option is added.

> 
> 
> > > +			Specifies, as a string, the TAINT flag set that will
> > > +			compose a bitmask for calling panic() when the kernel
> > > +			gets tainted.
> > > +			See Documentation/admin-guide/tainted-kernels.rst for
> > > +			details on the taint flags that users can pick to
> > > +			compose the bitmask to assign to panic_on_taint.
> > > +			When the string is prefixed with a '-' the bitmask
> > > +			set in panic_on_taint will be mutually exclusive
> > > +			with the sysctl knob kernel.tainted, and any attempt
> > > +			to write to that sysctl will fail with -EINVAL for
> > > +			any taint value that masks with the flags set for
> > > +			this option.
> > > +
> > >  	crash_kexec_post_notifiers
> > >  			Run kdump after running panic-notifiers and dumping
> > >  			kmsg. This only for the users who doubt kdump always
> > > diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> > > index 9b7a8d74a9d6..66bc102cb59a 100644
> > > --- a/include/linux/kernel.h
> > > +++ b/include/linux/kernel.h
> > > @@ -528,6 +528,8 @@ extern int panic_on_oops;
> > >  extern int panic_on_unrecovered_nmi;
> > >  extern int panic_on_io_nmi;
> > >  extern int panic_on_warn;
> > > +extern unsigned long panic_on_taint;
> > > +extern bool panic_on_taint_exclusive;
> > >  extern int sysctl_panic_on_rcu_stall;
> > >  extern int sysctl_panic_on_stackoverflow;
> > >  
> > > diff --git a/kernel/panic.c b/kernel/panic.c
> > > index b69ee9e76cb2..65c62f8a1de8 100644
> > > --- a/kernel/panic.c
> > > +++ b/kernel/panic.c
> > > @@ -25,6 +25,7 @@
> > >  #include <linux/kexec.h>
> > >  #include <linux/sched.h>
> > >  #include <linux/sysrq.h>
> > > +#include <linux/ctype.h>
> > >  #include <linux/init.h>
> > >  #include <linux/nmi.h>
> > >  #include <linux/console.h>
> > > @@ -44,6 +45,8 @@ static int pause_on_oops_flag;
> > >  static DEFINE_SPINLOCK(pause_on_oops_lock);
> > >  bool crash_kexec_post_notifiers;
> > >  int panic_on_warn __read_mostly;
> > > +unsigned long panic_on_taint;
> > > +bool panic_on_taint_exclusive = false;
> > >  
> > >  int panic_timeout = CONFIG_PANIC_TIMEOUT;
> > >  EXPORT_SYMBOL_GPL(panic_timeout);
> > > @@ -434,6 +437,11 @@ 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);
> > > +
> > > +	if (tainted_mask & panic_on_taint) {
> > > +		panic_on_taint = 0;
> > 
> > This panic_on_taint resetting is redundant? It will trigger crash, do we
> > need care if it's 0 or not?
> >
> 
> We might still get more than one CPU hitting a taint adding code path after 
> the one that tripped here called panic. To avoid multiple calls to panic, 
> in that particular scenario, we clear the panic_on_taint bitmask out. 
> Also, albeit non-frequent, we might be tracking TAINT_WARN, and still hit 
> a WARN_ON() in the panic / kdump path, thus incurring in a second 
> (and unwanted) call to panic here.  

Hmm, this cpu will set panic_cpu firstly, all other cpu need stop and
have no chance to execute panic. But yes, clearing panic_on_taint makes
code easier to understand.

> 
>  
> > > +		panic("panic_on_taint set ...");
> > > +	}
> > >  }
> > >  EXPORT_SYMBOL(add_taint);
> > >  
> > > @@ -686,3 +694,35 @@ static int __init oops_setup(char *s)
> > >  	return 0;
> > >  }
> > >  early_param("oops", oops_setup);
> > > +
> > > +static int __init panic_on_taint_setup(char *s)
> > > +{
> > > +	/* we just ignore panic_on_taint if passed without flags */
> > > +	if (!s)
> > > +		goto out;
> > > +
> > > +	for (; *s; s++) {
> > > +		int i;
> > > +
> > > +		if (*s == '-') {
> > > +			panic_on_taint_exclusive = true;
> > > +			continue;
> > > +		}
> > > +
> > > +		for (i = 0; i < TAINT_FLAGS_COUNT; i++) {
> > > +			if (toupper(*s) == taint_flags[i].c_true) {
> > > +				set_bit(i, &panic_on_taint);
> > > +				break;
> > > +			}
> > > +		}
> > 
> > Read admin-guide/tainted-kernels.rst, but still do not get what 'G' means.
> > If I specify 'panic_on_taint="G"' or 'panic_on_taint="-G"' in cmdline,
> > what is expected for this customer behaviour?
> > 
> 
> This will not panic the system as no taint flag gets actually set in 
> panic_on_taint bitmask for G.
> 
> G is the counterpart of P, and appears on print_tainted() whenever
> TAINT_PROPRIETARY_MODULE is not set. panic_on_taint doesn't set
> anything for G, as it doesn't represent any taint, but the lack
> of one particular taint, instead.
> 
> (apparently, TAINT_PROPRIETARY_MODULE is the only taint flag
> that follows that pattern of having an extra assigned letter 
> that means its absence, and perhaps it should be removed)

Yeah, agree. I will make a draft patch to remove it, see if there's
objection from people.

Re: [PATCH v3] kernel: add panic_on_taint

[Luis Chamberlain]

On Sat, May 09, 2020 at 09:57:37AM -0400, Rafael Aquini wrote:
> +Trigger Kdump on add_taint()
> +============================
> +
> +The kernel parameter, panic_on_taint, calls panic() from within add_taint(),
> +whenever the value set in this bitmask matches with the bit flag being set
> +by add_taint(). This will cause a kdump to occur at the panic() call.
> +In cases where a user wants to specify this during runtime,
> +/proc/sys/kernel/panic_on_taint can be set to a respective bitmask value
> +to achieve the same behaviour.
> +
>  Contact
>  =======
>  
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 7bc83f3d9bdf..4a69fe49a70d 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -3404,6 +3404,21 @@
>  	panic_on_warn	panic() instead of WARN().  Useful to cause kdump
>  			on a WARN().
>  
> +	panic_on_taint=	[KNL] conditionally panic() in add_taint()
> +			Format: <str>
> +			Specifies, as a string, the TAINT flag set that will
> +			compose a bitmask for calling panic() when the kernel
> +			gets tainted.
> +			See Documentation/admin-guide/tainted-kernels.rst for
> +			details on the taint flags that users can pick to
> +			compose the bitmask to assign to panic_on_taint.
> +			When the string is prefixed with a '-' the bitmask
> +			set in panic_on_taint will be mutually exclusive
> +			with the sysctl knob kernel.tainted, and any attempt
> +			to write to that sysctl will fail with -EINVAL for
> +			any taint value that masks with the flags set for
> +			this option.

This talks about using a string, but that it sets a bitmask. Its not
very clear that one must use the string representation from each taint
flag. Also, I don't think to use the character representation as we
limit ourselves to the alphabet and quirky what-should-be-arbitrary
characters that represent the taint flags. The taint flag character
representation is juse useful for human reading of a panic, but I think
because of the limitation of the mask with the alphabet this was not
such a great idea long term.

So, I don't think we should keep on extending the alphabet use case, a
simple digit representation would suffice. I think this means we'd need
two params one for exclusive and one for the value of the taint.

Using a hex value or number also lets us make the input value shorter.

If a kernel boots with panic-on-taint flag not yet supported, we don't
complain, therefore getting a false sense of security that we will panic
with a not yet supported taint flag. I think we should pr_warn() or
fail to boot when that happens.

  Luis

Re: [PATCH v3] kernel: add panic_on_taint

[Rafael Aquini]

On Mon, May 11, 2020 at 06:24:55PM +0000, Luis Chamberlain wrote:
> On Sat, May 09, 2020 at 09:57:37AM -0400, Rafael Aquini wrote:
> > +Trigger Kdump on add_taint()
> > +============================
> > +
> > +The kernel parameter, panic_on_taint, calls panic() from within add_taint(),
> > +whenever the value set in this bitmask matches with the bit flag being set
> > +by add_taint(). This will cause a kdump to occur at the panic() call.
> > +In cases where a user wants to specify this during runtime,
> > +/proc/sys/kernel/panic_on_taint can be set to a respective bitmask value
> > +to achieve the same behaviour.
> > +
> >  Contact
> >  =======
> >  
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index 7bc83f3d9bdf..4a69fe49a70d 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -3404,6 +3404,21 @@
> >  	panic_on_warn	panic() instead of WARN().  Useful to cause kdump
> >  			on a WARN().
> >  
> > +	panic_on_taint=	[KNL] conditionally panic() in add_taint()
> > +			Format: <str>
> > +			Specifies, as a string, the TAINT flag set that will
> > +			compose a bitmask for calling panic() when the kernel
> > +			gets tainted.
> > +			See Documentation/admin-guide/tainted-kernels.rst for
> > +			details on the taint flags that users can pick to
> > +			compose the bitmask to assign to panic_on_taint.
> > +			When the string is prefixed with a '-' the bitmask
> > +			set in panic_on_taint will be mutually exclusive
> > +			with the sysctl knob kernel.tainted, and any attempt
> > +			to write to that sysctl will fail with -EINVAL for
> > +			any taint value that masks with the flags set for
> > +			this option.
> 
> This talks about using a string, but that it sets a bitmask. Its not
> very clear that one must use the string representation from each taint
> flag. Also, I don't think to use the character representation as we
> limit ourselves to the alphabet and quirky what-should-be-arbitrary
> characters that represent the taint flags. The taint flag character
> representation is juse useful for human reading of a panic, but I think
> because of the limitation of the mask with the alphabet this was not
> such a great idea long term.
>

I respectfully disagree with you on this one, but that might be just
because I'm a non-native English speaker and the cause of confusion is 
not very clear to me. Would you mind providing a blurb with a text that
you think it would be clearer on this regard?

Also, making the input of the option to match with something one
is used to see in taint reports make it easier to use. It would
be still a human setting the boot option, so it makes sense to
utilize a known/meaningful representation for panic_on_taint input.

 
> So, I don't think we should keep on extending the alphabet use case, a
> simple digit representation would suffice. I think this means we'd need
> two params one for exclusive and one for the value of the taint.
> 
> Using a hex value or number also lets us make the input value shorter.
>

Albeit you're right on the limitation of an alphabetical representation, 
the truth is that taint flags are not introduced that frequently.
Considering how many times these flags were introduced in the past,
one could infer we probably will not run out of alphabet in the next 20 
years (a couple of new flags gets introduced every 2 years, or so, in
average), and the rate of change in these seems to be slowing down
considerably, as in the past 5 years, we've seen only 2 new flags.

I'm not against your suggestion, though, but I think it makes
clumsier to utilize the feature as you now require 2 kernel
cmdline options, instead of just one, and a less intuitive
way of setting it via base16 values. All at the expense of 
a theoretical shortage of taint flags. 

If the others that were already OK with the simple string interface 
don't object your change suggestions in that regard, I'll refactor
the parser to meet them. 


> If a kernel boots with panic-on-taint flag not yet supported, we don't
> complain, therefore getting a false sense of security that we will panic
> with a not yet supported taint flag. I think we should pr_warn() or
> fail to boot when that happens.
>

Bear in mind that these very early print outs (like the ones eventually
produced by setting panic_on_taint) to the log buffer might get lost in 
verbose-logging long-running time systems, but apart from that, I see no 
problems in being a little bit more verbose in that parser. I'll make
the changes for a next revision, if no one else objects them.

Cheers!
-- Rafael

Re: [PATCH v3] kernel: add panic_on_taint

[Luis Chamberlain]

On Mon, May 11, 2020 at 04:03:25PM -0400, Rafael Aquini wrote:
> On Mon, May 11, 2020 at 06:24:55PM +0000, Luis Chamberlain wrote:
> > On Sat, May 09, 2020 at 09:57:37AM -0400, Rafael Aquini wrote:
> > > +Trigger Kdump on add_taint()
> > > +============================
> > > +
> > > +The kernel parameter, panic_on_taint, calls panic() from within add_taint(),
> > > +whenever the value set in this bitmask matches with the bit flag being set
> > > +by add_taint(). This will cause a kdump to occur at the panic() call.
> > > +In cases where a user wants to specify this during runtime,
> > > +/proc/sys/kernel/panic_on_taint can be set to a respective bitmask value
> > > +to achieve the same behaviour.
> > > +
> > >  Contact
> > >  =======
> > >  
> > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > > index 7bc83f3d9bdf..4a69fe49a70d 100644
> > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > @@ -3404,6 +3404,21 @@
> > >  	panic_on_warn	panic() instead of WARN().  Useful to cause kdump
> > >  			on a WARN().
> > >  
> > > +	panic_on_taint=	[KNL] conditionally panic() in add_taint()
> > > +			Format: <str>
> > > +			Specifies, as a string, the TAINT flag set that will
> > > +			compose a bitmask for calling panic() when the kernel
> > > +			gets tainted.
> > > +			See Documentation/admin-guide/tainted-kernels.rst for
> > > +			details on the taint flags that users can pick to
> > > +			compose the bitmask to assign to panic_on_taint.
> > > +			When the string is prefixed with a '-' the bitmask
> > > +			set in panic_on_taint will be mutually exclusive
> > > +			with the sysctl knob kernel.tainted, and any attempt
> > > +			to write to that sysctl will fail with -EINVAL for
> > > +			any taint value that masks with the flags set for
> > > +			this option.
> > 
> > This talks about using a string, but that it sets a bitmask. Its not
> > very clear that one must use the string representation from each taint
> > flag. Also, I don't think to use the character representation as we
> > limit ourselves to the alphabet and quirky what-should-be-arbitrary
> > characters that represent the taint flags. The taint flag character
> > representation is juse useful for human reading of a panic, but I think
> > because of the limitation of the mask with the alphabet this was not
> > such a great idea long term.
> >
> 
> I respectfully disagree with you on this one, but that might be just
> because I'm a non-native English speaker and the cause of confusion is 
> not very clear to me. Would you mind providing a blurb with a text that
> you think it would be clearer on this regard?

A simple example of what can be used and what it would mean would
suffice.

> Also, making the input of the option to match with something one
> is used to see in taint reports make it easier to use. It would
> be still a human setting the boot option, so it makes sense to
> utilize a known/meaningful representation for panic_on_taint input.

Yes, however I still believe that what we are printing is only doing
a disservice to limiting the size of our bitmask.

> > So, I don't think we should keep on extending the alphabet use case, a
> > simple digit representation would suffice. I think this means we'd need
> > two params one for exclusive and one for the value of the taint.
> > 
> > Using a hex value or number also lets us make the input value shorter.
> >
> 
> Albeit you're right on the limitation of an alphabetical representation, 
> the truth is that taint flags are not introduced that frequently.
> Considering how many times these flags were introduced in the past,
> one could infer we probably will not run out of alphabet in the next 20 
> years (a couple of new flags gets introduced every 2 years, or so, in
> average), and the rate of change in these seems to be slowing down
> considerably, as in the past 5 years, we've seen only 2 new flags.
> 
> I'm not against your suggestion, though, but I think it makes
> clumsier to utilize the feature as you now require 2 kernel
> cmdline options, instead of just one, and a less intuitive
> way of setting it via base16 values. All at the expense of 
> a theoretical shortage of taint flags. 
> 
> If the others that were already OK with the simple string interface 
> don't object your change suggestions in that regard, I'll refactor
> the parser to meet them. 

It is just silly for us to restrict our bitmask to the alphabet. The
alphabetic restrictions were useful for print until we started running
out of meaningful characters, moving forward they'll just be a nuisance.

I don't think its wise to encourage their use now for input, now that
we have realized that their use will be pointless soon.

> > If a kernel boots with panic-on-taint flag not yet supported, we don't
> > complain, therefore getting a false sense of security that we will panic
> > with a not yet supported taint flag. I think we should pr_warn() or
> > fail to boot when that happens.
> >
> 
> Bear in mind that these very early print outs (like the ones eventually
> produced by setting panic_on_taint) to the log buffer might get lost in 
> verbose-logging long-running time systems, but apart from that, I see no 
> problems in being a little bit more verbose in that parser. I'll make
> the changes for a next revision, if no one else objects them.

See mitigations_parse_cmdline(), we pr_crit() there as well if we enter
a wrong value. I think that's the best we can do.

  Luis