On Wed, May 13, 2020 at 03:47:22PM +0000, Luis Chamberlain wrote:
> On Wed, May 13, 2020 at 11:00:26AM -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.
> > For instance, if one is interested in following up with
> > a post mortem analysis at the point a code path is hitting
> > a bad page (i.e. unaccount_page_cache_page(), or slab_bug()),
> > a crashdump could be collected by rebooting the kernel with
> > 'panic_on_taint=0x20' amended to the command line string.
> >
> > Another, perhaps less frequent, use for this option would be
> > as a mean for assuring a security policy case where only a
> > subset of taints, or no single taint (in paranoid mode),
> > is allowed for the running system.
> > The optional switch 'nousertaint' is handy in this particular
> > scenario as it will avoid userspace induced crashes by writes
> > to /proc/sys/kernel/tainted causing false positive hits for
> > such policies.
> >
> > Suggested-by: Qian Cai <cai@xxxxxx>
> > Signed-off-by: Rafael Aquini <aquini@xxxxxxxxxx>
> > ---
> > Changelog:
> > * v2: get rid of unnecessary/misguided compiler hints (Luis)
> > enhance documentation text for the new kernel parameter (Randy)
> > * v3: drop sysctl interface, keep it only as a kernel parameter (Luis)
> > * v4: change panic_on_taint input from alphabetical taint flags
> > to hexadecimal bitmasks, for clarity and extendability (Luis)
> >
> > Documentation/admin-guide/kdump/kdump.rst | 7 ++++
> > .../admin-guide/kernel-parameters.txt | 13 +++++++
> > include/linux/kernel.h | 4 +++
> > kernel/panic.c | 34 +++++++++++++++++++
> > kernel/sysctl.c | 11 +++++-
> > 5 files changed, 68 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/admin-guide/kdump/kdump.rst b/Documentation/admin-guide/kdump/kdump.rst
> > index ac7e131d2935..2707de840fd3 100644
> > --- a/Documentation/admin-guide/kdump/kdump.rst
> > +++ b/Documentation/admin-guide/kdump/kdump.rst
> > @@ -521,6 +521,13 @@ 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 facilitates calling 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.
> > +
> > Contact
> > =======
> >
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index 7bc83f3d9bdf..ce17fdbec7d1 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -3401,6 +3401,19 @@
> > bit 4: print ftrace buffer
> > bit 5: print all printk messages in buffer
> >
> > + panic_on_taint= Bitmask for conditionally call panic() in add_taint()
> > + Format: <hex>[,nousertaint]
> > + Hexadecimal bitmask representing the set of TAINT flags
> > + that will cause the kernel to panic when add_taint() is
> > + called with any of the flags in this set.
> > + The optional switch "nousertaint" can be utilized to
> > + prevent userland forced crashes by writing to sysctl
> > + /proc/sys/kernel/tainted any flagset matching with the
> > + bitmask set on panic_on_taint.
> > + See Documentation/admin-guide/tainted-kernels.rst for
> > + extra details on the taint flags that users can pick
> > + to compose the bitmask to assign to panic_on_taint.
> > +
> > panic_on_warn panic() instead of WARN(). Useful to cause kdump
> > on a WARN().
> >
> > diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> > index 9b7a8d74a9d6..70712944dffc 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_nousertaint;
> > extern int sysctl_panic_on_rcu_stall;
> > extern int sysctl_panic_on_stackoverflow;
> >
> > @@ -597,6 +599,8 @@ extern enum system_states {
> > #define TAINT_RANDSTRUCT 17
> > #define TAINT_FLAGS_COUNT 18
> >
> > +#define TAINT_FLAGS_MAX ((1UL << TAINT_FLAGS_COUNT) - 1)
> > +
> > struct taint_flag {
> > char c_true; /* character printed when tainted */
> > char c_false; /* character printed when not tainted */
> > diff --git a/kernel/panic.c b/kernel/panic.c
> > index b69ee9e76cb2..94b5c973770c 100644
> > --- a/kernel/panic.c
> > +++ b/kernel/panic.c
> > @@ -44,6 +44,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_nousertaint = false;
> >
> > int panic_timeout = CONFIG_PANIC_TIMEOUT;
> > EXPORT_SYMBOL_GPL(panic_timeout);
> > @@ -434,6 +436,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 +693,30 @@ static int __init oops_setup(char *s)
> > return 0;
> > }
> > early_param("oops", oops_setup);
> > +
> > +static int __init panic_on_taint_setup(char *s)
> > +{
> > + char *taint_str;
> > +
> > + if (!s)
> > + return -EINVAL;
> > +
> > + taint_str = strsep(&s, ",");
> > + if (kstrtoul(taint_str, 16, &panic_on_taint))
> > + return -EINVAL;
> > +
> > + /* make sure panic_on_taint doesn't hold out-of-range TAINT flags */
> > + panic_on_taint &= TAINT_FLAGS_MAX;
>
> While it may have made sennse for simplicity to not pr_warn_once on the
> proc_taint() case I think in this case we do want to pr_warn_once() as
> the user is wishing to DEFINITELY PANIC if such a taint flag is present.
>
In case the bitmask is invalidated (because user has set it deliberately
to 0, or because it was set to a specific flagset totally out of the valid
range, which will cause the bitwise-and to render panic_on_taint=0) the non-zero
return in the checkpoint below will take care of informing that the option
was malformed and it's not set. For all other cases where out-of-range
flags get ignored, but a flagset is committed to panic_on_taint, the user
can verify the results that will be printed out at the pr_info() call.
There is no need for an extra custom printout for this case, IMO.
> > +
> > + if (!panic_on_taint)
> > + return -EINVAL;
> > +
> > + if (s && !strcmp(s, "nousertaint"))
> > + panic_on_taint_nousertaint = true;
> > +
> > + pr_info("panic_on_taint: bitmask=0x%lx nousertaint_mode=%sabled\n",
> > + panic_on_taint, panic_on_taint_nousertaint ? "en" : "dis");
> > +
> > + return 0;
> > +}
> > +early_param("panic_on_taint", panic_on_taint_setup);
> > diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> > index 8a176d8727a3..e257c965683a 100644
> > --- a/kernel/sysctl.c
> > +++ b/kernel/sysctl.c
> > @@ -2623,11 +2623,20 @@ 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_nousertaint && (tmptaint & panic_on_taint))
> > + return -EINVAL;
> > +
>
> I like the compromise, but I think you also have to update this sysctl's
> documentation to reflect this is disabled if this new boot param is used.
>
Indeed, sorry I missed that part. I'll update it and repost.
-- Rafael
