linux-kernel - Re: [PATCH] tty/sysrq: Make local variable 'killer' in sysrq_handle

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ccc91067-9718-8eb8-c197-b9c560daa5ce@codeaurora.org>
Date:   Tue, 18 Sep 2018 14:35:02 +0530
From:   Sai Prakash Ranjan <saiprakash.ranjan@...eaurora.org>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Jiri Slaby <jslaby@...e.cz>, Matthias Kaehlcke <mka@...omium.org>,
        Douglas Anderson <dianders@...omium.org>,
        Evan Green <evgreen@...omium.org>,
        Manoj Gupta <manojgupta@...omium.org>,
        Stephen Boyd <swboyd@...omium.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] tty/sysrq: Make local variable 'killer' in
 sysrq_handle_crash() global

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@...eaurora.org>
>>>> Suggested-by: Evan Green <evgreen@...omium.org>
>>>> Signed-off-by: Matthias Kaehlcke <mka@...omium.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