[PATCH] ARM: ftrace: Don't assume stack frames are contiguous in memory

[Thorsten Scherer]

Hello,

On Mon, May 27, 2024 at 06:12:37PM +0200, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ardb at kernel.org>
> 
> 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.
> 
> Closes: https://lore.kernel.org/all/alp44tukzo6mvcwl4ke4ehhmojrqnv6xfcdeuliybxfjfvgd3e@gpjvwj33cc76
> Reported-by: Uwe Kleine-König <u.kleine-koenig at pengutronix.de>
> Closes: https://lore.kernel.org/all/d870c149-4363-43de-b0ea-7125dec5608e@broadcom.com/
> Reported-by: Justin Chen <justin.chen at broadcom.com>
> Cc: Thorsten Scherer <T.Scherer at eckelmann.de>
> Cc: Florian Fainelli <florian.fainelli at broadcom.com>
> Cc: Doug Berger <doug.berger at broadcom.com>
> Signed-off-by: Ard Biesheuvel <ardb at kernel.org>

Thank you for this patch.

This fixes the issue on our i.MX25 board (test based on v6.9).

Tested-by: Thorsten Scherer <t.scherer at eckelmann.de>

Best regards
Thorsten

> ---
>  arch/arm/kernel/ftrace.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c
> index a0b6d1e3812f..e61591f33a6c 100644
> --- a/arch/arm/kernel/ftrace.c
> +++ b/arch/arm/kernel/ftrace.c
> @@ -232,11 +232,24 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
>  	unsigned long old;
>  
>  	if (unlikely(atomic_read(&current->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,
> -- 
> 2.45.1.288.g0e0cd299f1-goog
>