| 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
| ||
|
Date: Tue, 3 Dec 2013 13:18:11 +0000
From: Dave Martin <Dave.Martin@....com>
To: Anurag Aggarwal <a.anurag@...sung.com>
Cc: linux@....linux.org.uk, linux-arm-kernel@...ts.infradead.org,
naveen.sel@...sung.com, narendra.m1@...sung.com, nico@...aro.org,
catalin.marinas@....com, will.deacon@....com,
linux-kernel@...r.kernel.org, ashish.kalra@...sung.com,
cpgs@...sung.com, anurag19aggarwal@...il.com,
naveenkrishna.ch@...il.com, rajat.suri@...sung.com,
poorva.s@...sung.com, mohammad.a2@...sung.com
Subject: Re: [PATCH V3] ARM : unwinder : Prevent data abort due to stack
overflow
On Tue, Dec 03, 2013 at 02:56:25PM +0530, Anurag Aggarwal wrote:
> Signed-off-by: Anurag Aggarwal <a.anurag@...sung.com>
Move the S-o-b line to the end of your commit message. I'm getting
tired of saying this.
>
> 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 data from the stack.
This is looking better now overall, but please take a moment to respond
to the comments I just made on the v2 thread.
Other comments on this patch below.
Cheers
---Dave
>
> ---
> arch/arm/kernel/unwind.c | 207 ++++++++++++++++++++++++++++++++++------------
> 1 files changed, 153 insertions(+), 54 deletions(-)
>
> diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
> index 00df012..a140725 100644
> --- a/arch/arm/kernel/unwind.c
> +++ b/arch/arm/kernel/unwind.c
> @@ -49,6 +49,8 @@
> #include <asm/traps.h>
> #include <asm/unwind.h>
>
> +#define TOTAL_REGISTERS 16
> +
> /* Dummy functions to avoid linker complaints */
> void __aeabi_unwind_cpp_pr0(void)
> {
> @@ -66,8 +68,9 @@ void __aeabi_unwind_cpp_pr2(void)
> EXPORT_SYMBOL(__aeabi_unwind_cpp_pr2);
>
> struct unwind_ctrl_block {
> - unsigned long vrs[16]; /* virtual register set */
> + unsigned long vrs[TOTAL_REGISTERS]; /* 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 +238,148 @@ static unsigned long unwind_get_byte(struct unwind_ctrl_block *ctrl)
> return ret;
> }
>
> +static int unwind_exec_pop_subset_r4_to_r13(struct unwind_ctrl_block *ctrl,
> + unsigned long insn)
> +{
> + int available_stack;
> + unsigned long mask;
> + unsigned long *vsp = (unsigned long *)ctrl->vrs[SP];
> + int load_sp, reg = 4;
> +
> + /* caculate the space available on stack */
> + available_stack = ctrl->sp_high - ctrl->vrs[SP];
sp_high and vrs[SP] are just integers, to available_stack is a number of
bytes, not words.
All your checks involving available_sp seem to assume words.
(Previously this was correct because you computed available_stack by
subtracting two unsigned long *).
> +
> + insn = (insn << 8) | unwind_get_byte(ctrl);
> + mask = insn & 0x0fff;
> + if (mask == 0) {
> + pr_warning("unwind: 'Refuse to unwind' instruction %04lx\n",
> + insn);
> + return -URC_FAILURE;
> + }
For cleanliness, we should keep decode in unwind_exec_insn() and do
only the execution of the specific insn in these functions.
mask could be a parameter instead of insn, for example.
> +
> + /*
> + * Check whether there is enough space
> + * on stack to execute the instruction
> + * if not then return failure
(Minor nit: in these multiline comments, please use just one space
after *. Also, feel free to use longer lines up to 79 chars -- this
could fit on two lines. My example had short lines because I needed to
demonstrate a multiline comment...)
> + */
> + if (available_stack < TOTAL_REGISTERS) {
> + unsigned long mask_copy = mask;
> + int required_stack = 0;
> +
> + while (mask_copy) {
> + if (mask_copy & 1)
> + required_stack++;
> + mask_copy >>= 1;
> + }
> +
> + if (available_stack < required_stack)
> + return -URC_FAILURE;
> + }
> +
> + load_sp = mask & (1 << (13 - 4));
> + while (mask) {
> + if (mask & 1)
> + ctrl->vrs[reg] = *vsp++;
> + 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]);
> +
> + return URC_OK;
> +}
> +
> +static int unwind_exec_pop_r4_to_rN(struct unwind_ctrl_block *ctrl,
> + unsigned long insn)
> +{
> + int available_stack;
> + unsigned long *vsp = (unsigned long *)ctrl->vrs[SP];
> + int reg;
> +
> + /* caculate the space available on stack */
> + available_stack = ctrl->sp_high - ctrl->vrs[SP];
> +
> + /*
> + * Check whether there is enough space
> + * on stack to execute the instruction
> + * if not then return failure
> + */
> + if (available_stack < TOTAL_REGISTERS) {
> + int required_stack;
> +
> + required_stack = insn & 7;
> + required_stack += (insn & 0x80) ? 1 : 0;
> +
> + if (available_stack < required_stack)
> + return -URC_FAILURE;
> + }
> +
> + /* pop R4-R[4+bbb] */
> + for (reg = 4; reg <= 4 + (insn & 7); reg++)
> + ctrl->vrs[reg] = *vsp++;
> + if (insn & 0x80)
> + ctrl->vrs[14] = *vsp++;
> + 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]);
> +
> + return URC_OK;
> +}
> +
> +static int unwind_exec_pop_subset_r0_to_r3(struct unwind_ctrl_block *ctrl,
> + unsigned long insn)
> +{
> + int available_stack;
> + unsigned long mask = unwind_get_byte(ctrl);
> + unsigned long *vsp = (unsigned long *)ctrl->vrs[SP];
> + int reg = 0;
> +
> + if (mask == 0 || mask & 0xf0) {
> + pr_warning("unwind: Spare encoding %04lx\n",
> + (insn << 8) | mask);
> + return -URC_FAILURE;
> + }
> +
> + /* caculate the space available on stack */
> + available_stack = ctrl->sp_high - ctrl->vrs[SP];
> +
> + /*
> + * Check whether there is enough space
> + * on stack to execute the instruction
> + * if not then return failure
> + */
> + if (available_stack < TOTAL_REGISTERS) {
> + unsigned long mask_copy = mask;
> + int required_stack = 0;
> +
> + while (mask_copy) {
> + if (mask_copy & 1)
> + required_stack++;
> + mask_copy >>= 1;
> + }
> + if (available_stack < required_stack)
> + return -URC_FAILURE;
> + }
> +
> + /* pop R0-R3 according to mask */
> + while (mask) {
> + if (mask & 1)
> + ctrl->vrs[reg] = *vsp++;
> + mask >>= 1;
> + reg++;
> + }
> + 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]);
> +
> + return URC_OK;
> +}
> +
> /*
> * Execute the current unwind instruction.
> */
> @@ -249,65 +394,19 @@ static int unwind_exec_insn(struct unwind_ctrl_block *ctrl)
> else if ((insn & 0xc0) == 0x40)
> ctrl->vrs[SP] -= ((insn & 0x3f) << 2) + 4;
> else if ((insn & 0xf0) == 0x80) {
Many of the { } in this ifelse are now unnecessary. You should remove
the ones that aren't needed any more.
> - unsigned long mask;
> - unsigned long *vsp = (unsigned long *)ctrl->vrs[SP];
> - int load_sp, reg = 4;
> -
> - insn = (insn << 8) | unwind_get_byte(ctrl);
> - mask = insn & 0x0fff;
> - if (mask == 0) {
> - pr_warning("unwind: 'Refuse to unwind' instruction %04lx\n",
> - insn);
> - return -URC_FAILURE;
> - }
> -
> - /* pop R4-R15 according to mask */
> - load_sp = mask & (1 << (13 - 4));
> - while (mask) {
> - if (mask & 1)
> - ctrl->vrs[reg] = *vsp++;
> - mask >>= 1;
> - reg++;
> - }
> - if (!load_sp)
> - ctrl->vrs[SP] = (unsigned long)vsp;
> + return unwind_exec_pop_subset_r4_to_r13(ctrl, insn);
> } else if ((insn & 0xf0) == 0x90 &&
> (insn & 0x0d) != 0x0d)
> ctrl->vrs[SP] = ctrl->vrs[insn & 0x0f];
> else if ((insn & 0xf0) == 0xa0) {
> - unsigned long *vsp = (unsigned long *)ctrl->vrs[SP];
> - int reg;
> -
> - /* pop R4-R[4+bbb] */
> - for (reg = 4; reg <= 4 + (insn & 7); reg++)
> - ctrl->vrs[reg] = *vsp++;
> - if (insn & 0x80)
> - ctrl->vrs[14] = *vsp++;
> - ctrl->vrs[SP] = (unsigned long)vsp;
> + return unwind_exec_pop_r4_to_rN(ctrl, insn);
> } else if (insn == 0xb0) {
> if (ctrl->vrs[PC] == 0)
> ctrl->vrs[PC] = ctrl->vrs[LR];
> /* no further processing */
> ctrl->entries = 0;
> } else if (insn == 0xb1) {
> - unsigned long mask = unwind_get_byte(ctrl);
> - unsigned long *vsp = (unsigned long *)ctrl->vrs[SP];
> - int reg = 0;
> -
> - if (mask == 0 || mask & 0xf0) {
> - pr_warning("unwind: Spare encoding %04lx\n",
> - (insn << 8) | mask);
> - return -URC_FAILURE;
> - }
> -
> - /* pop R0-R3 according to mask */
> - while (mask) {
> - if (mask & 1)
> - ctrl->vrs[reg] = *vsp++;
> - mask >>= 1;
> - reg++;
> - }
> - ctrl->vrs[SP] = (unsigned long)vsp;
> + return unwind_exec_pop_subset_r0_to_r3(ctrl, insn);
> } else if (insn == 0xb2) {
> unsigned long uleb128 = unwind_get_byte(ctrl);
>
> @@ -329,13 +428,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);
>
> pr_debug("%s(pc = %08lx lr = %08lx sp = %08lx)\n", __func__,
> frame->pc, frame->lr, frame->sp);
> @@ -386,7 +485,7 @@ int unwind_frame(struct stackframe *frame)
> int urc = unwind_exec_insn(&ctrl);
> if (urc < 0)
> return urc;
> - if (ctrl.vrs[SP] < low || ctrl.vrs[SP] >= high)
> + if (ctrl.vrs[SP] < low || ctrl.vrs[SP] >= ctrl.sp_high)
> return -URC_FAILURE;
> }
>
> --
> 1.7.0.4
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
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