Re: [PATCH 3/3] arm64: stacktrace: better handle corrupted stacks

From: James Morse <james.morse@arm•com>
To: Mark Rutland <mark.rutland@arm•com>
Cc: catalin.marinas@arm•com, tengfeif@codeaurora•org,
	will.deacon@arm•com, dave.martin@arm•com,
	linux-arm-kernel@lists•infradead.org
Subject: Re: [PATCH 3/3] arm64: stacktrace: better handle corrupted stacks
Date: Mon, 24 Jun 2019 12:34:40 +0100	[thread overview]
Message-ID: <7c70ab1c-e114-9d21-e37b-3d4e01ac6e43@arm.com> (raw)
In-Reply-To: <20190606125402.10229-4-mark.rutland@arm.com>

Hi Mark,

On 06/06/2019 13:54, Mark Rutland wrote:
> The arm64 stacktrace code is careful to only dereference frame records
> in valid stack ranges, ensuring that a corrupted frame record won't
> result in a faulting access.
> 
> However, it's still possible for corrupt frame records to result in
> infinite loops in the stacktrace code, which is also undesirable.
> 
> This patch ensures that we complete a stacktrace in finite time, by
> keeping track of which stacks we have already completed unwinding, and
> verifying that if the next frame record is on the same stack, it is at a
> higher address.

This looks good, I tried to take it for a spin to test SDEI stack tracing ... but it
wouldn't boot, it panic()s before earlycon.

defconfig doesn't do this, defconfig+CONFIG_PROVE_LOCKING does.
Toggling CONFIG_DEBUG_LOCK_ALLOC is the smallest config change to make this show up.

Its taking a translation fault:
| <__ll_sc_arch_atomic64_or>:
|        f9800031        prfm    pstl1strm, [x1]
|        c85f7c31        ldxr    x17, [x1]		(faulting instruction)
|        aa000231        orr     x17, x17, x0
|        c8107c31        stxr    w16, x17, [x1]
|        35ffffb0        cbnz    w16, ffff000010c7d19c <__ll_sc_a
|        d65f03c0        ret

x0: 0x0000000000000100
x1: 0xffff0000137399e8			(far_el2)

If x1 were part of 'frame' in __save_stack_trace it should be on the stack, but at
fault-time sp is 0xffff0000114a3a50. This happens before the linear map has been set up....

The lr points just after the set_bit() call in unwind_frame().

> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index b00ec7d483d1..1c45b33c7474 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -43,6 +43,8 @@
>  int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
>  {
>  	unsigned long fp = frame->fp;
> +	bool changed_stack = false;
> +	struct stack_info info;
>  
>  	if (fp & 0xf)
>  		return -EINVAL;
> @@ -50,12 +52,24 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
>  	if (!tsk)
>  		tsk = current;
>  
> -	if (!on_accessible_stack(tsk, fp, NULL))
> +	if (!on_accessible_stack(tsk, fp, &info))
>  		return -EINVAL;
>  
> +	if (test_bit(info.type, frame->stacks_done))
> +		return -EINVAL;
> +
> +	if (frame->stack_current != info.type) {

> +		set_bit(frame->stack_current, frame->stacks_done);

Changing this line:
| -               set_bit(frame->stack_current, frame->stacks_done);
| +               *frame->stacks_done |= (1 << frame->stack_current);
works fine.

But it doesn't cause a stacktrace to be printed, so I can't work out how
CONFIG_DEBUG_LOCK_ALLOC is relevant.

... this makes no sense, can anyone else reproduce it?

Thanks,

James

> +		frame->stack_current = info.type;
> +		changed_stack = true;
> +	}
> +
>  	frame->fp = READ_ONCE_NOCHECK(*(unsigned long *)(fp));
>  	frame->pc = READ_ONCE_NOCHECK(*(unsigned long *)(fp + 8));
>  
> +	if (!changed_stack && frame->fp <= fp)
> +		return -EINVAL;
> +
>  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
>  	if (tsk->ret_stack &&
>  			(frame->pc == (unsigned long)return_to_handler)) {

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists•infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel