Re: [PATCH V4] ARM : unwinder : Prevent data abort due to stackoverflow

[Dave Martin]

On Thu, Dec 05, 2013 at 11:26:25AM +0000, Anurag Aggarwal wrote:
> While unwinding backtrace, stack overflow is possible. This stack
> overflow can sometimes lead to data abort in system if the area after
> stack is not mapped to physical memory.
> 
> To prevent this problem from happening, execute the instructions that
> can cause a data abort in separate helper functions, where a check for
> feasibility is made before reading each word from the stack.
> 
> Signed-off-by: Anurag Aggarwal <a.anurag@xxxxxxxxxxx>
> ---
>  arch/arm/kernel/unwind.c |  127 ++++++++++++++++++++++++++++++++-------------
>  1 files changed, 90 insertions(+), 37 deletions(-)
> 
> diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
> index 00df012..94f6ef4 100644
> --- a/arch/arm/kernel/unwind.c
> +++ b/arch/arm/kernel/unwind.c
> @@ -68,6 +68,7 @@ EXPORT_SYMBOL(__aeabi_unwind_cpp_pr2);
>  struct unwind_ctrl_block {
>  	unsigned long vrs[16];		/* virtual register set */
>  	const unsigned long *insn;	/* pointer to the current instructions word */
> +	unsigned long sp_high;		/* highest value of sp allowed*/
>  	int entries;			/* number of entries left to interpret */
>  	int byte;			/* current byte number in the instructions word */
>  };
> @@ -235,6 +236,86 @@ static unsigned long unwind_get_byte(struct unwind_ctrl_block *ctrl)
>  	return ret;
>  }
>  
> +/* Before poping a register check whether it is feasible or not */
> +static int unwind_pop_register(struct unwind_ctrl_block *ctrl,
> +				unsigned long **vsp, unsigned int reg)
> +{
> +	if (*vsp >= (unsigned long *)ctrl->sp_high)
> +		return -URC_FAILURE;
> +
> +	ctrl->vrs[reg] = *(*vsp)++;
> +	return URC_OK;

It occurred to me that your optimisation can still be implemented on
top on this.

If you add an extra flag parameter to unwind_pop_register telling it
whether to do checking or not, I think that the compiler and/or
CPU branch predictor should be able to do a pretty good job of
optimising the common case.  Until we get near sp_high, if(check) will
branch the same way every time.

if (unlikely(check) &&
    *vsp >= (unsigned long *)ctrl->sp_high)) 

would make sense, in fact.


I think this brings most of the benefit, without needing to code the
insn exec rules twice.

I'm still not sure the optimisation benefits us much, but I think
that would be a tidier way of doing it if you want to try.

> +}
> +
> +/* Helper functions to execute the instructions */
> +static int unwind_exec_pop_subset_r4_to_r13(struct unwind_ctrl_block *ctrl,
> +						unsigned long mask)
> +{
> +	unsigned long *vsp = (unsigned long *)ctrl->vrs[SP];
> +	int load_sp, reg = 4;
> +
> +	load_sp = mask & (1 << (13 - 4));
> +	while (mask) {
> +		if (mask & 1)
> +			if (unwind_pop_register(ctrl, &vsp, reg))
> +				return -URC_FAILURE;
> +		mask >>= 1;
> +		reg++;
> +	}
> +	if (!load_sp)
> +		ctrl->vrs[SP] = (unsigned long)vsp;
> +
> +	pr_debug("%s: fp = %08lx sp = %08lx lr = %08lx pc = %08lx\n", __func__,
> +		ctrl->vrs[FP], ctrl->vrs[SP], ctrl->vrs[LR], ctrl->vrs[PC]);

Minor-ish nit: you now duplicate this same pr_debug() in many places.
Apologies, I didn't spot that in the previous review.

What about something like this:

static int unwind_exec_insn(...)
{
	int ret = URC_OK;

	} else if (...)
		ret = unwind_exec_pop_subset_r4_to_r13(...);
		if (ret)
			goto error;
	else ...

	pr_debug(...);

error:
	return ret;
}

Then everything returns through the same pr_debug() after the insn has
been executed.

[...]

> @@ -329,13 +382,13 @@ static int unwind_exec_insn(struct unwind_ctrl_block *ctrl)
>   */
>  int unwind_frame(struct stackframe *frame)
>  {
> -	unsigned long high, low;
> +	unsigned long low;
>  	const struct unwind_idx *idx;
>  	struct unwind_ctrl_block ctrl;
>  
> -	/* only go to a higher address on the stack */
> +	/* store the highest address on the stack to avoid crossing it*/
>  	low = frame->sp;
> -	high = ALIGN(low, THREAD_SIZE);
> +	ctrl.sp_high = ALIGN(low, THREAD_SIZE);

Does the code check anywhere that SP is word-aligned?

That should probably be checked if it's not done already.

I have some unrelated changes I want to make in this file (which is
part of why I'm being pushy about getting things nice and clean) ... so
I'm happy to follow up with that as a separate patch later.  It's a
separate issue, really.  It doesn't necessarily belong in this patch.

Cheers
---Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/