Re: [PATCH] tty/sysrq: Make local variable 'killer' in sysrq_handle_crash() global
From: Sai Prakash Ranjan
Date: Tue Sep 18 2018 - 05:07:43 EST
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@xxxxxxxxxxxxxx>
Suggested-by: Evan Green <evgreen@xxxxxxxxxxxx>
Signed-off-by: Matthias Kaehlcke <mka@xxxxxxxxxxxx>
---
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