* [PATCH 0/4] CONFIG_UNWINDER_FRAME_POINTER fixes+cleanups
@ 2020-07-30 20:51 ` Nick Desaulniers
0 siblings, 0 replies; 40+ messages in thread
From: Nick Desaulniers @ 2020-07-30 20:51 UTC (permalink / raw
To: Nathan Huckleberry, Russell King
Cc: Andrew Morton, Chunyan Zhang, clang-built-linux, Dmitry Safonov,
linux-arm-kernel, linux-kernel, linux-mediatek, Lvqiang Huang,
Matthias Brugger, Nick Desaulniers, Miles Chen
We received a report of boot failure on stable/4.14.y using Clang with
CONFIG_UNWINDER_FRAME_POINTER. Turns out, this was a cascaded failure
with at least 4 different things going wrong. Working backwards from the
failure:
4) There was no fixup handler in backtrace-clang.S for a specific
address calculation. If an indirect access to that address triggers a
page fault for which no corresponding fixup exists, then a panic is
triggered. Panicking triggers another unwind, and all this repeats in an
infinite loop. If the unwind was started from within start_kernel(),
this results in a kernel that does not boot. We can install a fixup
handler to fix the infinite loop, but why was the unwinder accessing an
address that would trigger a fault?
3) The unwinder has multiple conditions to know when to stop unwinding,
but checking for a valid address in the link register was not one of
them. If there was a value for lr that we could check for before using
it, then we could add that as another stopping condition to terminate
unwinding. But the garbage value in lr in the case of save_stack()
wasn't particularly noteworthy from any other address; it was ambiguous
whether we had more frames to continue unwinding through or not, but
what value would we check for?
2) When following a frame chain, we can generally follow the addresses
pushed onto the stack from the link register, lr. The callee generally
pushes this value. For our particular failure, the value in the link
register upon entry to save_stack() was garbage. The caller,
__mmap_switched, does a tail call into save_stack() since we don't plan
to return control flow back to __mmap_switched. It uses a `b` (branch)
instruction rather than a `bl` (branch+link) which is correct, since
there are no instructions after the `b save_stack` in __mmap_switched.
If we interpret the value of lr that was pushed on the stack in
save_stack(), then it appears that we have further frames to unwind.
When observing an unwind on mainline though, lr upon entry to
save_stack() was 0x00!
It turns out that this exact ambiguity was found+fixed already by
upstream
commit 59b6359dd92d ("ARM: 8702/1: head-common.S: Clear lr before jumping to start_kernel()")
which landed in 4.15-rc1 but was not yet backported to stable/4.14.y.
Sent to stable in:
https://lore.kernel.org/stable/20200730180340.1724137-1-ndesaulniers@google.com/T/#u
That gives us a value in lr upon entry to save_stack() that's noteworthy
as a terminal condition during unwinding. But why did we start
unwinding in start_kernel() in the first place?
1) A simple WARN_ON_ONCE was being triggered during start_kernel() due
to another patch that landed in v4.15-rc9 but wasn't backported to
stable/4.14.y. Sent to stable in:
https://lore.kernel.org/stable/20200727191746.3644844-1-ndesaulniers@google.com/T/#u
Read (or unwound; pun intended) in the order 1), 2), 3), 4) explains the
cascading chain of failures that lead to a kernel not booting.
Patch 0001 fixes 3) by adding a test for NULL to the conditions to stop
unwinding. This prevents the cascade from going further.
Patch 0002 fixes 4) by adding a fixup handler. It's not strictly
necessary right now, but I get the feeling that we might not be able to
trust the value of the link register pushed on the stack. I'm guessing
a stack buffer overflow could overwrite this value. Either way,
triggering an exception during unwind should be prevented at all costs
to avoid infinite loops.
Patches 0003/0004 are cleanup/bikeshed, feel free to NACK them and I
don't mind dropping them. They're just minor touchups I felt helped
readability from when I was debugging these. 0001 (and slightly so 0002)
are the only patches I really care about.
Nick Desaulniers (4):
ARM: backtrace-clang: check for NULL lr
ARM: backtrace-clang: add fixup for lr dereference
ARM: backtrace-clang: give labels more descriptive names
ARM: backtrace: use more descriptive labels
arch/arm/lib/backtrace-clang.S | 34 +++++++++++++++++++++-------------
arch/arm/lib/backtrace.S | 30 +++++++++++++++---------------
2 files changed, 36 insertions(+), 28 deletions(-)
--
2.28.0.163.g6104cc2f0b6-goog
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH 0/4] CONFIG_UNWINDER_FRAME_POINTER fixes+cleanups
@ 2020-07-30 20:51 ` Nick Desaulniers
0 siblings, 0 replies; 40+ messages in thread
From: Nick Desaulniers @ 2020-07-30 20:51 UTC (permalink / raw
To: Nathan Huckleberry, Russell King
Cc: Nick Desaulniers, Chunyan Zhang, Dmitry Safonov, linux-kernel,
clang-built-linux, Miles Chen, linux-mediatek, Matthias Brugger,
Andrew Morton, Lvqiang Huang, linux-arm-kernel
We received a report of boot failure on stable/4.14.y using Clang with
CONFIG_UNWINDER_FRAME_POINTER. Turns out, this was a cascaded failure
with at least 4 different things going wrong. Working backwards from the
failure:
4) There was no fixup handler in backtrace-clang.S for a specific
address calculation. If an indirect access to that address triggers a
page fault for which no corresponding fixup exists, then a panic is
triggered. Panicking triggers another unwind, and all this repeats in an
infinite loop. If the unwind was started from within start_kernel(),
this results in a kernel that does not boot. We can install a fixup
handler to fix the infinite loop, but why was the unwinder accessing an
address that would trigger a fault?
3) The unwinder has multiple conditions to know when to stop unwinding,
but checking for a valid address in the link register was not one of
them. If there was a value for lr that we could check for before using
it, then we could add that as another stopping condition to terminate
unwinding. But the garbage value in lr in the case of save_stack()
wasn't particularly noteworthy from any other address; it was ambiguous
whether we had more frames to continue unwinding through or not, but
what value would we check for?
2) When following a frame chain, we can generally follow the addresses
pushed onto the stack from the link register, lr. The callee generally
pushes this value. For our particular failure, the value in the link
register upon entry to save_stack() was garbage. The caller,
__mmap_switched, does a tail call into save_stack() since we don't plan
to return control flow back to __mmap_switched. It uses a `b` (branch)
instruction rather than a `bl` (branch+link) which is correct, since
there are no instructions after the `b save_stack` in __mmap_switched.
If we interpret the value of lr that was pushed on the stack in
save_stack(), then it appears that we have further frames to unwind.
When observing an unwind on mainline though, lr upon entry to
save_stack() was 0x00!
It turns out that this exact ambiguity was found+fixed already by
upstream
commit 59b6359dd92d ("ARM: 8702/1: head-common.S: Clear lr before jumping to start_kernel()")
which landed in 4.15-rc1 but was not yet backported to stable/4.14.y.
Sent to stable in:
https://lore.kernel.org/stable/20200730180340.1724137-1-ndesaulniers@google.com/T/#u
That gives us a value in lr upon entry to save_stack() that's noteworthy
as a terminal condition during unwinding. But why did we start
unwinding in start_kernel() in the first place?
1) A simple WARN_ON_ONCE was being triggered during start_kernel() due
to another patch that landed in v4.15-rc9 but wasn't backported to
stable/4.14.y. Sent to stable in:
https://lore.kernel.org/stable/20200727191746.3644844-1-ndesaulniers@google.com/T/#u
Read (or unwound; pun intended) in the order 1), 2), 3), 4) explains the
cascading chain of failures that lead to a kernel not booting.
Patch 0001 fixes 3) by adding a test for NULL to the conditions to stop
unwinding. This prevents the cascade from going further.
Patch 0002 fixes 4) by adding a fixup handler. It's not strictly
necessary right now, but I get the feeling that we might not be able to
trust the value of the link register pushed on the stack. I'm guessing
a stack buffer overflow could overwrite this value. Either way,
triggering an exception during unwind should be prevented at all costs
to avoid infinite loops.
Patches 0003/0004 are cleanup/bikeshed, feel free to NACK them and I
don't mind dropping them. They're just minor touchups I felt helped
readability from when I was debugging these. 0001 (and slightly so 0002)
are the only patches I really care about.
Nick Desaulniers (4):
ARM: backtrace-clang: check for NULL lr
ARM: backtrace-clang: add fixup for lr dereference
ARM: backtrace-clang: give labels more descriptive names
ARM: backtrace: use more descriptive labels
arch/arm/lib/backtrace-clang.S | 34 +++++++++++++++++++++-------------
arch/arm/lib/backtrace.S | 30 +++++++++++++++---------------
2 files changed, 36 insertions(+), 28 deletions(-)
--
2.28.0.163.g6104cc2f0b6-goog
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH 0/4] CONFIG_UNWINDER_FRAME_POINTER fixes+cleanups
@ 2020-07-30 20:51 ` Nick Desaulniers
0 siblings, 0 replies; 40+ messages in thread
From: Nick Desaulniers @ 2020-07-30 20:51 UTC (permalink / raw
To: Nathan Huckleberry, Russell King
Cc: Nick Desaulniers, Chunyan Zhang, Dmitry Safonov, linux-kernel,
clang-built-linux, Miles Chen, linux-mediatek, Matthias Brugger,
Andrew Morton, Lvqiang Huang, linux-arm-kernel
We received a report of boot failure on stable/4.14.y using Clang with
CONFIG_UNWINDER_FRAME_POINTER. Turns out, this was a cascaded failure
with at least 4 different things going wrong. Working backwards from the
failure:
4) There was no fixup handler in backtrace-clang.S for a specific
address calculation. If an indirect access to that address triggers a
page fault for which no corresponding fixup exists, then a panic is
triggered. Panicking triggers another unwind, and all this repeats in an
infinite loop. If the unwind was started from within start_kernel(),
this results in a kernel that does not boot. We can install a fixup
handler to fix the infinite loop, but why was the unwinder accessing an
address that would trigger a fault?
3) The unwinder has multiple conditions to know when to stop unwinding,
but checking for a valid address in the link register was not one of
them. If there was a value for lr that we could check for before using
it, then we could add that as another stopping condition to terminate
unwinding. But the garbage value in lr in the case of save_stack()
wasn't particularly noteworthy from any other address; it was ambiguous
whether we had more frames to continue unwinding through or not, but
what value would we check for?
2) When following a frame chain, we can generally follow the addresses
pushed onto the stack from the link register, lr. The callee generally
pushes this value. For our particular failure, the value in the link
register upon entry to save_stack() was garbage. The caller,
__mmap_switched, does a tail call into save_stack() since we don't plan
to return control flow back to __mmap_switched. It uses a `b` (branch)
instruction rather than a `bl` (branch+link) which is correct, since
there are no instructions after the `b save_stack` in __mmap_switched.
If we interpret the value of lr that was pushed on the stack in
save_stack(), then it appears that we have further frames to unwind.
When observing an unwind on mainline though, lr upon entry to
save_stack() was 0x00!
It turns out that this exact ambiguity was found+fixed already by
upstream
commit 59b6359dd92d ("ARM: 8702/1: head-common.S: Clear lr before jumping to start_kernel()")
which landed in 4.15-rc1 but was not yet backported to stable/4.14.y.
Sent to stable in:
https://lore.kernel.org/stable/20200730180340.1724137-1-ndesaulniers@google.com/T/#u
That gives us a value in lr upon entry to save_stack() that's noteworthy
as a terminal condition during unwinding. But why did we start
unwinding in start_kernel() in the first place?
1) A simple WARN_ON_ONCE was being triggered during start_kernel() due
to another patch that landed in v4.15-rc9 but wasn't backported to
stable/4.14.y. Sent to stable in:
https://lore.kernel.org/stable/20200727191746.3644844-1-ndesaulniers@google.com/T/#u
Read (or unwound; pun intended) in the order 1), 2), 3), 4) explains the
cascading chain of failures that lead to a kernel not booting.
Patch 0001 fixes 3) by adding a test for NULL to the conditions to stop
unwinding. This prevents the cascade from going further.
Patch 0002 fixes 4) by adding a fixup handler. It's not strictly
necessary right now, but I get the feeling that we might not be able to
trust the value of the link register pushed on the stack. I'm guessing
a stack buffer overflow could overwrite this value. Either way,
triggering an exception during unwind should be prevented at all costs
to avoid infinite loops.
Patches 0003/0004 are cleanup/bikeshed, feel free to NACK them and I
don't mind dropping them. They're just minor touchups I felt helped
readability from when I was debugging these. 0001 (and slightly so 0002)
are the only patches I really care about.
Nick Desaulniers (4):
ARM: backtrace-clang: check for NULL lr
ARM: backtrace-clang: add fixup for lr dereference
ARM: backtrace-clang: give labels more descriptive names
ARM: backtrace: use more descriptive labels
arch/arm/lib/backtrace-clang.S | 34 +++++++++++++++++++++-------------
arch/arm/lib/backtrace.S | 30 +++++++++++++++---------------
2 files changed, 36 insertions(+), 28 deletions(-)
--
2.28.0.163.g6104cc2f0b6-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH 1/4] ARM: backtrace-clang: check for NULL lr
2020-07-30 20:51 ` Nick Desaulniers
(?)
@ 2020-07-30 20:51 ` Nick Desaulniers
-1 siblings, 0 replies; 40+ messages in thread
From: Nick Desaulniers @ 2020-07-30 20:51 UTC (permalink / raw
To: Nathan Huckleberry, Russell King
Cc: Andrew Morton, Chunyan Zhang, clang-built-linux, Dmitry Safonov,
linux-arm-kernel, linux-kernel, linux-mediatek, Lvqiang Huang,
Matthias Brugger, Nick Desaulniers, Miles Chen, stable
If the link register was zeroed out, do not attempt to use it for
address calculations for which there are currently no fixup handlers,
which can lead to a panic during unwind. Since panicking triggers
another unwind, this can lead to an infinite loop. If this occurs
during start_kernel(), this can prevent a kernel from booting.
commit 59b6359dd92d ("ARM: 8702/1: head-common.S: Clear lr before jumping to start_kernel()")
intentionally zeros out the link register in __mmap_switched which tail
calls into start kernel. Test for this condition so that we can stop
unwinding when initiated within start_kernel() correctly.
Cc: stable@vger.kernel.org
Fixes: commit 6dc5fd93b2f1 ("ARM: 8900/1: UNWINDER_FRAME_POINTER implementation for Clang")
Reported-by: Miles Chen <miles.chen@mediatek.com>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
arch/arm/lib/backtrace-clang.S | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm/lib/backtrace-clang.S b/arch/arm/lib/backtrace-clang.S
index 6174c45f53a5..5388ac664c12 100644
--- a/arch/arm/lib/backtrace-clang.S
+++ b/arch/arm/lib/backtrace-clang.S
@@ -144,6 +144,8 @@ for_each_frame: tst frame, mask @ Check for address exceptions
*/
1003: ldr sv_lr, [sv_fp, #4] @ get saved lr from next frame
+ tst sv_lr, #0 @ If there's no previous lr,
+ beq finished_setup @ we're done.
ldr r0, [sv_lr, #-4] @ get call instruction
ldr r3, .Lopcode+4
and r2, r3, r0 @ is this a bl call
--
2.28.0.163.g6104cc2f0b6-goog
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH 1/4] ARM: backtrace-clang: check for NULL lr
@ 2020-07-30 20:51 ` Nick Desaulniers
0 siblings, 0 replies; 40+ messages in thread
From: Nick Desaulniers @ 2020-07-30 20:51 UTC (permalink / raw
To: Nathan Huckleberry, Russell King
Cc: Nick Desaulniers, Chunyan Zhang, Dmitry Safonov, linux-kernel,
stable, clang-built-linux, Miles Chen, linux-mediatek,
Matthias Brugger, Andrew Morton, Lvqiang Huang, linux-arm-kernel
If the link register was zeroed out, do not attempt to use it for
address calculations for which there are currently no fixup handlers,
which can lead to a panic during unwind. Since panicking triggers
another unwind, this can lead to an infinite loop. If this occurs
during start_kernel(), this can prevent a kernel from booting.
commit 59b6359dd92d ("ARM: 8702/1: head-common.S: Clear lr before jumping to start_kernel()")
intentionally zeros out the link register in __mmap_switched which tail
calls into start kernel. Test for this condition so that we can stop
unwinding when initiated within start_kernel() correctly.
Cc: stable@vger.kernel.org
Fixes: commit 6dc5fd93b2f1 ("ARM: 8900/1: UNWINDER_FRAME_POINTER implementation for Clang")
Reported-by: Miles Chen <miles.chen@mediatek.com>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
arch/arm/lib/backtrace-clang.S | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm/lib/backtrace-clang.S b/arch/arm/lib/backtrace-clang.S
index 6174c45f53a5..5388ac664c12 100644
--- a/arch/arm/lib/backtrace-clang.S
+++ b/arch/arm/lib/backtrace-clang.S
@@ -144,6 +144,8 @@ for_each_frame: tst frame, mask @ Check for address exceptions
*/
1003: ldr sv_lr, [sv_fp, #4] @ get saved lr from next frame
+ tst sv_lr, #0 @ If there's no previous lr,
+ beq finished_setup @ we're done.
ldr r0, [sv_lr, #-4] @ get call instruction
ldr r3, .Lopcode+4
and r2, r3, r0 @ is this a bl call
--
2.28.0.163.g6104cc2f0b6-goog
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH 1/4] ARM: backtrace-clang: check for NULL lr
@ 2020-07-30 20:51 ` Nick Desaulniers
0 siblings, 0 replies; 40+ messages in thread
From: Nick Desaulniers @ 2020-07-30 20:51 UTC (permalink / raw
To: Nathan Huckleberry, Russell King
Cc: Nick Desaulniers, Chunyan Zhang, Dmitry Safonov, linux-kernel,
stable, clang-built-linux, Miles Chen, linux-mediatek,
Matthias Brugger, Andrew Morton, Lvqiang Huang, linux-arm-kernel
If the link register was zeroed out, do not attempt to use it for
address calculations for which there are currently no fixup handlers,
which can lead to a panic during unwind. Since panicking triggers
another unwind, this can lead to an infinite loop. If this occurs
during start_kernel(), this can prevent a kernel from booting.
commit 59b6359dd92d ("ARM: 8702/1: head-common.S: Clear lr before jumping to start_kernel()")
intentionally zeros out the link register in __mmap_switched which tail
calls into start kernel. Test for this condition so that we can stop
unwinding when initiated within start_kernel() correctly.
Cc: stable@vger.kernel.org
Fixes: commit 6dc5fd93b2f1 ("ARM: 8900/1: UNWINDER_FRAME_POINTER implementation for Clang")
Reported-by: Miles Chen <miles.chen@mediatek.com>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
arch/arm/lib/backtrace-clang.S | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm/lib/backtrace-clang.S b/arch/arm/lib/backtrace-clang.S
index 6174c45f53a5..5388ac664c12 100644
--- a/arch/arm/lib/backtrace-clang.S
+++ b/arch/arm/lib/backtrace-clang.S
@@ -144,6 +144,8 @@ for_each_frame: tst frame, mask @ Check for address exceptions
*/
1003: ldr sv_lr, [sv_fp, #4] @ get saved lr from next frame
+ tst sv_lr, #0 @ If there's no previous lr,
+ beq finished_setup @ we're done.
ldr r0, [sv_lr, #-4] @ get call instruction
ldr r3, .Lopcode+4
and r2, r3, r0 @ is this a bl call
--
2.28.0.163.g6104cc2f0b6-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH 2/4] ARM: backtrace-clang: add fixup for lr dereference
2020-07-30 20:51 ` Nick Desaulniers
(?)
@ 2020-07-30 20:51 ` Nick Desaulniers
-1 siblings, 0 replies; 40+ messages in thread
From: Nick Desaulniers @ 2020-07-30 20:51 UTC (permalink / raw
To: Nathan Huckleberry, Russell King
Cc: Andrew Morton, Chunyan Zhang, clang-built-linux, Dmitry Safonov,
linux-arm-kernel, linux-kernel, linux-mediatek, Lvqiang Huang,
Matthias Brugger, Nick Desaulniers, Miles Chen, stable
If the value of the link register is not correct (tail call from asm
that didn't set it, stack corruption, memory no longer mapped), then
using it for an address calculation may trigger an exception. Without a
fixup handler, this will lead to a panic, which will unwind, which will
trigger the fault repeatedly in an infinite loop.
We don't observe such failures currently, but we have. Just to be safe,
add a fixup handler here so that at least we don't have an infinite
loop.
Cc: stable@vger.kernel.org
Fixes: commit 6dc5fd93b2f1 ("ARM: 8900/1: UNWINDER_FRAME_POINTER implementation for Clang")
Reported-by: Miles Chen <miles.chen@mediatek.com>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
arch/arm/lib/backtrace-clang.S | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/arch/arm/lib/backtrace-clang.S b/arch/arm/lib/backtrace-clang.S
index 5388ac664c12..40eb2215eaf4 100644
--- a/arch/arm/lib/backtrace-clang.S
+++ b/arch/arm/lib/backtrace-clang.S
@@ -146,7 +146,7 @@ for_each_frame: tst frame, mask @ Check for address exceptions
tst sv_lr, #0 @ If there's no previous lr,
beq finished_setup @ we're done.
- ldr r0, [sv_lr, #-4] @ get call instruction
+prev_call: ldr r0, [sv_lr, #-4] @ get call instruction
ldr r3, .Lopcode+4
and r2, r3, r0 @ is this a bl call
teq r2, r3
@@ -206,6 +206,13 @@ finished_setup:
mov r2, frame
bl printk
no_frame: ldmfd sp!, {r4 - r9, fp, pc}
+/*
+ * Accessing the address pointed to by the link register triggered an
+ * exception, don't try to unwind through it.
+ */
+bad_lr: mov sv_fp, #0
+ mov sv_lr, #0
+ b finished_setup
ENDPROC(c_backtrace)
.pushsection __ex_table,"a"
.align 3
@@ -214,6 +221,7 @@ ENDPROC(c_backtrace)
.long 1003b, 1006b
.long 1004b, 1006b
.long 1005b, 1006b
+ .long prev_call, bad_lr
.popsection
.Lbad: .asciz "%sBacktrace aborted due to bad frame pointer <%p>\n"
--
2.28.0.163.g6104cc2f0b6-goog
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH 2/4] ARM: backtrace-clang: add fixup for lr dereference
@ 2020-07-30 20:51 ` Nick Desaulniers
0 siblings, 0 replies; 40+ messages in thread
From: Nick Desaulniers @ 2020-07-30 20:51 UTC (permalink / raw
To: Nathan Huckleberry, Russell King
Cc: Nick Desaulniers, Chunyan Zhang, Dmitry Safonov, linux-kernel,
stable, clang-built-linux, Miles Chen, linux-mediatek,
Matthias Brugger, Andrew Morton, Lvqiang Huang, linux-arm-kernel
If the value of the link register is not correct (tail call from asm
that didn't set it, stack corruption, memory no longer mapped), then
using it for an address calculation may trigger an exception. Without a
fixup handler, this will lead to a panic, which will unwind, which will
trigger the fault repeatedly in an infinite loop.
We don't observe such failures currently, but we have. Just to be safe,
add a fixup handler here so that at least we don't have an infinite
loop.
Cc: stable@vger.kernel.org
Fixes: commit 6dc5fd93b2f1 ("ARM: 8900/1: UNWINDER_FRAME_POINTER implementation for Clang")
Reported-by: Miles Chen <miles.chen@mediatek.com>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
arch/arm/lib/backtrace-clang.S | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/arch/arm/lib/backtrace-clang.S b/arch/arm/lib/backtrace-clang.S
index 5388ac664c12..40eb2215eaf4 100644
--- a/arch/arm/lib/backtrace-clang.S
+++ b/arch/arm/lib/backtrace-clang.S
@@ -146,7 +146,7 @@ for_each_frame: tst frame, mask @ Check for address exceptions
tst sv_lr, #0 @ If there's no previous lr,
beq finished_setup @ we're done.
- ldr r0, [sv_lr, #-4] @ get call instruction
+prev_call: ldr r0, [sv_lr, #-4] @ get call instruction
ldr r3, .Lopcode+4
and r2, r3, r0 @ is this a bl call
teq r2, r3
@@ -206,6 +206,13 @@ finished_setup:
mov r2, frame
bl printk
no_frame: ldmfd sp!, {r4 - r9, fp, pc}
+/*
+ * Accessing the address pointed to by the link register triggered an
+ * exception, don't try to unwind through it.
+ */
+bad_lr: mov sv_fp, #0
+ mov sv_lr, #0
+ b finished_setup
ENDPROC(c_backtrace)
.pushsection __ex_table,"a"
.align 3
@@ -214,6 +221,7 @@ ENDPROC(c_backtrace)
.long 1003b, 1006b
.long 1004b, 1006b
.long 1005b, 1006b
+ .long prev_call, bad_lr
.popsection
.Lbad: .asciz "%sBacktrace aborted due to bad frame pointer <%p>\n"
--
2.28.0.163.g6104cc2f0b6-goog
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH 2/4] ARM: backtrace-clang: add fixup for lr dereference
@ 2020-07-30 20:51 ` Nick Desaulniers
0 siblings, 0 replies; 40+ messages in thread
From: Nick Desaulniers @ 2020-07-30 20:51 UTC (permalink / raw
To: Nathan Huckleberry, Russell King
Cc: Nick Desaulniers, Chunyan Zhang, Dmitry Safonov, linux-kernel,
stable, clang-built-linux, Miles Chen, linux-mediatek,
Matthias Brugger, Andrew Morton, Lvqiang Huang, linux-arm-kernel
If the value of the link register is not correct (tail call from asm
that didn't set it, stack corruption, memory no longer mapped), then
using it for an address calculation may trigger an exception. Without a
fixup handler, this will lead to a panic, which will unwind, which will
trigger the fault repeatedly in an infinite loop.
We don't observe such failures currently, but we have. Just to be safe,
add a fixup handler here so that at least we don't have an infinite
loop.
Cc: stable@vger.kernel.org
Fixes: commit 6dc5fd93b2f1 ("ARM: 8900/1: UNWINDER_FRAME_POINTER implementation for Clang")
Reported-by: Miles Chen <miles.chen@mediatek.com>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
arch/arm/lib/backtrace-clang.S | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/arch/arm/lib/backtrace-clang.S b/arch/arm/lib/backtrace-clang.S
index 5388ac664c12..40eb2215eaf4 100644
--- a/arch/arm/lib/backtrace-clang.S
+++ b/arch/arm/lib/backtrace-clang.S
@@ -146,7 +146,7 @@ for_each_frame: tst frame, mask @ Check for address exceptions
tst sv_lr, #0 @ If there's no previous lr,
beq finished_setup @ we're done.
- ldr r0, [sv_lr, #-4] @ get call instruction
+prev_call: ldr r0, [sv_lr, #-4] @ get call instruction
ldr r3, .Lopcode+4
and r2, r3, r0 @ is this a bl call
teq r2, r3
@@ -206,6 +206,13 @@ finished_setup:
mov r2, frame
bl printk
no_frame: ldmfd sp!, {r4 - r9, fp, pc}
+/*
+ * Accessing the address pointed to by the link register triggered an
+ * exception, don't try to unwind through it.
+ */
+bad_lr: mov sv_fp, #0
+ mov sv_lr, #0
+ b finished_setup
ENDPROC(c_backtrace)
.pushsection __ex_table,"a"
.align 3
@@ -214,6 +221,7 @@ ENDPROC(c_backtrace)
.long 1003b, 1006b
.long 1004b, 1006b
.long 1005b, 1006b
+ .long prev_call, bad_lr
.popsection
.Lbad: .asciz "%sBacktrace aborted due to bad frame pointer <%p>\n"
--
2.28.0.163.g6104cc2f0b6-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH 3/4] ARM: backtrace-clang: give labels more descriptive names
2020-07-30 20:51 ` Nick Desaulniers
(?)
@ 2020-07-30 20:51 ` Nick Desaulniers
-1 siblings, 0 replies; 40+ messages in thread
From: Nick Desaulniers @ 2020-07-30 20:51 UTC (permalink / raw
To: Nathan Huckleberry, Russell King
Cc: Andrew Morton, Chunyan Zhang, clang-built-linux, Dmitry Safonov,
linux-arm-kernel, linux-kernel, linux-mediatek, Lvqiang Huang,
Matthias Brugger, Nick Desaulniers, Miles Chen
Removes the 1004 label; it was neither a control flow target, nor an
instruction we expect to produce a fault.
Gives the labels slightly more readable names. The `b` suffixes are
handy to disambiguate between labels of the same identifier when there's
more than one. Since these labels are unique, let's just give them
names.
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
arch/arm/lib/backtrace-clang.S | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/arch/arm/lib/backtrace-clang.S b/arch/arm/lib/backtrace-clang.S
index 40eb2215eaf4..7dad2a6843a5 100644
--- a/arch/arm/lib/backtrace-clang.S
+++ b/arch/arm/lib/backtrace-clang.S
@@ -121,8 +121,8 @@ for_each_frame: tst frame, mask @ Check for address exceptions
* start. This value gets updated to be the function start later if it is
* possible.
*/
-1001: ldr sv_pc, [frame, #4] @ get saved 'pc'
-1002: ldr sv_fp, [frame, #0] @ get saved fp
+load_pc: ldr sv_pc, [frame, #4] @ get saved 'pc'
+load_fp: ldr sv_fp, [frame, #0] @ get saved fp
teq sv_fp, mask @ make sure next frame exists
beq no_frame
@@ -142,7 +142,7 @@ for_each_frame: tst frame, mask @ Check for address exceptions
* registers for the current function, but the stacktrace is still printed
* properly.
*/
-1003: ldr sv_lr, [sv_fp, #4] @ get saved lr from next frame
+load_lr: ldr sv_lr, [sv_fp, #4] @ get saved lr from next frame
tst sv_lr, #0 @ If there's no previous lr,
beq finished_setup @ we're done.
@@ -166,8 +166,7 @@ finished_setup:
/*
* Print the function (sv_pc) and where it was called from (sv_lr).
*/
-1004: mov r0, sv_pc
-
+ mov r0, sv_pc
mov r1, sv_lr
mov r2, frame
bic r1, r1, mask @ mask PC/LR for the mode
@@ -182,7 +181,7 @@ finished_setup:
* pointer the comparison will fail and no registers will print. Unwinding will
* continue as if there had been no registers stored in this frame.
*/
-1005: ldr r1, [sv_pc, #0] @ if stmfd sp!, {..., fp, lr}
+load_stmfd: ldr r1, [sv_pc, #0] @ if stmfd sp!, {..., fp, lr}
ldr r3, .Lopcode @ instruction exists,
teq r3, r1, lsr #11
ldr r0, [frame] @ locals are stored in
@@ -201,7 +200,7 @@ finished_setup:
mov frame, sv_fp @ above the current frame
bhi for_each_frame
-1006: adr r0, .Lbad
+bad_frame: adr r0, .Lbad
mov r1, loglvl
mov r2, frame
bl printk
@@ -216,11 +215,10 @@ bad_lr: mov sv_fp, #0
ENDPROC(c_backtrace)
.pushsection __ex_table,"a"
.align 3
- .long 1001b, 1006b
- .long 1002b, 1006b
- .long 1003b, 1006b
- .long 1004b, 1006b
- .long 1005b, 1006b
+ .long load_pc, bad_frame
+ .long load_fp, bad_frame
+ .long load_lr, bad_frame
+ .long load_stmfd, bad_frame
.long prev_call, bad_lr
.popsection
--
2.28.0.163.g6104cc2f0b6-goog
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH 3/4] ARM: backtrace-clang: give labels more descriptive names
@ 2020-07-30 20:51 ` Nick Desaulniers
0 siblings, 0 replies; 40+ messages in thread
From: Nick Desaulniers @ 2020-07-30 20:51 UTC (permalink / raw
To: Nathan Huckleberry, Russell King
Cc: Nick Desaulniers, Chunyan Zhang, Dmitry Safonov, linux-kernel,
clang-built-linux, Miles Chen, linux-mediatek, Matthias Brugger,
Andrew Morton, Lvqiang Huang, linux-arm-kernel
Removes the 1004 label; it was neither a control flow target, nor an
instruction we expect to produce a fault.
Gives the labels slightly more readable names. The `b` suffixes are
handy to disambiguate between labels of the same identifier when there's
more than one. Since these labels are unique, let's just give them
names.
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
arch/arm/lib/backtrace-clang.S | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/arch/arm/lib/backtrace-clang.S b/arch/arm/lib/backtrace-clang.S
index 40eb2215eaf4..7dad2a6843a5 100644
--- a/arch/arm/lib/backtrace-clang.S
+++ b/arch/arm/lib/backtrace-clang.S
@@ -121,8 +121,8 @@ for_each_frame: tst frame, mask @ Check for address exceptions
* start. This value gets updated to be the function start later if it is
* possible.
*/
-1001: ldr sv_pc, [frame, #4] @ get saved 'pc'
-1002: ldr sv_fp, [frame, #0] @ get saved fp
+load_pc: ldr sv_pc, [frame, #4] @ get saved 'pc'
+load_fp: ldr sv_fp, [frame, #0] @ get saved fp
teq sv_fp, mask @ make sure next frame exists
beq no_frame
@@ -142,7 +142,7 @@ for_each_frame: tst frame, mask @ Check for address exceptions
* registers for the current function, but the stacktrace is still printed
* properly.
*/
-1003: ldr sv_lr, [sv_fp, #4] @ get saved lr from next frame
+load_lr: ldr sv_lr, [sv_fp, #4] @ get saved lr from next frame
tst sv_lr, #0 @ If there's no previous lr,
beq finished_setup @ we're done.
@@ -166,8 +166,7 @@ finished_setup:
/*
* Print the function (sv_pc) and where it was called from (sv_lr).
*/
-1004: mov r0, sv_pc
-
+ mov r0, sv_pc
mov r1, sv_lr
mov r2, frame
bic r1, r1, mask @ mask PC/LR for the mode
@@ -182,7 +181,7 @@ finished_setup:
* pointer the comparison will fail and no registers will print. Unwinding will
* continue as if there had been no registers stored in this frame.
*/
-1005: ldr r1, [sv_pc, #0] @ if stmfd sp!, {..., fp, lr}
+load_stmfd: ldr r1, [sv_pc, #0] @ if stmfd sp!, {..., fp, lr}
ldr r3, .Lopcode @ instruction exists,
teq r3, r1, lsr #11
ldr r0, [frame] @ locals are stored in
@@ -201,7 +200,7 @@ finished_setup:
mov frame, sv_fp @ above the current frame
bhi for_each_frame
-1006: adr r0, .Lbad
+bad_frame: adr r0, .Lbad
mov r1, loglvl
mov r2, frame
bl printk
@@ -216,11 +215,10 @@ bad_lr: mov sv_fp, #0
ENDPROC(c_backtrace)
.pushsection __ex_table,"a"
.align 3
- .long 1001b, 1006b
- .long 1002b, 1006b
- .long 1003b, 1006b
- .long 1004b, 1006b
- .long 1005b, 1006b
+ .long load_pc, bad_frame
+ .long load_fp, bad_frame
+ .long load_lr, bad_frame
+ .long load_stmfd, bad_frame
.long prev_call, bad_lr
.popsection
--
2.28.0.163.g6104cc2f0b6-goog
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH 3/4] ARM: backtrace-clang: give labels more descriptive names
@ 2020-07-30 20:51 ` Nick Desaulniers
0 siblings, 0 replies; 40+ messages in thread
From: Nick Desaulniers @ 2020-07-30 20:51 UTC (permalink / raw
To: Nathan Huckleberry, Russell King
Cc: Nick Desaulniers, Chunyan Zhang, Dmitry Safonov, linux-kernel,
clang-built-linux, Miles Chen, linux-mediatek, Matthias Brugger,
Andrew Morton, Lvqiang Huang, linux-arm-kernel
Removes the 1004 label; it was neither a control flow target, nor an
instruction we expect to produce a fault.
Gives the labels slightly more readable names. The `b` suffixes are
handy to disambiguate between labels of the same identifier when there's
more than one. Since these labels are unique, let's just give them
names.
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
arch/arm/lib/backtrace-clang.S | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/arch/arm/lib/backtrace-clang.S b/arch/arm/lib/backtrace-clang.S
index 40eb2215eaf4..7dad2a6843a5 100644
--- a/arch/arm/lib/backtrace-clang.S
+++ b/arch/arm/lib/backtrace-clang.S
@@ -121,8 +121,8 @@ for_each_frame: tst frame, mask @ Check for address exceptions
* start. This value gets updated to be the function start later if it is
* possible.
*/
-1001: ldr sv_pc, [frame, #4] @ get saved 'pc'
-1002: ldr sv_fp, [frame, #0] @ get saved fp
+load_pc: ldr sv_pc, [frame, #4] @ get saved 'pc'
+load_fp: ldr sv_fp, [frame, #0] @ get saved fp
teq sv_fp, mask @ make sure next frame exists
beq no_frame
@@ -142,7 +142,7 @@ for_each_frame: tst frame, mask @ Check for address exceptions
* registers for the current function, but the stacktrace is still printed
* properly.
*/
-1003: ldr sv_lr, [sv_fp, #4] @ get saved lr from next frame
+load_lr: ldr sv_lr, [sv_fp, #4] @ get saved lr from next frame
tst sv_lr, #0 @ If there's no previous lr,
beq finished_setup @ we're done.
@@ -166,8 +166,7 @@ finished_setup:
/*
* Print the function (sv_pc) and where it was called from (sv_lr).
*/
-1004: mov r0, sv_pc
-
+ mov r0, sv_pc
mov r1, sv_lr
mov r2, frame
bic r1, r1, mask @ mask PC/LR for the mode
@@ -182,7 +181,7 @@ finished_setup:
* pointer the comparison will fail and no registers will print. Unwinding will
* continue as if there had been no registers stored in this frame.
*/
-1005: ldr r1, [sv_pc, #0] @ if stmfd sp!, {..., fp, lr}
+load_stmfd: ldr r1, [sv_pc, #0] @ if stmfd sp!, {..., fp, lr}
ldr r3, .Lopcode @ instruction exists,
teq r3, r1, lsr #11
ldr r0, [frame] @ locals are stored in
@@ -201,7 +200,7 @@ finished_setup:
mov frame, sv_fp @ above the current frame
bhi for_each_frame
-1006: adr r0, .Lbad
+bad_frame: adr r0, .Lbad
mov r1, loglvl
mov r2, frame
bl printk
@@ -216,11 +215,10 @@ bad_lr: mov sv_fp, #0
ENDPROC(c_backtrace)
.pushsection __ex_table,"a"
.align 3
- .long 1001b, 1006b
- .long 1002b, 1006b
- .long 1003b, 1006b
- .long 1004b, 1006b
- .long 1005b, 1006b
+ .long load_pc, bad_frame
+ .long load_fp, bad_frame
+ .long load_lr, bad_frame
+ .long load_stmfd, bad_frame
.long prev_call, bad_lr
.popsection
--
2.28.0.163.g6104cc2f0b6-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH 4/4] ARM: backtrace: use more descriptive labels
2020-07-30 20:51 ` Nick Desaulniers
(?)
@ 2020-07-30 20:51 ` Nick Desaulniers
-1 siblings, 0 replies; 40+ messages in thread
From: Nick Desaulniers @ 2020-07-30 20:51 UTC (permalink / raw
To: Nathan Huckleberry, Russell King
Cc: Andrew Morton, Chunyan Zhang, clang-built-linux, Dmitry Safonov,
linux-arm-kernel, linux-kernel, linux-mediatek, Lvqiang Huang,
Matthias Brugger, Nick Desaulniers, Miles Chen
We don't necessarily need the `b` suffixes used to disambiguate between
non-unique local labels. Give these labels more descriptive names.
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
arch/arm/lib/backtrace.S | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/arch/arm/lib/backtrace.S b/arch/arm/lib/backtrace.S
index 872f658638d9..138e961ff033 100644
--- a/arch/arm/lib/backtrace.S
+++ b/arch/arm/lib/backtrace.S
@@ -37,9 +37,9 @@ ENDPROC(c_backtrace)
THUMB( orreq mask, #0x03 )
movne mask, #0 @ mask for 32-bit
-1: stmfd sp!, {pc} @ calculate offset of PC stored
+store_pc: stmfd sp!, {pc} @ calculate offset of PC stored
ldr r0, [sp], #4 @ by stmfd for this CPU
- adr r1, 1b
+ adr r1, store_pc
sub offset, r0, r1
/*
@@ -60,14 +60,14 @@ ENDPROC(c_backtrace)
for_each_frame: tst frame, mask @ Check for address exceptions
bne no_frame
-1001: ldr sv_pc, [frame, #0] @ get saved pc
-1002: ldr sv_fp, [frame, #-12] @ get saved fp
+load_pc: ldr sv_pc, [frame, #0] @ get saved pc
+load_fp: ldr sv_fp, [frame, #-12] @ get saved fp
sub sv_pc, sv_pc, offset @ Correct PC for prefetching
bic sv_pc, sv_pc, mask @ mask PC/LR for the mode
-1003: ldr r2, [sv_pc, #-4] @ if stmfd sp!, {args} exists,
- ldr r3, .Ldsi+4 @ adjust saved 'pc' back one
+load_stmfd: ldr r2, [sv_pc, #-4] @ if stmfd sp!, {args} exists,
+ ldr r3, .Lopcode + 4 @ adjust saved 'pc' back one
teq r3, r2, lsr #11 @ instruction
subne r0, sv_pc, #4 @ allow for mov
subeq r0, sv_pc, #8 @ allow for mov + stmia
@@ -79,15 +79,15 @@ for_each_frame: tst frame, mask @ Check for address exceptions
bl dump_backtrace_entry
ldr r1, [sv_pc, #-4] @ if stmfd sp!, {args} exists,
- ldr r3, .Ldsi+4
+ ldr r3, .Lopcode + 4
teq r3, r1, lsr #11
ldreq r0, [frame, #-8] @ get sp
subeq r0, r0, #4 @ point at the last arg
mov r2, loglvl
bleq dump_backtrace_stm @ dump saved registers
-1004: ldr r1, [sv_pc, #0] @ if stmfd sp!, {..., fp, ip, lr, pc}
- ldr r3, .Ldsi @ instruction exists,
+reload_stmfd: ldr r1, [sv_pc, #0] @ if stmfd sp!, {..., fp, ip, lr, pc}
+ ldr r3, .Lopcode @ instruction exists,
teq r3, r1, lsr #11
subeq r0, frame, #16
mov r2, loglvl
@@ -100,7 +100,7 @@ for_each_frame: tst frame, mask @ Check for address exceptions
mov frame, sv_fp @ above the current frame
bhi for_each_frame
-1006: adr r0, .Lbad
+bad_frame: adr r0, .Lbad
mov r1, loglvl
mov r2, frame
bl printk
@@ -109,15 +109,15 @@ ENDPROC(c_backtrace)
.pushsection __ex_table,"a"
.align 3
- .long 1001b, 1006b
- .long 1002b, 1006b
- .long 1003b, 1006b
- .long 1004b, 1006b
+ .long load_pc, bad_frame
+ .long load_fp, bad_frame
+ .long load_stmfd, bad_frame
+ .long reload_stmfd, bad_frame
.popsection
.Lbad: .asciz "%sBacktrace aborted due to bad frame pointer <%p>\n"
.align
-.Ldsi: .word 0xe92dd800 >> 11 @ stmfd sp!, {... fp, ip, lr, pc}
+.Lopcode: .word 0xe92dd800 >> 11 @ stmfd sp!, {... fp, ip, lr, pc}
.word 0xe92d0000 >> 11 @ stmfd sp!, {}
#endif
--
2.28.0.163.g6104cc2f0b6-goog
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH 4/4] ARM: backtrace: use more descriptive labels
@ 2020-07-30 20:51 ` Nick Desaulniers
0 siblings, 0 replies; 40+ messages in thread
From: Nick Desaulniers @ 2020-07-30 20:51 UTC (permalink / raw
To: Nathan Huckleberry, Russell King
Cc: Nick Desaulniers, Chunyan Zhang, Dmitry Safonov, linux-kernel,
clang-built-linux, Miles Chen, linux-mediatek, Matthias Brugger,
Andrew Morton, Lvqiang Huang, linux-arm-kernel
We don't necessarily need the `b` suffixes used to disambiguate between
non-unique local labels. Give these labels more descriptive names.
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
arch/arm/lib/backtrace.S | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/arch/arm/lib/backtrace.S b/arch/arm/lib/backtrace.S
index 872f658638d9..138e961ff033 100644
--- a/arch/arm/lib/backtrace.S
+++ b/arch/arm/lib/backtrace.S
@@ -37,9 +37,9 @@ ENDPROC(c_backtrace)
THUMB( orreq mask, #0x03 )
movne mask, #0 @ mask for 32-bit
-1: stmfd sp!, {pc} @ calculate offset of PC stored
+store_pc: stmfd sp!, {pc} @ calculate offset of PC stored
ldr r0, [sp], #4 @ by stmfd for this CPU
- adr r1, 1b
+ adr r1, store_pc
sub offset, r0, r1
/*
@@ -60,14 +60,14 @@ ENDPROC(c_backtrace)
for_each_frame: tst frame, mask @ Check for address exceptions
bne no_frame
-1001: ldr sv_pc, [frame, #0] @ get saved pc
-1002: ldr sv_fp, [frame, #-12] @ get saved fp
+load_pc: ldr sv_pc, [frame, #0] @ get saved pc
+load_fp: ldr sv_fp, [frame, #-12] @ get saved fp
sub sv_pc, sv_pc, offset @ Correct PC for prefetching
bic sv_pc, sv_pc, mask @ mask PC/LR for the mode
-1003: ldr r2, [sv_pc, #-4] @ if stmfd sp!, {args} exists,
- ldr r3, .Ldsi+4 @ adjust saved 'pc' back one
+load_stmfd: ldr r2, [sv_pc, #-4] @ if stmfd sp!, {args} exists,
+ ldr r3, .Lopcode + 4 @ adjust saved 'pc' back one
teq r3, r2, lsr #11 @ instruction
subne r0, sv_pc, #4 @ allow for mov
subeq r0, sv_pc, #8 @ allow for mov + stmia
@@ -79,15 +79,15 @@ for_each_frame: tst frame, mask @ Check for address exceptions
bl dump_backtrace_entry
ldr r1, [sv_pc, #-4] @ if stmfd sp!, {args} exists,
- ldr r3, .Ldsi+4
+ ldr r3, .Lopcode + 4
teq r3, r1, lsr #11
ldreq r0, [frame, #-8] @ get sp
subeq r0, r0, #4 @ point at the last arg
mov r2, loglvl
bleq dump_backtrace_stm @ dump saved registers
-1004: ldr r1, [sv_pc, #0] @ if stmfd sp!, {..., fp, ip, lr, pc}
- ldr r3, .Ldsi @ instruction exists,
+reload_stmfd: ldr r1, [sv_pc, #0] @ if stmfd sp!, {..., fp, ip, lr, pc}
+ ldr r3, .Lopcode @ instruction exists,
teq r3, r1, lsr #11
subeq r0, frame, #16
mov r2, loglvl
@@ -100,7 +100,7 @@ for_each_frame: tst frame, mask @ Check for address exceptions
mov frame, sv_fp @ above the current frame
bhi for_each_frame
-1006: adr r0, .Lbad
+bad_frame: adr r0, .Lbad
mov r1, loglvl
mov r2, frame
bl printk
@@ -109,15 +109,15 @@ ENDPROC(c_backtrace)
.pushsection __ex_table,"a"
.align 3
- .long 1001b, 1006b
- .long 1002b, 1006b
- .long 1003b, 1006b
- .long 1004b, 1006b
+ .long load_pc, bad_frame
+ .long load_fp, bad_frame
+ .long load_stmfd, bad_frame
+ .long reload_stmfd, bad_frame
.popsection
.Lbad: .asciz "%sBacktrace aborted due to bad frame pointer <%p>\n"
.align
-.Ldsi: .word 0xe92dd800 >> 11 @ stmfd sp!, {... fp, ip, lr, pc}
+.Lopcode: .word 0xe92dd800 >> 11 @ stmfd sp!, {... fp, ip, lr, pc}
.word 0xe92d0000 >> 11 @ stmfd sp!, {}
#endif
--
2.28.0.163.g6104cc2f0b6-goog
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH 4/4] ARM: backtrace: use more descriptive labels
@ 2020-07-30 20:51 ` Nick Desaulniers
0 siblings, 0 replies; 40+ messages in thread
From: Nick Desaulniers @ 2020-07-30 20:51 UTC (permalink / raw
To: Nathan Huckleberry, Russell King
Cc: Nick Desaulniers, Chunyan Zhang, Dmitry Safonov, linux-kernel,
clang-built-linux, Miles Chen, linux-mediatek, Matthias Brugger,
Andrew Morton, Lvqiang Huang, linux-arm-kernel
We don't necessarily need the `b` suffixes used to disambiguate between
non-unique local labels. Give these labels more descriptive names.
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
arch/arm/lib/backtrace.S | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/arch/arm/lib/backtrace.S b/arch/arm/lib/backtrace.S
index 872f658638d9..138e961ff033 100644
--- a/arch/arm/lib/backtrace.S
+++ b/arch/arm/lib/backtrace.S
@@ -37,9 +37,9 @@ ENDPROC(c_backtrace)
THUMB( orreq mask, #0x03 )
movne mask, #0 @ mask for 32-bit
-1: stmfd sp!, {pc} @ calculate offset of PC stored
+store_pc: stmfd sp!, {pc} @ calculate offset of PC stored
ldr r0, [sp], #4 @ by stmfd for this CPU
- adr r1, 1b
+ adr r1, store_pc
sub offset, r0, r1
/*
@@ -60,14 +60,14 @@ ENDPROC(c_backtrace)
for_each_frame: tst frame, mask @ Check for address exceptions
bne no_frame
-1001: ldr sv_pc, [frame, #0] @ get saved pc
-1002: ldr sv_fp, [frame, #-12] @ get saved fp
+load_pc: ldr sv_pc, [frame, #0] @ get saved pc
+load_fp: ldr sv_fp, [frame, #-12] @ get saved fp
sub sv_pc, sv_pc, offset @ Correct PC for prefetching
bic sv_pc, sv_pc, mask @ mask PC/LR for the mode
-1003: ldr r2, [sv_pc, #-4] @ if stmfd sp!, {args} exists,
- ldr r3, .Ldsi+4 @ adjust saved 'pc' back one
+load_stmfd: ldr r2, [sv_pc, #-4] @ if stmfd sp!, {args} exists,
+ ldr r3, .Lopcode + 4 @ adjust saved 'pc' back one
teq r3, r2, lsr #11 @ instruction
subne r0, sv_pc, #4 @ allow for mov
subeq r0, sv_pc, #8 @ allow for mov + stmia
@@ -79,15 +79,15 @@ for_each_frame: tst frame, mask @ Check for address exceptions
bl dump_backtrace_entry
ldr r1, [sv_pc, #-4] @ if stmfd sp!, {args} exists,
- ldr r3, .Ldsi+4
+ ldr r3, .Lopcode + 4
teq r3, r1, lsr #11
ldreq r0, [frame, #-8] @ get sp
subeq r0, r0, #4 @ point at the last arg
mov r2, loglvl
bleq dump_backtrace_stm @ dump saved registers
-1004: ldr r1, [sv_pc, #0] @ if stmfd sp!, {..., fp, ip, lr, pc}
- ldr r3, .Ldsi @ instruction exists,
+reload_stmfd: ldr r1, [sv_pc, #0] @ if stmfd sp!, {..., fp, ip, lr, pc}
+ ldr r3, .Lopcode @ instruction exists,
teq r3, r1, lsr #11
subeq r0, frame, #16
mov r2, loglvl
@@ -100,7 +100,7 @@ for_each_frame: tst frame, mask @ Check for address exceptions
mov frame, sv_fp @ above the current frame
bhi for_each_frame
-1006: adr r0, .Lbad
+bad_frame: adr r0, .Lbad
mov r1, loglvl
mov r2, frame
bl printk
@@ -109,15 +109,15 @@ ENDPROC(c_backtrace)
.pushsection __ex_table,"a"
.align 3
- .long 1001b, 1006b
- .long 1002b, 1006b
- .long 1003b, 1006b
- .long 1004b, 1006b
+ .long load_pc, bad_frame
+ .long load_fp, bad_frame
+ .long load_stmfd, bad_frame
+ .long reload_stmfd, bad_frame
.popsection
.Lbad: .asciz "%sBacktrace aborted due to bad frame pointer <%p>\n"
.align
-.Ldsi: .word 0xe92dd800 >> 11 @ stmfd sp!, {... fp, ip, lr, pc}
+.Lopcode: .word 0xe92dd800 >> 11 @ stmfd sp!, {... fp, ip, lr, pc}
.word 0xe92d0000 >> 11 @ stmfd sp!, {}
#endif
--
2.28.0.163.g6104cc2f0b6-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH 2/4] ARM: backtrace-clang: add fixup for lr dereference
2020-07-30 20:51 ` Nick Desaulniers
(?)
(?)
@ 2020-08-01 23:18 ` Sasha Levin
2020-08-03 18:13 ` Nick Desaulniers
-1 siblings, 1 reply; 40+ messages in thread
From: Sasha Levin @ 2020-08-01 23:18 UTC (permalink / raw
To: Sasha Levin, Nick Desaulniers, Nathan Huckleberry
Cc: Andrew Morton, stable, stable
Hi
[This is an automated email]
This commit has been processed because it contains a "Fixes:" tag
fixing commit: 6dc5fd93b2f1 ("ARM: 8900/1: UNWINDER_FRAME_POINTER implementation for Clang").
The bot has tested the following trees: v5.7.11, v5.4.54.
v5.7.11: Failed to apply! Possible dependencies:
5489ab50c227 ("arm/asm: add loglvl to c_backtrace()")
99c56f602183 ("ARM: backtrace-clang: check for NULL lr")
v5.4.54: Failed to apply! Possible dependencies:
40ff1ddb5570 ("ARM: 8948/1: Prevent OOB access in stacktrace")
5489ab50c227 ("arm/asm: add loglvl to c_backtrace()")
99c56f602183 ("ARM: backtrace-clang: check for NULL lr")
NOTE: The patch will not be queued to stable trees until it is upstream.
How should we proceed with this patch?
--
Thanks
Sasha
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/4] ARM: backtrace-clang: add fixup for lr dereference
2020-08-01 23:18 ` Sasha Levin
@ 2020-08-03 18:13 ` Nick Desaulniers
2020-08-04 6:27 ` Greg KH
0 siblings, 1 reply; 40+ messages in thread
From: Nick Desaulniers @ 2020-08-03 18:13 UTC (permalink / raw
To: Sasha Levin, Greg KH; +Cc: Nathan Huckleberry, Andrew Morton, # 3.4.x
On Sat, Aug 1, 2020 at 4:18 PM Sasha Levin <sashal@kernel.org> wrote:
>
> Hi
>
> [This is an automated email]
>
> This commit has been processed because it contains a "Fixes:" tag
> fixing commit: 6dc5fd93b2f1 ("ARM: 8900/1: UNWINDER_FRAME_POINTER implementation for Clang").
>
> The bot has tested the following trees: v5.7.11, v5.4.54.
>
> v5.7.11: Failed to apply! Possible dependencies:
> 5489ab50c227 ("arm/asm: add loglvl to c_backtrace()")
> 99c56f602183 ("ARM: backtrace-clang: check for NULL lr")
>
> v5.4.54: Failed to apply! Possible dependencies:
> 40ff1ddb5570 ("ARM: 8948/1: Prevent OOB access in stacktrace")
> 5489ab50c227 ("arm/asm: add loglvl to c_backtrace()")
> 99c56f602183 ("ARM: backtrace-clang: check for NULL lr")
>
>
> NOTE: The patch will not be queued to stable trees until it is upstream.
>
> How should we proceed with this patch?
Ah, ok, I'll provide manual backports then once this hits mainline.
In that case, should I drop the explicit `Cc: stable...` tag?
(I don't think the dependency on the loglvl should be backported,
which is the source of conflict)
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/4] ARM: backtrace-clang: add fixup for lr dereference
2020-08-03 18:13 ` Nick Desaulniers
@ 2020-08-04 6:27 ` Greg KH
0 siblings, 0 replies; 40+ messages in thread
From: Greg KH @ 2020-08-04 6:27 UTC (permalink / raw
To: Nick Desaulniers; +Cc: Sasha Levin, Nathan Huckleberry, Andrew Morton, # 3.4.x
On Mon, Aug 03, 2020 at 11:13:04AM -0700, Nick Desaulniers wrote:
> On Sat, Aug 1, 2020 at 4:18 PM Sasha Levin <sashal@kernel.org> wrote:
> >
> > Hi
> >
> > [This is an automated email]
> >
> > This commit has been processed because it contains a "Fixes:" tag
> > fixing commit: 6dc5fd93b2f1 ("ARM: 8900/1: UNWINDER_FRAME_POINTER implementation for Clang").
> >
> > The bot has tested the following trees: v5.7.11, v5.4.54.
> >
> > v5.7.11: Failed to apply! Possible dependencies:
> > 5489ab50c227 ("arm/asm: add loglvl to c_backtrace()")
> > 99c56f602183 ("ARM: backtrace-clang: check for NULL lr")
> >
> > v5.4.54: Failed to apply! Possible dependencies:
> > 40ff1ddb5570 ("ARM: 8948/1: Prevent OOB access in stacktrace")
> > 5489ab50c227 ("arm/asm: add loglvl to c_backtrace()")
> > 99c56f602183 ("ARM: backtrace-clang: check for NULL lr")
> >
> >
> > NOTE: The patch will not be queued to stable trees until it is upstream.
> >
> > How should we proceed with this patch?
>
> Ah, ok, I'll provide manual backports then once this hits mainline.
> In that case, should I drop the explicit `Cc: stable...` tag?
No, it's good to have it there as then you get the automatic email
saying it failed to apply :)
thanks,
greg k-h
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/4] ARM: backtrace-clang: add fixup for lr dereference
2020-07-30 20:51 ` Nick Desaulniers
` (2 preceding siblings ...)
(?)
@ 2020-08-06 1:24 ` Sasha Levin
-1 siblings, 0 replies; 40+ messages in thread
From: Sasha Levin @ 2020-08-06 1:24 UTC (permalink / raw
To: Sasha Levin, Nick Desaulniers, Nathan Huckleberry
Cc: Andrew Morton, stable, stable
Hi
[This is an automated email]
This commit has been processed because it contains a "Fixes:" tag
fixing commit: 6dc5fd93b2f1 ("ARM: 8900/1: UNWINDER_FRAME_POINTER implementation for Clang").
The bot has tested the following trees: v5.7.11, v5.4.54.
v5.7.11: Failed to apply! Possible dependencies:
5489ab50c227 ("arm/asm: add loglvl to c_backtrace()")
e6902a275517 ("ARM: backtrace-clang: check for NULL lr")
v5.4.54: Failed to apply! Possible dependencies:
40ff1ddb5570 ("ARM: 8948/1: Prevent OOB access in stacktrace")
5489ab50c227 ("arm/asm: add loglvl to c_backtrace()")
e6902a275517 ("ARM: backtrace-clang: check for NULL lr")
NOTE: The patch will not be queued to stable trees until it is upstream.
How should we proceed with this patch?
--
Thanks
Sasha
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/4] CONFIG_UNWINDER_FRAME_POINTER fixes+cleanups
2020-07-30 20:51 ` Nick Desaulniers
` (5 preceding siblings ...)
(?)
@ 2020-08-06 1:24 ` Sasha Levin
-1 siblings, 0 replies; 40+ messages in thread
From: Sasha Levin @ 2020-08-06 1:24 UTC (permalink / raw
To: Sasha Levin, Nick Desaulniers, Nathan Huckleberry; +Cc: Andrew Morton, stable
Hi
[This is an automated email]
This commit has been processed because it contains a "Fixes:" tag
fixing commit: 6dc5fd93b2f1 ("ARM: 8900/1: UNWINDER_FRAME_POINTER implementation for Clang").
The bot has tested the following trees: v5.7.11, v5.4.54.
v5.7.11: Failed to apply! Possible dependencies:
5489ab50c227 ("arm/asm: add loglvl to c_backtrace()")
637ce97e7c24 ("ARM: backtrace-clang: check for NULL lr")
7c8ef99a0b04 ("ARM: backtrace-clang: add fixup for lr dereference")
v5.4.54: Failed to apply! Possible dependencies:
40ff1ddb5570 ("ARM: 8948/1: Prevent OOB access in stacktrace")
5489ab50c227 ("arm/asm: add loglvl to c_backtrace()")
637ce97e7c24 ("ARM: backtrace-clang: check for NULL lr")
7c8ef99a0b04 ("ARM: backtrace-clang: add fixup for lr dereference")
NOTE: The patch will not be queued to stable trees until it is upstream.
How should we proceed with this patch?
--
Thanks
Sasha
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/4] ARM: backtrace-clang: add fixup for lr dereference
2020-07-30 20:51 ` Nick Desaulniers
(?)
@ 2020-08-06 22:38 ` Nathan Huckleberry
-1 siblings, 0 replies; 40+ messages in thread
From: Nathan Huckleberry @ 2020-08-06 22:38 UTC (permalink / raw
To: Nick Desaulniers
Cc: Russell King, Andrew Morton, Chunyan Zhang, clang-built-linux,
Dmitry Safonov, linux-arm-kernel, linux-kernel, linux-mediatek,
Lvqiang Huang, Matthias Brugger, Miles Chen, stable,
Nathan Huckleberry
Mostly looks good to me. Just a minor nit.
On Thu, Jul 30, 2020 at 3:51 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> If the value of the link register is not correct (tail call from asm
> that didn't set it, stack corruption, memory no longer mapped), then
> using it for an address calculation may trigger an exception. Without a
> fixup handler, this will lead to a panic, which will unwind, which will
> trigger the fault repeatedly in an infinite loop.
>
> We don't observe such failures currently, but we have. Just to be safe,
> add a fixup handler here so that at least we don't have an infinite
> loop.
>
> Cc: stable@vger.kernel.org
> Fixes: commit 6dc5fd93b2f1 ("ARM: 8900/1: UNWINDER_FRAME_POINTER implementation for Clang")
> Reported-by: Miles Chen <miles.chen@mediatek.com>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> arch/arm/lib/backtrace-clang.S | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/lib/backtrace-clang.S b/arch/arm/lib/backtrace-clang.S
> index 5388ac664c12..40eb2215eaf4 100644
> --- a/arch/arm/lib/backtrace-clang.S
> +++ b/arch/arm/lib/backtrace-clang.S
> @@ -146,7 +146,7 @@ for_each_frame: tst frame, mask @ Check for address exceptions
>
> tst sv_lr, #0 @ If there's no previous lr,
> beq finished_setup @ we're done.
> - ldr r0, [sv_lr, #-4] @ get call instruction
> +prev_call: ldr r0, [sv_lr, #-4] @ get call instruction
> ldr r3, .Lopcode+4
> and r2, r3, r0 @ is this a bl call
> teq r2, r3
> @@ -206,6 +206,13 @@ finished_setup:
> mov r2, frame
> bl printk
> no_frame: ldmfd sp!, {r4 - r9, fp, pc}
> +/*
> + * Accessing the address pointed to by the link register triggered an
> + * exception, don't try to unwind through it.
> + */
> +bad_lr: mov sv_fp, #0
It might be nice to emit a warning here since we'll
only hit this case if something fishy is going on
with the saved lr.
> + mov sv_lr, #0
> + b finished_setup
> ENDPROC(c_backtrace)
> .pushsection __ex_table,"a"
> .align 3
> @@ -214,6 +221,7 @@ ENDPROC(c_backtrace)
> .long 1003b, 1006b
> .long 1004b, 1006b
> .long 1005b, 1006b
> + .long prev_call, bad_lr
> .popsection
>
> .Lbad: .asciz "%sBacktrace aborted due to bad frame pointer <%p>\n"
> --
> 2.28.0.163.g6104cc2f0b6-goog
>
Thanks,
Huck
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/4] ARM: backtrace-clang: add fixup for lr dereference
@ 2020-08-06 22:38 ` Nathan Huckleberry
0 siblings, 0 replies; 40+ messages in thread
From: Nathan Huckleberry @ 2020-08-06 22:38 UTC (permalink / raw
To: Nick Desaulniers
Cc: Chunyan Zhang, Dmitry Safonov, Russell King, stable, linux-kernel,
clang-built-linux, Miles Chen, linux-mediatek, Matthias Brugger,
Andrew Morton, Nathan Huckleberry, Lvqiang Huang,
linux-arm-kernel
Mostly looks good to me. Just a minor nit.
On Thu, Jul 30, 2020 at 3:51 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> If the value of the link register is not correct (tail call from asm
> that didn't set it, stack corruption, memory no longer mapped), then
> using it for an address calculation may trigger an exception. Without a
> fixup handler, this will lead to a panic, which will unwind, which will
> trigger the fault repeatedly in an infinite loop.
>
> We don't observe such failures currently, but we have. Just to be safe,
> add a fixup handler here so that at least we don't have an infinite
> loop.
>
> Cc: stable@vger.kernel.org
> Fixes: commit 6dc5fd93b2f1 ("ARM: 8900/1: UNWINDER_FRAME_POINTER implementation for Clang")
> Reported-by: Miles Chen <miles.chen@mediatek.com>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> arch/arm/lib/backtrace-clang.S | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/lib/backtrace-clang.S b/arch/arm/lib/backtrace-clang.S
> index 5388ac664c12..40eb2215eaf4 100644
> --- a/arch/arm/lib/backtrace-clang.S
> +++ b/arch/arm/lib/backtrace-clang.S
> @@ -146,7 +146,7 @@ for_each_frame: tst frame, mask @ Check for address exceptions
>
> tst sv_lr, #0 @ If there's no previous lr,
> beq finished_setup @ we're done.
> - ldr r0, [sv_lr, #-4] @ get call instruction
> +prev_call: ldr r0, [sv_lr, #-4] @ get call instruction
> ldr r3, .Lopcode+4
> and r2, r3, r0 @ is this a bl call
> teq r2, r3
> @@ -206,6 +206,13 @@ finished_setup:
> mov r2, frame
> bl printk
> no_frame: ldmfd sp!, {r4 - r9, fp, pc}
> +/*
> + * Accessing the address pointed to by the link register triggered an
> + * exception, don't try to unwind through it.
> + */
> +bad_lr: mov sv_fp, #0
It might be nice to emit a warning here since we'll
only hit this case if something fishy is going on
with the saved lr.
> + mov sv_lr, #0
> + b finished_setup
> ENDPROC(c_backtrace)
> .pushsection __ex_table,"a"
> .align 3
> @@ -214,6 +221,7 @@ ENDPROC(c_backtrace)
> .long 1003b, 1006b
> .long 1004b, 1006b
> .long 1005b, 1006b
> + .long prev_call, bad_lr
> .popsection
>
> .Lbad: .asciz "%sBacktrace aborted due to bad frame pointer <%p>\n"
> --
> 2.28.0.163.g6104cc2f0b6-goog
>
Thanks,
Huck
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/4] ARM: backtrace-clang: add fixup for lr dereference
@ 2020-08-06 22:38 ` Nathan Huckleberry
0 siblings, 0 replies; 40+ messages in thread
From: Nathan Huckleberry @ 2020-08-06 22:38 UTC (permalink / raw
To: Nick Desaulniers
Cc: Chunyan Zhang, Dmitry Safonov, Russell King, stable, linux-kernel,
clang-built-linux, Miles Chen, linux-mediatek, Matthias Brugger,
Andrew Morton, Nathan Huckleberry, Lvqiang Huang,
linux-arm-kernel
Mostly looks good to me. Just a minor nit.
On Thu, Jul 30, 2020 at 3:51 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> If the value of the link register is not correct (tail call from asm
> that didn't set it, stack corruption, memory no longer mapped), then
> using it for an address calculation may trigger an exception. Without a
> fixup handler, this will lead to a panic, which will unwind, which will
> trigger the fault repeatedly in an infinite loop.
>
> We don't observe such failures currently, but we have. Just to be safe,
> add a fixup handler here so that at least we don't have an infinite
> loop.
>
> Cc: stable@vger.kernel.org
> Fixes: commit 6dc5fd93b2f1 ("ARM: 8900/1: UNWINDER_FRAME_POINTER implementation for Clang")
> Reported-by: Miles Chen <miles.chen@mediatek.com>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> arch/arm/lib/backtrace-clang.S | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/lib/backtrace-clang.S b/arch/arm/lib/backtrace-clang.S
> index 5388ac664c12..40eb2215eaf4 100644
> --- a/arch/arm/lib/backtrace-clang.S
> +++ b/arch/arm/lib/backtrace-clang.S
> @@ -146,7 +146,7 @@ for_each_frame: tst frame, mask @ Check for address exceptions
>
> tst sv_lr, #0 @ If there's no previous lr,
> beq finished_setup @ we're done.
> - ldr r0, [sv_lr, #-4] @ get call instruction
> +prev_call: ldr r0, [sv_lr, #-4] @ get call instruction
> ldr r3, .Lopcode+4
> and r2, r3, r0 @ is this a bl call
> teq r2, r3
> @@ -206,6 +206,13 @@ finished_setup:
> mov r2, frame
> bl printk
> no_frame: ldmfd sp!, {r4 - r9, fp, pc}
> +/*
> + * Accessing the address pointed to by the link register triggered an
> + * exception, don't try to unwind through it.
> + */
> +bad_lr: mov sv_fp, #0
It might be nice to emit a warning here since we'll
only hit this case if something fishy is going on
with the saved lr.
> + mov sv_lr, #0
> + b finished_setup
> ENDPROC(c_backtrace)
> .pushsection __ex_table,"a"
> .align 3
> @@ -214,6 +221,7 @@ ENDPROC(c_backtrace)
> .long 1003b, 1006b
> .long 1004b, 1006b
> .long 1005b, 1006b
> + .long prev_call, bad_lr
> .popsection
>
> .Lbad: .asciz "%sBacktrace aborted due to bad frame pointer <%p>\n"
> --
> 2.28.0.163.g6104cc2f0b6-goog
>
Thanks,
Huck
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 3/4] ARM: backtrace-clang: give labels more descriptive names
2020-07-30 20:51 ` Nick Desaulniers
(?)
@ 2020-08-06 22:39 ` Nathan Huckleberry
-1 siblings, 0 replies; 40+ messages in thread
From: Nathan Huckleberry @ 2020-08-06 22:39 UTC (permalink / raw
To: Nick Desaulniers
Cc: Russell King, Andrew Morton, Chunyan Zhang, clang-built-linux,
Dmitry Safonov, linux-arm-kernel, linux-kernel, linux-mediatek,
Lvqiang Huang, Matthias Brugger, Miles Chen, Nathan Huckleberry
The style cleanup looks great. I just have one extra thing that
can probably be thrown into this patch.
On Thu, Jul 30, 2020 at 3:51 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> Removes the 1004 label; it was neither a control flow target, nor an
> instruction we expect to produce a fault.
>
> Gives the labels slightly more readable names. The `b` suffixes are
> handy to disambiguate between labels of the same identifier when there's
> more than one. Since these labels are unique, let's just give them
> names.
>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> arch/arm/lib/backtrace-clang.S | 22 ++++++++++------------
> 1 file changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm/lib/backtrace-clang.S b/arch/arm/lib/backtrace-clang.S
> index 40eb2215eaf4..7dad2a6843a5 100644
> --- a/arch/arm/lib/backtrace-clang.S
> +++ b/arch/arm/lib/backtrace-clang.S
> @@ -121,8 +121,8 @@ for_each_frame: tst frame, mask @ Check for address exceptions
> * start. This value gets updated to be the function start later if it is
> * possible.
> */
> -1001: ldr sv_pc, [frame, #4] @ get saved 'pc'
> -1002: ldr sv_fp, [frame, #0] @ get saved fp
> +load_pc: ldr sv_pc, [frame, #4] @ get saved 'pc'
> +load_fp: ldr sv_fp, [frame, #0] @ get saved fp
>
> teq sv_fp, mask @ make sure next frame exists
> beq no_frame
> @@ -142,7 +142,7 @@ for_each_frame: tst frame, mask @ Check for address exceptions
> * registers for the current function, but the stacktrace is still printed
> * properly.
> */
> -1003: ldr sv_lr, [sv_fp, #4] @ get saved lr from next frame
> +load_lr: ldr sv_lr, [sv_fp, #4] @ get saved lr from next frame
>
> tst sv_lr, #0 @ If there's no previous lr,
> beq finished_setup @ we're done.
> @@ -166,8 +166,7 @@ finished_setup:
> /*
> * Print the function (sv_pc) and where it was called from (sv_lr).
> */
> -1004: mov r0, sv_pc
> -
> + mov r0, sv_pc
> mov r1, sv_lr
> mov r2, frame
> bic r1, r1, mask @ mask PC/LR for the mode
> @@ -182,7 +181,7 @@ finished_setup:
> * pointer the comparison will fail and no registers will print. Unwinding will
> * continue as if there had been no registers stored in this frame.
> */
> -1005: ldr r1, [sv_pc, #0] @ if stmfd sp!, {..., fp, lr}
> +load_stmfd: ldr r1, [sv_pc, #0] @ if stmfd sp!, {..., fp, lr}
> ldr r3, .Lopcode @ instruction exists,
> teq r3, r1, lsr #11
> ldr r0, [frame] @ locals are stored in
> @@ -201,7 +200,7 @@ finished_setup:
> mov frame, sv_fp @ above the current frame
> bhi for_each_frame
>
> -1006: adr r0, .Lbad
> +bad_frame: adr r0, .Lbad
> mov r1, loglvl
> mov r2, frame
> bl printk
> @@ -216,11 +215,10 @@ bad_lr: mov sv_fp, #0
> ENDPROC(c_backtrace)
> .pushsection __ex_table,"a"
> .align 3
> - .long 1001b, 1006b
> - .long 1002b, 1006b
> - .long 1003b, 1006b
> - .long 1004b, 1006b
> - .long 1005b, 1006b
> + .long load_pc, bad_frame
> + .long load_fp, bad_frame
> + .long load_lr, bad_frame
> + .long load_stmfd, bad_frame
Load_stmfd should get its own fixup
handler since it should emit errors about a bad
pc, not a bad frame pointer.
> .long prev_call, bad_lr
> .popsection
>
> --
> 2.28.0.163.g6104cc2f0b6-goog
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 3/4] ARM: backtrace-clang: give labels more descriptive names
@ 2020-08-06 22:39 ` Nathan Huckleberry
0 siblings, 0 replies; 40+ messages in thread
From: Nathan Huckleberry @ 2020-08-06 22:39 UTC (permalink / raw
To: Nick Desaulniers
Cc: Chunyan Zhang, Dmitry Safonov, Russell King, linux-kernel,
clang-built-linux, Miles Chen, linux-mediatek, Matthias Brugger,
Andrew Morton, Nathan Huckleberry, Lvqiang Huang,
linux-arm-kernel
The style cleanup looks great. I just have one extra thing that
can probably be thrown into this patch.
On Thu, Jul 30, 2020 at 3:51 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> Removes the 1004 label; it was neither a control flow target, nor an
> instruction we expect to produce a fault.
>
> Gives the labels slightly more readable names. The `b` suffixes are
> handy to disambiguate between labels of the same identifier when there's
> more than one. Since these labels are unique, let's just give them
> names.
>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> arch/arm/lib/backtrace-clang.S | 22 ++++++++++------------
> 1 file changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm/lib/backtrace-clang.S b/arch/arm/lib/backtrace-clang.S
> index 40eb2215eaf4..7dad2a6843a5 100644
> --- a/arch/arm/lib/backtrace-clang.S
> +++ b/arch/arm/lib/backtrace-clang.S
> @@ -121,8 +121,8 @@ for_each_frame: tst frame, mask @ Check for address exceptions
> * start. This value gets updated to be the function start later if it is
> * possible.
> */
> -1001: ldr sv_pc, [frame, #4] @ get saved 'pc'
> -1002: ldr sv_fp, [frame, #0] @ get saved fp
> +load_pc: ldr sv_pc, [frame, #4] @ get saved 'pc'
> +load_fp: ldr sv_fp, [frame, #0] @ get saved fp
>
> teq sv_fp, mask @ make sure next frame exists
> beq no_frame
> @@ -142,7 +142,7 @@ for_each_frame: tst frame, mask @ Check for address exceptions
> * registers for the current function, but the stacktrace is still printed
> * properly.
> */
> -1003: ldr sv_lr, [sv_fp, #4] @ get saved lr from next frame
> +load_lr: ldr sv_lr, [sv_fp, #4] @ get saved lr from next frame
>
> tst sv_lr, #0 @ If there's no previous lr,
> beq finished_setup @ we're done.
> @@ -166,8 +166,7 @@ finished_setup:
> /*
> * Print the function (sv_pc) and where it was called from (sv_lr).
> */
> -1004: mov r0, sv_pc
> -
> + mov r0, sv_pc
> mov r1, sv_lr
> mov r2, frame
> bic r1, r1, mask @ mask PC/LR for the mode
> @@ -182,7 +181,7 @@ finished_setup:
> * pointer the comparison will fail and no registers will print. Unwinding will
> * continue as if there had been no registers stored in this frame.
> */
> -1005: ldr r1, [sv_pc, #0] @ if stmfd sp!, {..., fp, lr}
> +load_stmfd: ldr r1, [sv_pc, #0] @ if stmfd sp!, {..., fp, lr}
> ldr r3, .Lopcode @ instruction exists,
> teq r3, r1, lsr #11
> ldr r0, [frame] @ locals are stored in
> @@ -201,7 +200,7 @@ finished_setup:
> mov frame, sv_fp @ above the current frame
> bhi for_each_frame
>
> -1006: adr r0, .Lbad
> +bad_frame: adr r0, .Lbad
> mov r1, loglvl
> mov r2, frame
> bl printk
> @@ -216,11 +215,10 @@ bad_lr: mov sv_fp, #0
> ENDPROC(c_backtrace)
> .pushsection __ex_table,"a"
> .align 3
> - .long 1001b, 1006b
> - .long 1002b, 1006b
> - .long 1003b, 1006b
> - .long 1004b, 1006b
> - .long 1005b, 1006b
> + .long load_pc, bad_frame
> + .long load_fp, bad_frame
> + .long load_lr, bad_frame
> + .long load_stmfd, bad_frame
Load_stmfd should get its own fixup
handler since it should emit errors about a bad
pc, not a bad frame pointer.
> .long prev_call, bad_lr
> .popsection
>
> --
> 2.28.0.163.g6104cc2f0b6-goog
>
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 3/4] ARM: backtrace-clang: give labels more descriptive names
@ 2020-08-06 22:39 ` Nathan Huckleberry
0 siblings, 0 replies; 40+ messages in thread
From: Nathan Huckleberry @ 2020-08-06 22:39 UTC (permalink / raw
To: Nick Desaulniers
Cc: Chunyan Zhang, Dmitry Safonov, Russell King, linux-kernel,
clang-built-linux, Miles Chen, linux-mediatek, Matthias Brugger,
Andrew Morton, Nathan Huckleberry, Lvqiang Huang,
linux-arm-kernel
The style cleanup looks great. I just have one extra thing that
can probably be thrown into this patch.
On Thu, Jul 30, 2020 at 3:51 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> Removes the 1004 label; it was neither a control flow target, nor an
> instruction we expect to produce a fault.
>
> Gives the labels slightly more readable names. The `b` suffixes are
> handy to disambiguate between labels of the same identifier when there's
> more than one. Since these labels are unique, let's just give them
> names.
>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> arch/arm/lib/backtrace-clang.S | 22 ++++++++++------------
> 1 file changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm/lib/backtrace-clang.S b/arch/arm/lib/backtrace-clang.S
> index 40eb2215eaf4..7dad2a6843a5 100644
> --- a/arch/arm/lib/backtrace-clang.S
> +++ b/arch/arm/lib/backtrace-clang.S
> @@ -121,8 +121,8 @@ for_each_frame: tst frame, mask @ Check for address exceptions
> * start. This value gets updated to be the function start later if it is
> * possible.
> */
> -1001: ldr sv_pc, [frame, #4] @ get saved 'pc'
> -1002: ldr sv_fp, [frame, #0] @ get saved fp
> +load_pc: ldr sv_pc, [frame, #4] @ get saved 'pc'
> +load_fp: ldr sv_fp, [frame, #0] @ get saved fp
>
> teq sv_fp, mask @ make sure next frame exists
> beq no_frame
> @@ -142,7 +142,7 @@ for_each_frame: tst frame, mask @ Check for address exceptions
> * registers for the current function, but the stacktrace is still printed
> * properly.
> */
> -1003: ldr sv_lr, [sv_fp, #4] @ get saved lr from next frame
> +load_lr: ldr sv_lr, [sv_fp, #4] @ get saved lr from next frame
>
> tst sv_lr, #0 @ If there's no previous lr,
> beq finished_setup @ we're done.
> @@ -166,8 +166,7 @@ finished_setup:
> /*
> * Print the function (sv_pc) and where it was called from (sv_lr).
> */
> -1004: mov r0, sv_pc
> -
> + mov r0, sv_pc
> mov r1, sv_lr
> mov r2, frame
> bic r1, r1, mask @ mask PC/LR for the mode
> @@ -182,7 +181,7 @@ finished_setup:
> * pointer the comparison will fail and no registers will print. Unwinding will
> * continue as if there had been no registers stored in this frame.
> */
> -1005: ldr r1, [sv_pc, #0] @ if stmfd sp!, {..., fp, lr}
> +load_stmfd: ldr r1, [sv_pc, #0] @ if stmfd sp!, {..., fp, lr}
> ldr r3, .Lopcode @ instruction exists,
> teq r3, r1, lsr #11
> ldr r0, [frame] @ locals are stored in
> @@ -201,7 +200,7 @@ finished_setup:
> mov frame, sv_fp @ above the current frame
> bhi for_each_frame
>
> -1006: adr r0, .Lbad
> +bad_frame: adr r0, .Lbad
> mov r1, loglvl
> mov r2, frame
> bl printk
> @@ -216,11 +215,10 @@ bad_lr: mov sv_fp, #0
> ENDPROC(c_backtrace)
> .pushsection __ex_table,"a"
> .align 3
> - .long 1001b, 1006b
> - .long 1002b, 1006b
> - .long 1003b, 1006b
> - .long 1004b, 1006b
> - .long 1005b, 1006b
> + .long load_pc, bad_frame
> + .long load_fp, bad_frame
> + .long load_lr, bad_frame
> + .long load_stmfd, bad_frame
Load_stmfd should get its own fixup
handler since it should emit errors about a bad
pc, not a bad frame pointer.
> .long prev_call, bad_lr
> .popsection
>
> --
> 2.28.0.163.g6104cc2f0b6-goog
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 1/4] ARM: backtrace-clang: check for NULL lr
2020-07-30 20:51 ` Nick Desaulniers
(?)
@ 2020-08-07 18:07 ` Nathan Huckleberry
-1 siblings, 0 replies; 40+ messages in thread
From: Nathan Huckleberry @ 2020-08-07 18:07 UTC (permalink / raw
To: Nick Desaulniers
Cc: Russell King, Andrew Morton, Chunyan Zhang, clang-built-linux,
Dmitry Safonov, linux-arm-kernel, linux-kernel, linux-mediatek,
Lvqiang Huang, Matthias Brugger, Miles Chen, stable
On Thu, Jul 30, 2020 at 3:51 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> If the link register was zeroed out, do not attempt to use it for
> address calculations for which there are currently no fixup handlers,
> which can lead to a panic during unwind. Since panicking triggers
> another unwind, this can lead to an infinite loop. If this occurs
> during start_kernel(), this can prevent a kernel from booting.
>
> commit 59b6359dd92d ("ARM: 8702/1: head-common.S: Clear lr before jumping to start_kernel()")
> intentionally zeros out the link register in __mmap_switched which tail
> calls into start kernel. Test for this condition so that we can stop
> unwinding when initiated within start_kernel() correctly.
>
> Cc: stable@vger.kernel.org
> Fixes: commit 6dc5fd93b2f1 ("ARM: 8900/1: UNWINDER_FRAME_POINTER implementation for Clang")
> Reported-by: Miles Chen <miles.chen@mediatek.com>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> arch/arm/lib/backtrace-clang.S | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm/lib/backtrace-clang.S b/arch/arm/lib/backtrace-clang.S
> index 6174c45f53a5..5388ac664c12 100644
> --- a/arch/arm/lib/backtrace-clang.S
> +++ b/arch/arm/lib/backtrace-clang.S
> @@ -144,6 +144,8 @@ for_each_frame: tst frame, mask @ Check for address exceptions
> */
> 1003: ldr sv_lr, [sv_fp, #4] @ get saved lr from next frame
>
> + tst sv_lr, #0 @ If there's no previous lr,
> + beq finished_setup @ we're done.
> ldr r0, [sv_lr, #-4] @ get call instruction
> ldr r3, .Lopcode+4
> and r2, r3, r0 @ is this a bl call
> --
> 2.28.0.163.g6104cc2f0b6-goog
>
Reviewed-by: Nathan Huckleberry <nhuck15@gmail.com>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 1/4] ARM: backtrace-clang: check for NULL lr
@ 2020-08-07 18:07 ` Nathan Huckleberry
0 siblings, 0 replies; 40+ messages in thread
From: Nathan Huckleberry @ 2020-08-07 18:07 UTC (permalink / raw
To: Nick Desaulniers
Cc: Chunyan Zhang, Dmitry Safonov, Russell King, stable, linux-kernel,
clang-built-linux, Miles Chen, linux-mediatek, Matthias Brugger,
Andrew Morton, Lvqiang Huang, linux-arm-kernel
On Thu, Jul 30, 2020 at 3:51 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> If the link register was zeroed out, do not attempt to use it for
> address calculations for which there are currently no fixup handlers,
> which can lead to a panic during unwind. Since panicking triggers
> another unwind, this can lead to an infinite loop. If this occurs
> during start_kernel(), this can prevent a kernel from booting.
>
> commit 59b6359dd92d ("ARM: 8702/1: head-common.S: Clear lr before jumping to start_kernel()")
> intentionally zeros out the link register in __mmap_switched which tail
> calls into start kernel. Test for this condition so that we can stop
> unwinding when initiated within start_kernel() correctly.
>
> Cc: stable@vger.kernel.org
> Fixes: commit 6dc5fd93b2f1 ("ARM: 8900/1: UNWINDER_FRAME_POINTER implementation for Clang")
> Reported-by: Miles Chen <miles.chen@mediatek.com>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> arch/arm/lib/backtrace-clang.S | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm/lib/backtrace-clang.S b/arch/arm/lib/backtrace-clang.S
> index 6174c45f53a5..5388ac664c12 100644
> --- a/arch/arm/lib/backtrace-clang.S
> +++ b/arch/arm/lib/backtrace-clang.S
> @@ -144,6 +144,8 @@ for_each_frame: tst frame, mask @ Check for address exceptions
> */
> 1003: ldr sv_lr, [sv_fp, #4] @ get saved lr from next frame
>
> + tst sv_lr, #0 @ If there's no previous lr,
> + beq finished_setup @ we're done.
> ldr r0, [sv_lr, #-4] @ get call instruction
> ldr r3, .Lopcode+4
> and r2, r3, r0 @ is this a bl call
> --
> 2.28.0.163.g6104cc2f0b6-goog
>
Reviewed-by: Nathan Huckleberry <nhuck15@gmail.com>
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 1/4] ARM: backtrace-clang: check for NULL lr
@ 2020-08-07 18:07 ` Nathan Huckleberry
0 siblings, 0 replies; 40+ messages in thread
From: Nathan Huckleberry @ 2020-08-07 18:07 UTC (permalink / raw
To: Nick Desaulniers
Cc: Chunyan Zhang, Dmitry Safonov, Russell King, stable, linux-kernel,
clang-built-linux, Miles Chen, linux-mediatek, Matthias Brugger,
Andrew Morton, Lvqiang Huang, linux-arm-kernel
On Thu, Jul 30, 2020 at 3:51 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> If the link register was zeroed out, do not attempt to use it for
> address calculations for which there are currently no fixup handlers,
> which can lead to a panic during unwind. Since panicking triggers
> another unwind, this can lead to an infinite loop. If this occurs
> during start_kernel(), this can prevent a kernel from booting.
>
> commit 59b6359dd92d ("ARM: 8702/1: head-common.S: Clear lr before jumping to start_kernel()")
> intentionally zeros out the link register in __mmap_switched which tail
> calls into start kernel. Test for this condition so that we can stop
> unwinding when initiated within start_kernel() correctly.
>
> Cc: stable@vger.kernel.org
> Fixes: commit 6dc5fd93b2f1 ("ARM: 8900/1: UNWINDER_FRAME_POINTER implementation for Clang")
> Reported-by: Miles Chen <miles.chen@mediatek.com>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> arch/arm/lib/backtrace-clang.S | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm/lib/backtrace-clang.S b/arch/arm/lib/backtrace-clang.S
> index 6174c45f53a5..5388ac664c12 100644
> --- a/arch/arm/lib/backtrace-clang.S
> +++ b/arch/arm/lib/backtrace-clang.S
> @@ -144,6 +144,8 @@ for_each_frame: tst frame, mask @ Check for address exceptions
> */
> 1003: ldr sv_lr, [sv_fp, #4] @ get saved lr from next frame
>
> + tst sv_lr, #0 @ If there's no previous lr,
> + beq finished_setup @ we're done.
> ldr r0, [sv_lr, #-4] @ get call instruction
> ldr r3, .Lopcode+4
> and r2, r3, r0 @ is this a bl call
> --
> 2.28.0.163.g6104cc2f0b6-goog
>
Reviewed-by: Nathan Huckleberry <nhuck15@gmail.com>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 3/4] ARM: backtrace-clang: give labels more descriptive names
2020-08-06 22:39 ` Nathan Huckleberry
(?)
@ 2020-08-10 22:32 ` Nick Desaulniers
-1 siblings, 0 replies; 40+ messages in thread
From: Nick Desaulniers @ 2020-08-10 22:32 UTC (permalink / raw
To: Nathan Huckleberry
Cc: Russell King, Andrew Morton, Chunyan Zhang, clang-built-linux,
Dmitry Safonov, Linux ARM, LKML, linux-mediatek, Lvqiang Huang,
Matthias Brugger, Miles Chen, Nathan Huckleberry
On Thu, Aug 6, 2020 at 3:39 PM Nathan Huckleberry <nhuck15@gmail.com> wrote:
>
> The style cleanup looks great. I just have one extra thing that
> can probably be thrown into this patch.
>
> On Thu, Jul 30, 2020 at 3:51 PM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > Removes the 1004 label; it was neither a control flow target, nor an
> > instruction we expect to produce a fault.
> >
> > Gives the labels slightly more readable names. The `b` suffixes are
> > handy to disambiguate between labels of the same identifier when there's
> > more than one. Since these labels are unique, let's just give them
> > names.
> >
> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > ---
> > arch/arm/lib/backtrace-clang.S | 22 ++++++++++------------
> > 1 file changed, 10 insertions(+), 12 deletions(-)
> >
> > diff --git a/arch/arm/lib/backtrace-clang.S b/arch/arm/lib/backtrace-clang.S
> > index 40eb2215eaf4..7dad2a6843a5 100644
> > --- a/arch/arm/lib/backtrace-clang.S
> > +++ b/arch/arm/lib/backtrace-clang.S
> > @@ -121,8 +121,8 @@ for_each_frame: tst frame, mask @ Check for address exceptions
> > * start. This value gets updated to be the function start later if it is
> > * possible.
> > */
> > -1001: ldr sv_pc, [frame, #4] @ get saved 'pc'
> > -1002: ldr sv_fp, [frame, #0] @ get saved fp
> > +load_pc: ldr sv_pc, [frame, #4] @ get saved 'pc'
> > +load_fp: ldr sv_fp, [frame, #0] @ get saved fp
> >
> > teq sv_fp, mask @ make sure next frame exists
> > beq no_frame
> > @@ -142,7 +142,7 @@ for_each_frame: tst frame, mask @ Check for address exceptions
> > * registers for the current function, but the stacktrace is still printed
> > * properly.
> > */
> > -1003: ldr sv_lr, [sv_fp, #4] @ get saved lr from next frame
> > +load_lr: ldr sv_lr, [sv_fp, #4] @ get saved lr from next frame
> >
> > tst sv_lr, #0 @ If there's no previous lr,
> > beq finished_setup @ we're done.
> > @@ -166,8 +166,7 @@ finished_setup:
> > /*
> > * Print the function (sv_pc) and where it was called from (sv_lr).
> > */
> > -1004: mov r0, sv_pc
> > -
> > + mov r0, sv_pc
> > mov r1, sv_lr
> > mov r2, frame
> > bic r1, r1, mask @ mask PC/LR for the mode
> > @@ -182,7 +181,7 @@ finished_setup:
> > * pointer the comparison will fail and no registers will print. Unwinding will
> > * continue as if there had been no registers stored in this frame.
> > */
> > -1005: ldr r1, [sv_pc, #0] @ if stmfd sp!, {..., fp, lr}
> > +load_stmfd: ldr r1, [sv_pc, #0] @ if stmfd sp!, {..., fp, lr}
> > ldr r3, .Lopcode @ instruction exists,
> > teq r3, r1, lsr #11
> > ldr r0, [frame] @ locals are stored in
> > @@ -201,7 +200,7 @@ finished_setup:
> > mov frame, sv_fp @ above the current frame
> > bhi for_each_frame
> >
> > -1006: adr r0, .Lbad
> > +bad_frame: adr r0, .Lbad
> > mov r1, loglvl
> > mov r2, frame
> > bl printk
> > @@ -216,11 +215,10 @@ bad_lr: mov sv_fp, #0
> > ENDPROC(c_backtrace)
> > .pushsection __ex_table,"a"
> > .align 3
> > - .long 1001b, 1006b
> > - .long 1002b, 1006b
> > - .long 1003b, 1006b
> > - .long 1004b, 1006b
> > - .long 1005b, 1006b
> > + .long load_pc, bad_frame
> > + .long load_fp, bad_frame
> > + .long load_lr, bad_frame
> > + .long load_stmfd, bad_frame
>
> Load_stmfd should get its own fixup
> handler since it should emit errors about a bad
> pc, not a bad frame pointer.
Yeah, I can add that. It's a little orthogonal though to this patch
that renames labels. I'd consider more so a pre-existing bug. Let me
add a patch to the series that gives it a new fixup handler, separate
from the label renaming, in a v2 of the series.
>
> > .long prev_call, bad_lr
> > .popsection
> >
> > --
> > 2.28.0.163.g6104cc2f0b6-goog
> >
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 3/4] ARM: backtrace-clang: give labels more descriptive names
@ 2020-08-10 22:32 ` Nick Desaulniers
0 siblings, 0 replies; 40+ messages in thread
From: Nick Desaulniers @ 2020-08-10 22:32 UTC (permalink / raw
To: Nathan Huckleberry
Cc: Chunyan Zhang, Dmitry Safonov, Russell King, LKML,
clang-built-linux, Miles Chen, linux-mediatek, Matthias Brugger,
Andrew Morton, Nathan Huckleberry, Lvqiang Huang, Linux ARM
On Thu, Aug 6, 2020 at 3:39 PM Nathan Huckleberry <nhuck15@gmail.com> wrote:
>
> The style cleanup looks great. I just have one extra thing that
> can probably be thrown into this patch.
>
> On Thu, Jul 30, 2020 at 3:51 PM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > Removes the 1004 label; it was neither a control flow target, nor an
> > instruction we expect to produce a fault.
> >
> > Gives the labels slightly more readable names. The `b` suffixes are
> > handy to disambiguate between labels of the same identifier when there's
> > more than one. Since these labels are unique, let's just give them
> > names.
> >
> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > ---
> > arch/arm/lib/backtrace-clang.S | 22 ++++++++++------------
> > 1 file changed, 10 insertions(+), 12 deletions(-)
> >
> > diff --git a/arch/arm/lib/backtrace-clang.S b/arch/arm/lib/backtrace-clang.S
> > index 40eb2215eaf4..7dad2a6843a5 100644
> > --- a/arch/arm/lib/backtrace-clang.S
> > +++ b/arch/arm/lib/backtrace-clang.S
> > @@ -121,8 +121,8 @@ for_each_frame: tst frame, mask @ Check for address exceptions
> > * start. This value gets updated to be the function start later if it is
> > * possible.
> > */
> > -1001: ldr sv_pc, [frame, #4] @ get saved 'pc'
> > -1002: ldr sv_fp, [frame, #0] @ get saved fp
> > +load_pc: ldr sv_pc, [frame, #4] @ get saved 'pc'
> > +load_fp: ldr sv_fp, [frame, #0] @ get saved fp
> >
> > teq sv_fp, mask @ make sure next frame exists
> > beq no_frame
> > @@ -142,7 +142,7 @@ for_each_frame: tst frame, mask @ Check for address exceptions
> > * registers for the current function, but the stacktrace is still printed
> > * properly.
> > */
> > -1003: ldr sv_lr, [sv_fp, #4] @ get saved lr from next frame
> > +load_lr: ldr sv_lr, [sv_fp, #4] @ get saved lr from next frame
> >
> > tst sv_lr, #0 @ If there's no previous lr,
> > beq finished_setup @ we're done.
> > @@ -166,8 +166,7 @@ finished_setup:
> > /*
> > * Print the function (sv_pc) and where it was called from (sv_lr).
> > */
> > -1004: mov r0, sv_pc
> > -
> > + mov r0, sv_pc
> > mov r1, sv_lr
> > mov r2, frame
> > bic r1, r1, mask @ mask PC/LR for the mode
> > @@ -182,7 +181,7 @@ finished_setup:
> > * pointer the comparison will fail and no registers will print. Unwinding will
> > * continue as if there had been no registers stored in this frame.
> > */
> > -1005: ldr r1, [sv_pc, #0] @ if stmfd sp!, {..., fp, lr}
> > +load_stmfd: ldr r1, [sv_pc, #0] @ if stmfd sp!, {..., fp, lr}
> > ldr r3, .Lopcode @ instruction exists,
> > teq r3, r1, lsr #11
> > ldr r0, [frame] @ locals are stored in
> > @@ -201,7 +200,7 @@ finished_setup:
> > mov frame, sv_fp @ above the current frame
> > bhi for_each_frame
> >
> > -1006: adr r0, .Lbad
> > +bad_frame: adr r0, .Lbad
> > mov r1, loglvl
> > mov r2, frame
> > bl printk
> > @@ -216,11 +215,10 @@ bad_lr: mov sv_fp, #0
> > ENDPROC(c_backtrace)
> > .pushsection __ex_table,"a"
> > .align 3
> > - .long 1001b, 1006b
> > - .long 1002b, 1006b
> > - .long 1003b, 1006b
> > - .long 1004b, 1006b
> > - .long 1005b, 1006b
> > + .long load_pc, bad_frame
> > + .long load_fp, bad_frame
> > + .long load_lr, bad_frame
> > + .long load_stmfd, bad_frame
>
> Load_stmfd should get its own fixup
> handler since it should emit errors about a bad
> pc, not a bad frame pointer.
Yeah, I can add that. It's a little orthogonal though to this patch
that renames labels. I'd consider more so a pre-existing bug. Let me
add a patch to the series that gives it a new fixup handler, separate
from the label renaming, in a v2 of the series.
>
> > .long prev_call, bad_lr
> > .popsection
> >
> > --
> > 2.28.0.163.g6104cc2f0b6-goog
> >
--
Thanks,
~Nick Desaulniers
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 3/4] ARM: backtrace-clang: give labels more descriptive names
@ 2020-08-10 22:32 ` Nick Desaulniers
0 siblings, 0 replies; 40+ messages in thread
From: Nick Desaulniers @ 2020-08-10 22:32 UTC (permalink / raw
To: Nathan Huckleberry
Cc: Chunyan Zhang, Dmitry Safonov, Russell King, LKML,
clang-built-linux, Miles Chen, linux-mediatek, Matthias Brugger,
Andrew Morton, Nathan Huckleberry, Lvqiang Huang, Linux ARM
On Thu, Aug 6, 2020 at 3:39 PM Nathan Huckleberry <nhuck15@gmail.com> wrote:
>
> The style cleanup looks great. I just have one extra thing that
> can probably be thrown into this patch.
>
> On Thu, Jul 30, 2020 at 3:51 PM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > Removes the 1004 label; it was neither a control flow target, nor an
> > instruction we expect to produce a fault.
> >
> > Gives the labels slightly more readable names. The `b` suffixes are
> > handy to disambiguate between labels of the same identifier when there's
> > more than one. Since these labels are unique, let's just give them
> > names.
> >
> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > ---
> > arch/arm/lib/backtrace-clang.S | 22 ++++++++++------------
> > 1 file changed, 10 insertions(+), 12 deletions(-)
> >
> > diff --git a/arch/arm/lib/backtrace-clang.S b/arch/arm/lib/backtrace-clang.S
> > index 40eb2215eaf4..7dad2a6843a5 100644
> > --- a/arch/arm/lib/backtrace-clang.S
> > +++ b/arch/arm/lib/backtrace-clang.S
> > @@ -121,8 +121,8 @@ for_each_frame: tst frame, mask @ Check for address exceptions
> > * start. This value gets updated to be the function start later if it is
> > * possible.
> > */
> > -1001: ldr sv_pc, [frame, #4] @ get saved 'pc'
> > -1002: ldr sv_fp, [frame, #0] @ get saved fp
> > +load_pc: ldr sv_pc, [frame, #4] @ get saved 'pc'
> > +load_fp: ldr sv_fp, [frame, #0] @ get saved fp
> >
> > teq sv_fp, mask @ make sure next frame exists
> > beq no_frame
> > @@ -142,7 +142,7 @@ for_each_frame: tst frame, mask @ Check for address exceptions
> > * registers for the current function, but the stacktrace is still printed
> > * properly.
> > */
> > -1003: ldr sv_lr, [sv_fp, #4] @ get saved lr from next frame
> > +load_lr: ldr sv_lr, [sv_fp, #4] @ get saved lr from next frame
> >
> > tst sv_lr, #0 @ If there's no previous lr,
> > beq finished_setup @ we're done.
> > @@ -166,8 +166,7 @@ finished_setup:
> > /*
> > * Print the function (sv_pc) and where it was called from (sv_lr).
> > */
> > -1004: mov r0, sv_pc
> > -
> > + mov r0, sv_pc
> > mov r1, sv_lr
> > mov r2, frame
> > bic r1, r1, mask @ mask PC/LR for the mode
> > @@ -182,7 +181,7 @@ finished_setup:
> > * pointer the comparison will fail and no registers will print. Unwinding will
> > * continue as if there had been no registers stored in this frame.
> > */
> > -1005: ldr r1, [sv_pc, #0] @ if stmfd sp!, {..., fp, lr}
> > +load_stmfd: ldr r1, [sv_pc, #0] @ if stmfd sp!, {..., fp, lr}
> > ldr r3, .Lopcode @ instruction exists,
> > teq r3, r1, lsr #11
> > ldr r0, [frame] @ locals are stored in
> > @@ -201,7 +200,7 @@ finished_setup:
> > mov frame, sv_fp @ above the current frame
> > bhi for_each_frame
> >
> > -1006: adr r0, .Lbad
> > +bad_frame: adr r0, .Lbad
> > mov r1, loglvl
> > mov r2, frame
> > bl printk
> > @@ -216,11 +215,10 @@ bad_lr: mov sv_fp, #0
> > ENDPROC(c_backtrace)
> > .pushsection __ex_table,"a"
> > .align 3
> > - .long 1001b, 1006b
> > - .long 1002b, 1006b
> > - .long 1003b, 1006b
> > - .long 1004b, 1006b
> > - .long 1005b, 1006b
> > + .long load_pc, bad_frame
> > + .long load_fp, bad_frame
> > + .long load_lr, bad_frame
> > + .long load_stmfd, bad_frame
>
> Load_stmfd should get its own fixup
> handler since it should emit errors about a bad
> pc, not a bad frame pointer.
Yeah, I can add that. It's a little orthogonal though to this patch
that renames labels. I'd consider more so a pre-existing bug. Let me
add a patch to the series that gives it a new fixup handler, separate
from the label renaming, in a v2 of the series.
>
> > .long prev_call, bad_lr
> > .popsection
> >
> > --
> > 2.28.0.163.g6104cc2f0b6-goog
> >
--
Thanks,
~Nick Desaulniers
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/4] ARM: backtrace-clang: add fixup for lr dereference
2020-08-06 22:38 ` Nathan Huckleberry
(?)
@ 2020-08-10 22:33 ` Nick Desaulniers
-1 siblings, 0 replies; 40+ messages in thread
From: Nick Desaulniers @ 2020-08-10 22:33 UTC (permalink / raw
To: Nathan Huckleberry
Cc: Russell King, Andrew Morton, Chunyan Zhang, clang-built-linux,
Dmitry Safonov, Linux ARM, LKML, linux-mediatek, Lvqiang Huang,
Matthias Brugger, Miles Chen, # 3.4.x, Nathan Huckleberry
On Thu, Aug 6, 2020 at 3:39 PM Nathan Huckleberry <nhuck15@gmail.com> wrote:
>
> Mostly looks good to me. Just a minor nit.
>
> On Thu, Jul 30, 2020 at 3:51 PM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > If the value of the link register is not correct (tail call from asm
> > that didn't set it, stack corruption, memory no longer mapped), then
> > using it for an address calculation may trigger an exception. Without a
> > fixup handler, this will lead to a panic, which will unwind, which will
> > trigger the fault repeatedly in an infinite loop.
> >
> > We don't observe such failures currently, but we have. Just to be safe,
> > add a fixup handler here so that at least we don't have an infinite
> > loop.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: commit 6dc5fd93b2f1 ("ARM: 8900/1: UNWINDER_FRAME_POINTER implementation for Clang")
> > Reported-by: Miles Chen <miles.chen@mediatek.com>
> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > ---
> > arch/arm/lib/backtrace-clang.S | 10 +++++++++-
> > 1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/lib/backtrace-clang.S b/arch/arm/lib/backtrace-clang.S
> > index 5388ac664c12..40eb2215eaf4 100644
> > --- a/arch/arm/lib/backtrace-clang.S
> > +++ b/arch/arm/lib/backtrace-clang.S
> > @@ -146,7 +146,7 @@ for_each_frame: tst frame, mask @ Check for address exceptions
> >
> > tst sv_lr, #0 @ If there's no previous lr,
> > beq finished_setup @ we're done.
> > - ldr r0, [sv_lr, #-4] @ get call instruction
> > +prev_call: ldr r0, [sv_lr, #-4] @ get call instruction
> > ldr r3, .Lopcode+4
> > and r2, r3, r0 @ is this a bl call
> > teq r2, r3
> > @@ -206,6 +206,13 @@ finished_setup:
> > mov r2, frame
> > bl printk
> > no_frame: ldmfd sp!, {r4 - r9, fp, pc}
> > +/*
> > + * Accessing the address pointed to by the link register triggered an
> > + * exception, don't try to unwind through it.
> > + */
> > +bad_lr: mov sv_fp, #0
>
> It might be nice to emit a warning here since we'll
> only hit this case if something fishy is going on
> with the saved lr.
Yeah, something fishy is going on if that ever happens. Let me create
a V2 with an additional print.
>
> > + mov sv_lr, #0
> > + b finished_setup
> > ENDPROC(c_backtrace)
> > .pushsection __ex_table,"a"
> > .align 3
> > @@ -214,6 +221,7 @@ ENDPROC(c_backtrace)
> > .long 1003b, 1006b
> > .long 1004b, 1006b
> > .long 1005b, 1006b
> > + .long prev_call, bad_lr
> > .popsection
> >
> > .Lbad: .asciz "%sBacktrace aborted due to bad frame pointer <%p>\n"
> > --
> > 2.28.0.163.g6104cc2f0b6-goog
> >
>
> Thanks,
> Huck
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/4] ARM: backtrace-clang: add fixup for lr dereference
@ 2020-08-10 22:33 ` Nick Desaulniers
0 siblings, 0 replies; 40+ messages in thread
From: Nick Desaulniers @ 2020-08-10 22:33 UTC (permalink / raw
To: Nathan Huckleberry
Cc: Chunyan Zhang, Dmitry Safonov, Russell King, # 3.4.x, LKML,
clang-built-linux, Miles Chen, linux-mediatek, Matthias Brugger,
Andrew Morton, Nathan Huckleberry, Lvqiang Huang, Linux ARM
On Thu, Aug 6, 2020 at 3:39 PM Nathan Huckleberry <nhuck15@gmail.com> wrote:
>
> Mostly looks good to me. Just a minor nit.
>
> On Thu, Jul 30, 2020 at 3:51 PM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > If the value of the link register is not correct (tail call from asm
> > that didn't set it, stack corruption, memory no longer mapped), then
> > using it for an address calculation may trigger an exception. Without a
> > fixup handler, this will lead to a panic, which will unwind, which will
> > trigger the fault repeatedly in an infinite loop.
> >
> > We don't observe such failures currently, but we have. Just to be safe,
> > add a fixup handler here so that at least we don't have an infinite
> > loop.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: commit 6dc5fd93b2f1 ("ARM: 8900/1: UNWINDER_FRAME_POINTER implementation for Clang")
> > Reported-by: Miles Chen <miles.chen@mediatek.com>
> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > ---
> > arch/arm/lib/backtrace-clang.S | 10 +++++++++-
> > 1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/lib/backtrace-clang.S b/arch/arm/lib/backtrace-clang.S
> > index 5388ac664c12..40eb2215eaf4 100644
> > --- a/arch/arm/lib/backtrace-clang.S
> > +++ b/arch/arm/lib/backtrace-clang.S
> > @@ -146,7 +146,7 @@ for_each_frame: tst frame, mask @ Check for address exceptions
> >
> > tst sv_lr, #0 @ If there's no previous lr,
> > beq finished_setup @ we're done.
> > - ldr r0, [sv_lr, #-4] @ get call instruction
> > +prev_call: ldr r0, [sv_lr, #-4] @ get call instruction
> > ldr r3, .Lopcode+4
> > and r2, r3, r0 @ is this a bl call
> > teq r2, r3
> > @@ -206,6 +206,13 @@ finished_setup:
> > mov r2, frame
> > bl printk
> > no_frame: ldmfd sp!, {r4 - r9, fp, pc}
> > +/*
> > + * Accessing the address pointed to by the link register triggered an
> > + * exception, don't try to unwind through it.
> > + */
> > +bad_lr: mov sv_fp, #0
>
> It might be nice to emit a warning here since we'll
> only hit this case if something fishy is going on
> with the saved lr.
Yeah, something fishy is going on if that ever happens. Let me create
a V2 with an additional print.
>
> > + mov sv_lr, #0
> > + b finished_setup
> > ENDPROC(c_backtrace)
> > .pushsection __ex_table,"a"
> > .align 3
> > @@ -214,6 +221,7 @@ ENDPROC(c_backtrace)
> > .long 1003b, 1006b
> > .long 1004b, 1006b
> > .long 1005b, 1006b
> > + .long prev_call, bad_lr
> > .popsection
> >
> > .Lbad: .asciz "%sBacktrace aborted due to bad frame pointer <%p>\n"
> > --
> > 2.28.0.163.g6104cc2f0b6-goog
> >
>
> Thanks,
> Huck
--
Thanks,
~Nick Desaulniers
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/4] ARM: backtrace-clang: add fixup for lr dereference
@ 2020-08-10 22:33 ` Nick Desaulniers
0 siblings, 0 replies; 40+ messages in thread
From: Nick Desaulniers @ 2020-08-10 22:33 UTC (permalink / raw
To: Nathan Huckleberry
Cc: Chunyan Zhang, Dmitry Safonov, Russell King, # 3.4.x, LKML,
clang-built-linux, Miles Chen, linux-mediatek, Matthias Brugger,
Andrew Morton, Nathan Huckleberry, Lvqiang Huang, Linux ARM
On Thu, Aug 6, 2020 at 3:39 PM Nathan Huckleberry <nhuck15@gmail.com> wrote:
>
> Mostly looks good to me. Just a minor nit.
>
> On Thu, Jul 30, 2020 at 3:51 PM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > If the value of the link register is not correct (tail call from asm
> > that didn't set it, stack corruption, memory no longer mapped), then
> > using it for an address calculation may trigger an exception. Without a
> > fixup handler, this will lead to a panic, which will unwind, which will
> > trigger the fault repeatedly in an infinite loop.
> >
> > We don't observe such failures currently, but we have. Just to be safe,
> > add a fixup handler here so that at least we don't have an infinite
> > loop.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: commit 6dc5fd93b2f1 ("ARM: 8900/1: UNWINDER_FRAME_POINTER implementation for Clang")
> > Reported-by: Miles Chen <miles.chen@mediatek.com>
> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > ---
> > arch/arm/lib/backtrace-clang.S | 10 +++++++++-
> > 1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/lib/backtrace-clang.S b/arch/arm/lib/backtrace-clang.S
> > index 5388ac664c12..40eb2215eaf4 100644
> > --- a/arch/arm/lib/backtrace-clang.S
> > +++ b/arch/arm/lib/backtrace-clang.S
> > @@ -146,7 +146,7 @@ for_each_frame: tst frame, mask @ Check for address exceptions
> >
> > tst sv_lr, #0 @ If there's no previous lr,
> > beq finished_setup @ we're done.
> > - ldr r0, [sv_lr, #-4] @ get call instruction
> > +prev_call: ldr r0, [sv_lr, #-4] @ get call instruction
> > ldr r3, .Lopcode+4
> > and r2, r3, r0 @ is this a bl call
> > teq r2, r3
> > @@ -206,6 +206,13 @@ finished_setup:
> > mov r2, frame
> > bl printk
> > no_frame: ldmfd sp!, {r4 - r9, fp, pc}
> > +/*
> > + * Accessing the address pointed to by the link register triggered an
> > + * exception, don't try to unwind through it.
> > + */
> > +bad_lr: mov sv_fp, #0
>
> It might be nice to emit a warning here since we'll
> only hit this case if something fishy is going on
> with the saved lr.
Yeah, something fishy is going on if that ever happens. Let me create
a V2 with an additional print.
>
> > + mov sv_lr, #0
> > + b finished_setup
> > ENDPROC(c_backtrace)
> > .pushsection __ex_table,"a"
> > .align 3
> > @@ -214,6 +221,7 @@ ENDPROC(c_backtrace)
> > .long 1003b, 1006b
> > .long 1004b, 1006b
> > .long 1005b, 1006b
> > + .long prev_call, bad_lr
> > .popsection
> >
> > .Lbad: .asciz "%sBacktrace aborted due to bad frame pointer <%p>\n"
> > --
> > 2.28.0.163.g6104cc2f0b6-goog
> >
>
> Thanks,
> Huck
--
Thanks,
~Nick Desaulniers
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/4] ARM: backtrace-clang: add fixup for lr dereference
2020-07-30 20:51 ` Nick Desaulniers
` (4 preceding siblings ...)
(?)
@ 2020-08-13 16:25 ` Sasha Levin
-1 siblings, 0 replies; 40+ messages in thread
From: Sasha Levin @ 2020-08-13 16:25 UTC (permalink / raw
To: Sasha Levin, Nick Desaulniers, Nathan Huckleberry
Cc: Andrew Morton, stable, stable
Hi
[This is an automated email]
This commit has been processed because it contains a "Fixes:" tag
fixing commit: 6dc5fd93b2f1 ("ARM: 8900/1: UNWINDER_FRAME_POINTER implementation for Clang").
The bot has tested the following trees: v5.8, v5.7.14, v5.4.57.
v5.8: Failed to apply! Possible dependencies:
90c11fed93ca ("ARM: backtrace-clang: check for NULL lr")
v5.7.14: Failed to apply! Possible dependencies:
5489ab50c227 ("arm/asm: add loglvl to c_backtrace()")
90c11fed93ca ("ARM: backtrace-clang: check for NULL lr")
v5.4.57: Failed to apply! Possible dependencies:
40ff1ddb5570 ("ARM: 8948/1: Prevent OOB access in stacktrace")
5489ab50c227 ("arm/asm: add loglvl to c_backtrace()")
90c11fed93ca ("ARM: backtrace-clang: check for NULL lr")
NOTE: The patch will not be queued to stable trees until it is upstream.
How should we proceed with this patch?
--
Thanks
Sasha
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/4] ARM: backtrace-clang: add fixup for lr dereference
2020-07-30 20:51 ` Nick Desaulniers
` (5 preceding siblings ...)
(?)
@ 2020-08-19 23:56 ` Sasha Levin
-1 siblings, 0 replies; 40+ messages in thread
From: Sasha Levin @ 2020-08-19 23:56 UTC (permalink / raw
To: Sasha Levin, Nick Desaulniers, Nathan Huckleberry
Cc: Andrew Morton, stable, stable
Hi
[This is an automated email]
This commit has been processed because it contains a "Fixes:" tag
fixing commit: 6dc5fd93b2f1 ("ARM: 8900/1: UNWINDER_FRAME_POINTER implementation for Clang").
The bot has tested the following trees: v5.8.1, v5.7.15, v5.4.58.
v5.8.1: Failed to apply! Possible dependencies:
8e8b31494db7 ("ARM: backtrace-clang: check for NULL lr")
v5.7.15: Failed to apply! Possible dependencies:
5489ab50c227 ("arm/asm: add loglvl to c_backtrace()")
8e8b31494db7 ("ARM: backtrace-clang: check for NULL lr")
v5.4.58: Failed to apply! Possible dependencies:
40ff1ddb5570 ("ARM: 8948/1: Prevent OOB access in stacktrace")
5489ab50c227 ("arm/asm: add loglvl to c_backtrace()")
8e8b31494db7 ("ARM: backtrace-clang: check for NULL lr")
NOTE: The patch will not be queued to stable trees until it is upstream.
How should we proceed with this patch?
--
Thanks
Sasha
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/4] ARM: backtrace-clang: add fixup for lr dereference
2020-08-10 22:33 ` Nick Desaulniers
(?)
@ 2020-08-20 0:13 ` Nick Desaulniers
-1 siblings, 0 replies; 40+ messages in thread
From: Nick Desaulniers @ 2020-08-20 0:13 UTC (permalink / raw
To: Nathan Huckleberry
Cc: Russell King, Andrew Morton, Chunyan Zhang, clang-built-linux,
Dmitry Safonov, Linux ARM, LKML, linux-mediatek, Lvqiang Huang,
Matthias Brugger, Miles Chen, # 3.4.x, Nathan Huckleberry
On Mon, Aug 10, 2020 at 3:33 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Thu, Aug 6, 2020 at 3:39 PM Nathan Huckleberry <nhuck15@gmail.com> wrote:
> >
> > Mostly looks good to me. Just a minor nit.
> >
> > On Thu, Jul 30, 2020 at 3:51 PM Nick Desaulniers
> > <ndesaulniers@google.com> wrote:
> > > +/*
> > > + * Accessing the address pointed to by the link register triggered an
> > > + * exception, don't try to unwind through it.
> > > + */
> > > +bad_lr: mov sv_fp, #0
> >
> > It might be nice to emit a warning here since we'll
> > only hit this case if something fishy is going on
> > with the saved lr.
>
> Yeah, something fishy is going on if that ever happens. Let me create
> a V2 with an additional print.
FWIW, I ran into another bug on -next when trying to update this.
Report:
https://lore.kernel.org/lkml/20200811204729.1116341-1-ndesaulniers@google.com/
Fix:
https://lore.kernel.org/lkml/20200814212525.6118-1-john.ogness@linutronix.de/T/#t
Then I got bogged down in planning for plumbers and other fires. I
hope to revisit the series after plumbers.
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/4] ARM: backtrace-clang: add fixup for lr dereference
@ 2020-08-20 0:13 ` Nick Desaulniers
0 siblings, 0 replies; 40+ messages in thread
From: Nick Desaulniers @ 2020-08-20 0:13 UTC (permalink / raw
To: Nathan Huckleberry
Cc: Chunyan Zhang, Dmitry Safonov, Russell King, # 3.4.x, LKML,
clang-built-linux, Miles Chen, linux-mediatek, Matthias Brugger,
Andrew Morton, Nathan Huckleberry, Lvqiang Huang, Linux ARM
On Mon, Aug 10, 2020 at 3:33 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Thu, Aug 6, 2020 at 3:39 PM Nathan Huckleberry <nhuck15@gmail.com> wrote:
> >
> > Mostly looks good to me. Just a minor nit.
> >
> > On Thu, Jul 30, 2020 at 3:51 PM Nick Desaulniers
> > <ndesaulniers@google.com> wrote:
> > > +/*
> > > + * Accessing the address pointed to by the link register triggered an
> > > + * exception, don't try to unwind through it.
> > > + */
> > > +bad_lr: mov sv_fp, #0
> >
> > It might be nice to emit a warning here since we'll
> > only hit this case if something fishy is going on
> > with the saved lr.
>
> Yeah, something fishy is going on if that ever happens. Let me create
> a V2 with an additional print.
FWIW, I ran into another bug on -next when trying to update this.
Report:
https://lore.kernel.org/lkml/20200811204729.1116341-1-ndesaulniers@google.com/
Fix:
https://lore.kernel.org/lkml/20200814212525.6118-1-john.ogness@linutronix.de/T/#t
Then I got bogged down in planning for plumbers and other fires. I
hope to revisit the series after plumbers.
--
Thanks,
~Nick Desaulniers
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/4] ARM: backtrace-clang: add fixup for lr dereference
@ 2020-08-20 0:13 ` Nick Desaulniers
0 siblings, 0 replies; 40+ messages in thread
From: Nick Desaulniers @ 2020-08-20 0:13 UTC (permalink / raw
To: Nathan Huckleberry
Cc: Chunyan Zhang, Dmitry Safonov, Russell King, # 3.4.x, LKML,
clang-built-linux, Miles Chen, linux-mediatek, Matthias Brugger,
Andrew Morton, Nathan Huckleberry, Lvqiang Huang, Linux ARM
On Mon, Aug 10, 2020 at 3:33 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Thu, Aug 6, 2020 at 3:39 PM Nathan Huckleberry <nhuck15@gmail.com> wrote:
> >
> > Mostly looks good to me. Just a minor nit.
> >
> > On Thu, Jul 30, 2020 at 3:51 PM Nick Desaulniers
> > <ndesaulniers@google.com> wrote:
> > > +/*
> > > + * Accessing the address pointed to by the link register triggered an
> > > + * exception, don't try to unwind through it.
> > > + */
> > > +bad_lr: mov sv_fp, #0
> >
> > It might be nice to emit a warning here since we'll
> > only hit this case if something fishy is going on
> > with the saved lr.
>
> Yeah, something fishy is going on if that ever happens. Let me create
> a V2 with an additional print.
FWIW, I ran into another bug on -next when trying to update this.
Report:
https://lore.kernel.org/lkml/20200811204729.1116341-1-ndesaulniers@google.com/
Fix:
https://lore.kernel.org/lkml/20200814212525.6118-1-john.ogness@linutronix.de/T/#t
Then I got bogged down in planning for plumbers and other fires. I
hope to revisit the series after plumbers.
--
Thanks,
~Nick Desaulniers
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
end of thread, other threads:[~2020-08-20 0:15 UTC | newest]
Thread overview: 40+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-07-30 20:51 [PATCH 0/4] CONFIG_UNWINDER_FRAME_POINTER fixes+cleanups Nick Desaulniers
2020-07-30 20:51 ` Nick Desaulniers
2020-07-30 20:51 ` Nick Desaulniers
2020-07-30 20:51 ` [PATCH 1/4] ARM: backtrace-clang: check for NULL lr Nick Desaulniers
2020-07-30 20:51 ` Nick Desaulniers
2020-07-30 20:51 ` Nick Desaulniers
2020-08-07 18:07 ` Nathan Huckleberry
2020-08-07 18:07 ` Nathan Huckleberry
2020-08-07 18:07 ` Nathan Huckleberry
2020-07-30 20:51 ` [PATCH 2/4] ARM: backtrace-clang: add fixup for lr dereference Nick Desaulniers
2020-07-30 20:51 ` Nick Desaulniers
2020-07-30 20:51 ` Nick Desaulniers
2020-08-01 23:18 ` Sasha Levin
2020-08-03 18:13 ` Nick Desaulniers
2020-08-04 6:27 ` Greg KH
2020-08-06 1:24 ` Sasha Levin
2020-08-06 22:38 ` Nathan Huckleberry
2020-08-06 22:38 ` Nathan Huckleberry
2020-08-06 22:38 ` Nathan Huckleberry
2020-08-10 22:33 ` Nick Desaulniers
2020-08-10 22:33 ` Nick Desaulniers
2020-08-10 22:33 ` Nick Desaulniers
2020-08-20 0:13 ` Nick Desaulniers
2020-08-20 0:13 ` Nick Desaulniers
2020-08-20 0:13 ` Nick Desaulniers
2020-08-13 16:25 ` Sasha Levin
2020-08-19 23:56 ` Sasha Levin
2020-07-30 20:51 ` [PATCH 3/4] ARM: backtrace-clang: give labels more descriptive names Nick Desaulniers
2020-07-30 20:51 ` Nick Desaulniers
2020-07-30 20:51 ` Nick Desaulniers
2020-08-06 22:39 ` Nathan Huckleberry
2020-08-06 22:39 ` Nathan Huckleberry
2020-08-06 22:39 ` Nathan Huckleberry
2020-08-10 22:32 ` Nick Desaulniers
2020-08-10 22:32 ` Nick Desaulniers
2020-08-10 22:32 ` Nick Desaulniers
2020-07-30 20:51 ` [PATCH 4/4] ARM: backtrace: use more descriptive labels Nick Desaulniers
2020-07-30 20:51 ` Nick Desaulniers
2020-07-30 20:51 ` Nick Desaulniers
2020-08-06 1:24 ` [PATCH 0/4] CONFIG_UNWINDER_FRAME_POINTER fixes+cleanups Sasha Levin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.