Re: [PATCH] tty/sysrq: Make local variable 'killer' in sysrq_handle_crash() global

From: Matthias Kaehlcke <mka@chromium.org>
To: Jiri Slaby <jslaby@suse.cz>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Douglas Anderson <dianders@chromium.org>,
	Evan Green <evgreen@chromium.org>,
	Manoj Gupta <manojgupta@chromium.org>,
	Stephen Boyd <swboyd@chromium.org>,
	Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>,
	Nick Desaulniers <ndesaulniers@google.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] tty/sysrq: Make local variable 'killer' in sysrq_handle_crash() global
Date: Tue, 18 Sep 2018 10:24:27 -0700	[thread overview]
Message-ID: <20180918172427.GO22824@google.com> (raw)
In-Reply-To: <a75e8a5d-25d0-83b8-6f7c-b80e69985b60@suse.cz>

On Tue, Sep 18, 2018 at 08:11:33AM +0200, Jiri Slaby wrote:
> On 09/17/2018, 11:33 PM, Matthias Kaehlcke wrote:
> > sysrq_handle_crash() dereferences a NULL pointer on purpose to force
> > an exception, the local variable 'killer' is assigned to NULL and
> > dereferenced later. Clang detects the NULL pointer dereference at compile
> > time and emits a BRK instruction (on arm64) instead of the expected NULL
> > pointer exception. Change 'killer' to a global variable (and rename it
> > to 'sysrq_killer' to avoid possible clashes) to prevent Clang from
> > detecting the condition. By default global variables are initialized
> > with zero/NULL in C, therefore an explicit initialization is not needed.
> > 
> > Reported-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
> > Suggested-by: Evan Green <evgreen@chromium.org>
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> >  drivers/tty/sysrq.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
> > index 06ed20dd01ba..49fa8e758690 100644
> > --- a/drivers/tty/sysrq.c
> > +++ b/drivers/tty/sysrq.c
> > @@ -132,10 +132,10 @@ static struct sysrq_key_op sysrq_unraw_op = {
> >  #define sysrq_unraw_op (*(struct sysrq_key_op *)NULL)
> >  #endif /* CONFIG_VT */
> >  
> > +char *sysrq_killer;
> > +
> >  static void sysrq_handle_crash(int key)
> >  {
> > -	char *killer = NULL;
> > -
> >  	/* we need to release the RCU read lock here,
> >  	 * otherwise we get an annoying
> >  	 * 'BUG: sleeping function called from invalid context'
> > @@ -144,7 +144,7 @@ static void sysrq_handle_crash(int key)
> >  	rcu_read_unlock();
> >  	panic_on_oops = 1;	/* force panic */
> >  	wmb();
> > -	*killer = 1;
> > +	*sysrq_killer = 1;
> 
> Just because a static analyzer is wrong? Oh wait, even compiler is
> wrong. At least make it a static global. Or what about OPTIMIZER_HIDE_VAR?

With a static global the compiler still can know that the value is
constant.

Anyway, our compiler folks raised concerns that the variable could
still be identified as constant with Link Time Optimization (LTO)
enabled, so this patch isn't a good solution.

On the positive side the compiler engs don't expect the NULL pointer
access being optimized away with -fno-delete-null-pointer-checks
enabled, which recently landed in upstream LLVM.

I confirmed that with -fno-delete-null-pointer-checks Clang does not
emit 'brk' in this case and the kernel crashes with a NULL pointer
exception when this code path is triggered.

Seems we can forget about this patch, though we might still want to
replace the forced crash with a panic() as mentioned in this thread.

Thanks

m.