ARM: ftrace: Don't assume stack frames are contiguous in memory
Subsystems (2)
PATCH 1/1 ARM: ftrace: Don't assume stack frames are contiguous in memory
From: Ard Biesheuvel <[email protected]>
The frame pointer unwinder relies on a standard layout of the stack frame, consisting of (in downward order)
Calling frame:
PC <---------+
LR | SP | FP | .. locals .. | Callee frame: | PC | LR | SP |
FP ----------+
where after storing its previous value on the stack, FP is made to point at the location of PC in the callee stack frame. The ftrace code assumes that this activation record is pushed first, and that any stack space for locals is allocated below this. This would imply that the caller's value of SP can be obtained by adding 4 to FP (which points to PC in the calling frame).
However, recent versions of GCC appear to deviate from this rule, and so the only reliable way to obtain the caller's value of SP is to read it from the activation record. Since this involves a read from memory rather than simple arithmetic, we need to use the uaccess API here which protects against inadvertent data aborts due to corruption of data on the stack.
The plain uaccess API is ftrace instrumented itself, so to avoid unbounded recursion, use the __get_kernel_nofault() primitive instead.
--- arch/arm/kernel/ftrace.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)
arch/arm/kernel/ftrace.c +15 -2
@@ -232,11 +232,24 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,unsigned long old;if (unlikely (atomic_read (¤t->tracing_graph_pause))) +err_out:return ;if (IS_ENABLED (CONFIG_UNWINDER_FRAME_POINTER )) { -/* FP points one word below parent's top of stack */ - frame_pointer +=4 ; +/* +* Usually, the stack frames are contiguous in memory but cases +* have been observed where the next stack frame does not live +* at 'frame_pointer + 4' as this code used to assume. +* +* Instead, dereference the field in the stack frame that +* stores the SP of the calling frame: to avoid unbounded +* recursion, this cannot involve any ftrace instrumented +* functions, so use the __get_kernel_nofault() primitive +* directly. +*/ +__get_kernel_nofault (&frame_pointer, + (unsigned long *)(frame_pointer -8 ), + unsigned long, err_out); }else {struct stackframe frame = { .fp = frame_pointer,
Signature · 7 lines
2.45.1.288.g0e0cd299f1-goog _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hello,
Quoted 34 lines · Ard Biesheuvel
From: Ard Biesheuvel <[email protected]>
The frame pointer unwinder relies on a standard layout of the stack frame, consisting of (in downward order)
Calling frame:
PC <---------+LR | SP | FP | .. locals .. | Callee frame: | PC | LR | SP |FP ----------+
where after storing its previous value on the stack, FP is made to point at the location of PC in the callee stack frame. The ftrace code assumes that this activation record is pushed first, and that any stack space for locals is allocated below this. This would imply that the caller's value of SP can be obtained by adding 4 to FP (which points to PC in the calling frame).
However, recent versions of GCC appear to deviate from this rule, and so the only reliable way to obtain the caller's value of SP is to read it from the activation record. Since this involves a read from memory rather than simple arithmetic, we need to use the uaccess API here which protects against inadvertent data aborts due to corruption of data on the stack.
The plain uaccess API is ftrace instrumented itself, so to avoid unbounded recursion, use the
__get_kernel_nofault()primitive instead.
Quoted 8 lines
Closes https://lore.kernel.org/all/alp44tukzo6mvcwl4ke4ehhmojrqnv6xfcdeuliybxfjfvgd3e@gpjvwj33cc76 open hereReported-by Uwe Kleine-König <[email protected]>Reported-by Justin Chen <[email protected]>Cc Thorsten Scherer <[email protected]>Cc Florian Fainelli <[email protected]>Cc Doug Berger <[email protected]>Signed-off-by Ard Biesheuvel <[email protected]>
Thank you for this patch.
This fixes the issue on our i.MX25 board (test based on v6.9).
Best regards
Thorsten
Quoted 38 lines
--- arch/arm/kernel/ftrace.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)arch/arm/kernel/ftrace.c +15
-2@@ -232,11 +232,24 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,unsigned long old;if (unlikely (atomic_read (¤t->tracing_graph_pause))) +err_out:return ;if (IS_ENABLED (CONFIG_UNWINDER_FRAME_POINTER )) { -/* FP points one word below parent's top of stack */ - frame_pointer +=4 ; +/* +* Usually, the stack frames are contiguous in memory but cases +* have been observed where the next stack frame does not live +* at 'frame_pointer + 4' as this code used to assume. +* +* Instead, dereference the field in the stack frame that +* stores the SP of the calling frame: to avoid unbounded +* recursion, this cannot involve any ftrace instrumented +* functions, so use the __get_kernel_nofault() primitive +* directly. +*/ +__get_kernel_nofault (&frame_pointer, + (unsigned long *)(frame_pointer -8 ), + unsigned long, err_out); }else {struct stackframe frame = { .fp = frame_pointer,Signature · 1 line
2.45.1.288.g0e0cd299f1-goog
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Quoted 34 lines · Ard Biesheuvel
From: Ard Biesheuvel <[email protected]>
The frame pointer unwinder relies on a standard layout of the stack frame, consisting of (in downward order)
Calling frame:
PC <---------+LR | SP | FP | .. locals .. | Callee frame: | PC | LR | SP |FP ----------+
where after storing its previous value on the stack, FP is made to point at the location of PC in the callee stack frame. The ftrace code assumes that this activation record is pushed first, and that any stack space for locals is allocated below this. This would imply that the caller's value of SP can be obtained by adding 4 to FP (which points to PC in the calling frame).
However, recent versions of GCC appear to deviate from this rule, and so the only reliable way to obtain the caller's value of SP is to read it from the activation record. Since this involves a read from memory rather than simple arithmetic, we need to use the uaccess API here which protects against inadvertent data aborts due to corruption of data on the stack.
The plain uaccess API is ftrace instrumented itself, so to avoid unbounded recursion, use the
__get_kernel_nofault()primitive instead.
Quoted 8 lines
Closes https://lore.kernel.org/all/alp44tukzo6mvcwl4ke4ehhmojrqnv6xfcdeuliybxfjfvgd3e@gpjvwj33cc76 open hereReported-by Uwe Kleine-König <[email protected]>Reported-by Justin Chen <[email protected]>Cc Thorsten Scherer <[email protected]>Cc Florian Fainelli <[email protected]>Cc Doug Berger <[email protected]>Signed-off-by Ard Biesheuvel <[email protected]>
Good catch and nice patch.
Yours,
Linus Walleij
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Quoted 31 lines · Ard Biesheuvel
From: Ard Biesheuvel <[email protected]>
The frame pointer unwinder relies on a standard layout of the stack frame, consisting of (in downward order)
Calling frame:
PC <---------+ LR | SP | FP | .. locals .. | Callee frame: | PC | LR | SP |FP ----------+
where after storing its previous value on the stack, FP is made to point at the location of PC in the callee stack frame. The ftrace code assumes that this activation record is pushed first, and that any stack space for locals is allocated below this. This would imply that the caller's value of SP can be obtained by adding 4 to FP (which points to PC in the calling frame).
However, recent versions of GCC appear to deviate from this rule, and so the only reliable way to obtain the caller's value of SP is to read it from the activation record. Since this involves a read from memory rather than simple arithmetic, we need to use the uaccess API here which protects against inadvertent data aborts due to corruption of data on the stack.
Quoted 2 lines
The plain uaccess API is ftrace instrumented itself, so to avoid unbounded recursion, use the
__get_kernel_nofault()primitive instead.
Apologies this feel through the cracks. Going back to the original thread, I said the first proposed fix worked, but I accidentally tested an old configuration. I was investigating why it didn't work and never got back to it. Looks like you found out why.
Thanks,
Justin
Quoted 9 lines
Closes https://lore.kernel.org/all/alp44tukzo6mvcwl4ke4ehhmojrqnv6xfcdeuliybxfjfvgd3e@gpjvwj33cc76 open hereReported-by Uwe Kleine-König <[email protected]>Reported-by Justin Chen <[email protected]>Cc Thorsten Scherer <[email protected]>Cc Florian Fainelli <[email protected]>Cc Doug Berger <[email protected]>Signed-off-by Ard Biesheuvel <[email protected]>
Quoted 25 lines
--- arch/arm/kernel/ftrace.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)arch/arm/kernel/ftrace.c +8
-2@@ -232,11 +232,24 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,unsigned long old;if (unlikely (atomic_read (¤t->tracing_graph_pause))) +err_out:return ;if (IS_ENABLED (CONFIG_UNWINDER_FRAME_POINTER )) { -/* FP points one word below parent's top of stack */ - frame_pointer +=4 ; +/* +* Usually, the stack frames are contiguous in memory but cases +* have been observed where the next stack frame does not live +* at 'frame_pointer + 4' as this code used to assume. +* +* Instead, dereference the field in the stack frame that +* stores the SP of the calling frame: to avoid unbounded
arch/arm/kernel/ftrace.c void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr, · line 255
Quoted 10 lines
arch/arm/kernel/ftrace.c +7
-0+* recursion, this cannot involve any ftrace instrumented +* functions, so use the __get_kernel_nofault() primitive +* directly. +*/ +__get_kernel_nofault (&frame_pointer, + (unsigned long *)(frame_pointer -8 ), + unsigned long, err_out); }else {struct stackframe frame = { .fp = frame_pointer,
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel