Re: [RFC PATCH v2] do_exit(): panic() recursion detected

[Vladimir Kondratiev]

I see 2 paths how "bad things" can cause recursive do_exit - various 
traps that go through die() and therefore covered by panic_on_oops; and 
do_group_exit() as result of fatal signal.

Provided one add "panic on coredump" functionality, path through 
do_group_exit() covered as well.

Let's drop this patch.

Thanks, Vladimir

On 12/7/20 5:49 PM, Eric W. Biederman wrote:
> Vladimir Kondratiev <vladimir.kondratiev@xxxxxxxxxxxxxxx> writes:
>
> > Please ignore version 1 of the patch - it was sent from wrong mail address.
> >
> > To clarify the reason:
> >
> > Situation where do_exit() re-entered, discovered by static code analysis.
> > For safety critical system, it is better to panic() when minimal chance of
> > corruption detected. For this reason, we also panic on fatal signal delivery -
> > patch for this not submitted yet.
>
> What did the static code analysis say?  What triggers the recursion.
>
> What makes it safe to even call panic on this code path?  Is there
> enough kernel stack?
>
> My sense is that if this actually can happen and is a real concern,
> and that it is safe to do something on this code path it is probably
> better just to ooops.  That way if someone is trying to debug such
> a recursion they will have a backtrace to work with.  Plus panic
> on oops will work.
>
> Eric
>
> > On 12/7/20 2:44 PM, Vladimir Kondratiev wrote:
> > > Recursive do_exit() is symptom of compromised kernel integrity.
> > > For safety critical systems, it may be better to
> > > panic() in this case to minimize risk.
> > >
> > > Signed-off-by: Vladimir Kondratiev <vladimir.kondratiev@xxxxxxxxxxxxxxx>
> > > Change-Id: I42f45900a08c4282c511b05e9e6061360d07db60
> > > ---
> > >    Documentation/admin-guide/kernel-parameters.txt | 6 ++++++
> > >    include/linux/kernel.h                          | 1 +
> > >    kernel/exit.c                                   | 7 +++++++
> > >    kernel/sysctl.c                                 | 9 +++++++++
> > >    4 files changed, 23 insertions(+)
> > >
> > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > > index 44fde25bb221..6e12a6804557 100644
> > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > @@ -3508,6 +3508,12 @@
> > >    			bit 4: print ftrace buffer
> > >    			bit 5: print all printk messages in buffer
> > >    +	panic_on_exit_recursion
> > > +			panic() when do_exit() recursion detected, rather then
> > > +			try to stay running whenever possible.
> > > +			Useful on safety critical systems; re-entry in do_exit
> > > +			is a symptom of compromised kernel integrity.
> > > +
> > >    	panic_on_taint=	Bitmask for conditionally calling panic() in add_taint()
> > >    			Format: <hex>[,nousertaint]
> > >    			Hexadecimal bitmask representing the set of TAINT flags
> > > diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> > > index 2f05e9128201..5afb20534cb2 100644
> > > --- a/include/linux/kernel.h
> > > +++ b/include/linux/kernel.h
> > > @@ -539,6 +539,7 @@ extern int sysctl_panic_on_rcu_stall;
> > >    extern int sysctl_panic_on_stackoverflow;
> > >      extern bool crash_kexec_post_notifiers;
> > > +extern int panic_on_exit_recursion;
> > >      /*
> > >     * panic_cpu is used for synchronizing panic() and crash_kexec() execution. It
> > > diff --git a/kernel/exit.c b/kernel/exit.c
> > > index 1f236ed375f8..162799a8b539 100644
> > > --- a/kernel/exit.c
> > > +++ b/kernel/exit.c
> > > @@ -68,6 +68,9 @@
> > >    #include <asm/unistd.h>
> > >    #include <asm/mmu_context.h>
> > >    +int panic_on_exit_recursion __read_mostly;
> > > +core_param(panic_on_exit_recursion, panic_on_exit_recursion, int, 0644);
> > > +
> > >    static void __unhash_process(struct task_struct *p, bool group_dead)
> > >    {
> > >    	nr_threads--;
> > > @@ -757,6 +760,10 @@ void __noreturn do_exit(long code)
> > >    	 */
> > >    	if (unlikely(tsk->flags & PF_EXITING)) {
> > >    		pr_alert("Fixing recursive fault but reboot is needed!\n");
> > > +		if (panic_on_exit_recursion)
> > > +			panic("Recursive do_exit() detected in %s[%d]\n",
> > > +			      current->comm, task_pid_nr(current));
> > > +
> > >    		futex_exit_recursive(tsk);
> > >    		set_current_state(TASK_UNINTERRUPTIBLE);
> > >    		schedule();
> > > diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> > > index afad085960b8..bb397fba2c42 100644
> > > --- a/kernel/sysctl.c
> > > +++ b/kernel/sysctl.c
> > > @@ -2600,6 +2600,15 @@ static struct ctl_table kern_table[] = {
> > >    		.extra2		= &one_thousand,
> > >    	},
> > >    #endif
> > > +	{
> > > +		.procname	= "panic_on_exit_recursion",
> > > +		.data		= &panic_on_exit_recursion,
> > > +		.maxlen		= sizeof(int),
> > > +		.mode		= 0644,
> > > +		.proc_handler	= proc_dointvec_minmax,
> > > +		.extra1		= SYSCTL_ZERO,
> > > +		.extra2		= SYSCTL_ONE,
> > > +	},
> > >    	{
> > >    		.procname	= "panic_on_warn",
> > >    		.data		= &panic_on_warn,