lkml.org 
[lkml]   [2018]   [Sep]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] tty/sysrq: Make local variable 'killer' in sysrq_handle_crash() global
From
Date
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

\
 
 \ /
  Last update: 2018-09-18 11:05    [W:0.071 / U:1.180 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site