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

[Mark Rutland <mark.rutland@arm.com>]

On Thu, Jun 27, 2019 at 05:24:06PM +0100, James Morse wrote:
> 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.
> 
> I see this truncate stacks when walking from the SDEI handler...
> 
> 
> > 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);
> > +		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;
> 
> This is where it goes wrong. changed_stack is only true for the first frame on a newly
> visited stack. This means for the last frame of the previous stack, (which must point to
> the next stack), we still require 'frame->fp <= fp'.
> 
> It think like this just happens to be true for the irq stacks as they are allocated early.
> (whereas the SDEI stacks are allocated late).
> 
> 
> This hunk fixes it for me:
> ------------------------------------%<------------------------------------
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 8a1fa90c784d..cb5dee233ede 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -43,7 +43,6 @@
>  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)
> @@ -61,14 +60,16 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe
> *frame)
>         if (frame->stack_current != info.type) {
>                 set_bit(frame->stack_current, frame->stacks_done);
>                 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;
> +       if (info.low <= frame->fp && frame->fp <= info.high) {
> +               /* within stack bounds: the next frame must be older */
> +               if (frame->fp <= fp)
> +                       return -EINVAL;
> +       }

This fixes the cross-stack case, but it retains the check on the unwound
frame's fp, which may or may not be problematic, but it highlights that
we do something weird.

For frames A->B->C, we unwind A->B, then if C is on the same stack is B
we check whether B->C is sane.

I think that falls apart for cases which are already bad, e.g for:

	+---+
	| B |
	+---+
	| C |
	+---+
	| A |
	+---+

... we'd refuse to unwind A->B, whereas I think we should unwind A->B but
refuse to unwind B->C.

I think we need to keep track of the previous fp, and check that as part
of unwinding A->B. For the first unwind we can prime the frame with
STACK_TYPE_UNKNOWN, since any valid FP will have be treated as a
transition from that stack.

That seems to work in local testing, so I'll have a v2 shortly...

Thanks,
Mark.

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