| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20131205135928.GA4096@e103592.cambridge.arm.com>
Date: Thu, 5 Dec 2013 13:59:35 +0000
From: Dave Martin <Dave.Martin@....com>
To: Anurag Aggarwal <a.anurag@...sung.com>
Cc: "linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux@....linux.org.uk" <linux@....linux.org.uk>,
"cpgs@...sung.com" <cpgs@...sung.com>,
"narendra.m1@...sung.com" <narendra.m1@...sung.com>,
"poorva.s@...sung.com" <poorva.s@...sung.com>,
"naveen.sel@...sung.com" <naveen.sel@...sung.com>,
"ashish.kalra@...sung.com" <ashish.kalra@...sung.com>,
"mohammad.a2@...sung.com" <mohammad.a2@...sung.com>,
"rajat.suri@...sung.com" <rajat.suri@...sung.com>,
"naveenkrishna.ch@...il.com" <naveenkrishna.ch@...il.com>,
"anurag19aggarwal@...il.com" <anurag19aggarwal@...il.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Will Deacon <Will.Deacon@....com>,
"nico@...aro.org" <nico@...aro.org>,
Catalin Marinas <Catalin.Marinas@....com>
Subject: Re: [PATCH V4] ARM : unwinder : Prevent data abort due to stack
overflow
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@...sung.com>
> ---
> 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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists