From: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.cz>, Matthias Kaehlcke <mka@chromium.org>,
Douglas Anderson <dianders@chromium.org>,
Evan Green <evgreen@chromium.org>,
Manoj Gupta <manojgupta@chromium.org>,
Stephen Boyd <swboyd@chromium.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 14:35:02 +0530 [thread overview]
Message-ID: <ccc91067-9718-8eb8-c197-b9c560daa5ce@codeaurora.org> (raw)
In-Reply-To: <20180918072037.GA4442@kroah.com>
On 9/18/2018 12:50 PM, Greg Kroah-Hartman wrote:
> On Tue, Sep 18, 2018 at 12:28:39PM +0530, Sai Prakash Ranjan wrote:
>> On 9/18/2018 11:41 AM, 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?
>>>
>>
>> static global does not work, clang still inserts brk. As for
>> OPTIMIZE_HIDE_VAR, it seems to work.
>> But, I dont think it is defined for clang in which case it defaults to using
>> barrier(). There is already one wmb(), so will it be right?
>
> Ick, why is this needed at all? Why are we trying to "roll our own
> panic" in this code?
>
Hi Greg, do you mean like why there is a killer var at all or why this
change is required?
Below are some additional info about this issue:
CLANG generated:
ffffff800842bc1c <sysrq_handle_crash>:
ffffff800842bc1c: a9bf7bfd stp x29, x30, [sp,#-16]!
ffffff800842bc20: 910003fd mov x29, sp
ffffff800842bc24: 97f1a34e bl ffffff800809495c
<_mcount>
ffffff800842bc28: 97f38876 bl ffffff800810de00
<__rcu_read_unlock>
ffffff800842bc2c: f0006888 adrp x8, ffffff800913e000
<reset_devices>
ffffff800842bc30: 320003e9 orr w9, wzr, #0x1
ffffff800842bc34: b9091109 str w9, [x8,#2320]
ffffff800842bc38: d5033e9f dsb st
ffffff800842bc3c: d4200020 brk #0x1 <----
GCC generated:
ffffff8008452f60 <sysrq_handle_crash>:
ffffff8008452f60: a9bf7bfd stp x29, x30, [sp,#-16]!
ffffff8008452f64: 910003fd mov x29, sp
ffffff8008452f68: aa1e03e0 mov x0, x30
ffffff8008452f6c: 97f1078c bl ffffff8008094d9c <_mcount>
ffffff8008452f70: 97f2f2cc bl ffffff800810faa0
<__rcu_read_unlock>
ffffff8008452f74: 900067e1 adrp x1, ffffff800914e000
<reset_devices>
ffffff8008452f78: 52800020 mov w0, #0x1
// #1
ffffff8008452f7c: b9096820 str w0, [x1,#2408]
ffffff8008452f80: d5033e9f dsb st
ffffff8008452f84: d2800001 mov x1, #0x0
// #0
ffffff8008452f88: 39000020 strb w0, [x1]
ffffff8008452f8c: a8c17bfd ldp x29, x30, [sp],#16
ffffff8008452f90: d65f03c0 ret
Trigger sysrq crash:
localhost ~ # echo c > /proc/sysrq-trigger
[ 78.320401] sysrq: SysRq : Trigger a crash
[ 78.324752] Unexpected kernel BRK exception at EL1
[ 78.329691] Unhandled debug exception: ptrace BRK handler
(0xf2000001) at 0x000000000e25c368
[ 78.338384] Internal error: : 0 [#1] PREEMPT SMP
Thanks,
Sai
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
next prev parent reply other threads:[~2018-09-18 9:05 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-09-17 21:33 [PATCH] tty/sysrq: Make local variable 'killer' in sysrq_handle_crash() global Matthias Kaehlcke
2018-09-18 6:09 ` Sai Prakash Ranjan
2018-09-18 6:11 ` Jiri Slaby
2018-09-18 6:58 ` Sai Prakash Ranjan
2018-09-18 7:20 ` Greg Kroah-Hartman
2018-09-18 9:05 ` Sai Prakash Ranjan [this message]
2018-09-18 9:17 ` Greg Kroah-Hartman
2018-09-18 9:53 ` Sai Prakash Ranjan
[not found] ` <32bf1760-4967-a37a-6a17-3f7d5f6e071e@suse.cz>
2018-09-18 17:32 ` Matthias Kaehlcke
2018-09-18 17:24 ` Matthias Kaehlcke
2018-09-18 11:46 ` David Laight
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ccc91067-9718-8eb8-c197-b9c560daa5ce@codeaurora.org \
--to=saiprakash.ranjan@codeaurora.org \
--cc=dianders@chromium.org \
--cc=evgreen@chromium.org \
--cc=gregkh@linuxfoundation.org \
--cc=jslaby@suse.cz \
--cc=linux-kernel@vger.kernel.org \
--cc=manojgupta@chromium.org \
--cc=mka@chromium.org \
--cc=ndesaulniers@google.com \
--cc=swboyd@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).