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

[Dave Martin <Dave.Martin@arm.com>]

On Fri, Jun 28, 2019 at 02:02:55PM +0100, Mark Rutland wrote:
> On Fri, Jun 28, 2019 at 12:15:03PM +0100, Dave Martin wrote:
> > 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).
> > 
> > I don't understand this.
> 
> I ended up drawing a diagram to figure this out, and realised James is
> right.
> 
> > Either we are on an edge frame (changed_stack is true) or not (false).
> > 
> > If not, the two FPs are on the same stack and we should compare them.
> > Otherwise they're on different stacks and such a comparison is nonsense.
> > 
> > I don't see any third situation.
> > 
> > So, what's wrong here?
> 
> The problem is that we unwind one frame, then check the fp of that
> frame.
> 
> Say we have three stack frames, A->B->C, where A and B are on the IRQ
> stack, and C is on the task stack.
> 
> At entry to unwind_frame(), frame describes A, and frame->fp points at
> B. Thus frame->stack_current == info.type, and changed_stack == false.
> 
> Then we sample B:
> 
> 	frame->fp = READ_ONCE(fp); // points at C on the task tasck
> 
> Then we do:
> 
> 	if (!changed_stack && frame->fp <= fp)
> 
> ... where changed_stack describes the A->B transition (false), but
> frame->fp <= fp is the B->C transition, and B and C are on different
> stacks!

OK, if I've understood that right, it looks like frame->stack_current
describes where the contents of frame were fetched from, not the frame
at frame->fp.

This is actually pretty confusing: the frame stack_current refers to is
already history: we have no pointer to it any more anyway.

I wonder whether this can be refactored so that stack_current doesn't
lag behind: say, call it fp_stack (the stack frame->fp points at).

That's just one option though.  I'll take a look at the repost.

Cheers
---Dave

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