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

[Justin Chen]

On 5/27/24 9:12 AM, 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.

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

> 
> 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>

Tested-by: Justin Chen <justin.chen at broadcom.com>

> ---
>   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,
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4206 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20240528/01fce01f/attachment.p7s>