Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1686080 > unrolled thread
| Started by | Mark Rutland <mark.rutland@arm.com> |
|---|---|
| First post | 2017-07-13 00:40 +0200 |
| Last post | 2017-07-13 13:30 +0200 |
| Articles | 20 on this page of 29 — 5 participants |
Back to article view | Back to linux.kernel
[RFC PATCH 0/6] arm64: alternative VMAP_STACK implementation Mark Rutland <mark.rutland@arm.com> - 2017-07-13 00:40 +0200
[RFC PATCH 5/6] arm64: keep track of current stack Mark Rutland <mark.rutland@arm.com> - 2017-07-13 00:40 +0200
[RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP Mark Rutland <mark.rutland@arm.com> - 2017-07-13 00:40 +0200
Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-07-13 09:00 +0200
Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP Mark Rutland <mark.rutland@arm.com> - 2017-07-13 13:00 +0200
Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-07-13 13:50 +0200
Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP Mark Rutland <mark.rutland@arm.com> - 2017-07-13 18:20 +0200
Re: [kernel-hardening] Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP Mark Rutland <mark.rutland@arm.com> - 2017-07-13 20:00 +0200
Re: [kernel-hardening] Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-07-13 20:30 +0200
Re: [kernel-hardening] Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP Mark Rutland <mark.rutland@arm.com> - 2017-07-14 12:40 +0200
Re: [kernel-hardening] Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-07-14 12:50 +0200
Re: [kernel-hardening] Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-07-14 14:30 +0200
Re: [kernel-hardening] Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP Mark Rutland <mark.rutland@arm.com> - 2017-07-14 16:10 +0200
Re: [kernel-hardening] Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-07-14 16:20 +0200
Re: [kernel-hardening] Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP Robin Murphy <robin.murphy@arm.com> - 2017-07-14 16:40 +0200
Re: [kernel-hardening] Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP Robin Murphy <robin.murphy@arm.com> - 2017-07-14 17:10 +0200
Re: [kernel-hardening] Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-07-14 17:20 +0200
Re: [kernel-hardening] Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP Mark Rutland <mark.rutland@arm.com> - 2017-07-14 17:30 +0200
Re: [kernel-hardening] Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP Mark Rutland <mark.rutland@arm.com> - 2017-07-14 23:30 +0200
Re: [kernel-hardening] Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-07-16 02:10 +0200
Re: [kernel-hardening] Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP Mark Rutland <mark.rutland@arm.com> - 2017-07-14 15:00 +0200
Re: [kernel-hardening] Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-07-14 15:00 +0200
[RFC PATCH 4/6] arm64: pass stack base to secondary_start_kernel Mark Rutland <mark.rutland@arm.com> - 2017-07-13 00:40 +0200
[RFC PATCH 1/6] arm64: use tpidr_el1 for current, free sp_el0 Mark Rutland <mark.rutland@arm.com> - 2017-07-13 00:40 +0200
Re: [RFC PATCH 1/6] arm64: use tpidr_el1 for current, free sp_el0 Will Deacon <will.deacon@arm.com> - 2017-07-14 03:40 +0200
[RFC PATCH 3/6] arm64: pad stacks to PAGE_SIZE for VMAP_STACK Mark Rutland <mark.rutland@arm.com> - 2017-07-13 00:40 +0200
[RFC PATCH 2/6] arm64: avoid open-coding THREAD_SIZE{,_ORDER} Mark Rutland <mark.rutland@arm.com> - 2017-07-13 00:40 +0200
Re: [RFC PATCH 2/6] arm64: avoid open-coding THREAD_SIZE{,_ORDER} James Morse <james.morse@arm.com> - 2017-07-13 12:20 +0200
Re: [RFC PATCH 2/6] arm64: avoid open-coding THREAD_SIZE{,_ORDER} Mark Rutland <mark.rutland@arm.com> - 2017-07-13 13:30 +0200
Page 1 of 2 [1] 2 Next page →
| From | Mark Rutland <mark.rutland@arm.com> |
|---|---|
| Date | 2017-07-13 00:40 +0200 |
| Subject | [RFC PATCH 0/6] arm64: alternative VMAP_STACK implementation |
| Message-ID | <u2ysy-5Ke-5@gated-at.bofh.it> |
Hi,
While reviewing Ard's VMAP_STACK series [1], I tried to put together some notes
based on my prior aborted attempts, and tricked myself into turning them into
this series. I suspect we'll want bits of both.
Like Ard's series, this doesn't use EL1t mode, and instead performs a check
early in el1_sync. However, there are a few differences:
* This series frees up SP_EL0, and inverts the current<->percpu relationship
rather than using a GPR for current.
* The out-of-bounds detection *only* considers the SP. Stray accesses below the
SP will be handled as regular faults, unless handling these triggers a stack
overflow.
* There is a dedicated handler for the stack out-of-bounds case (as with x86),
rather than piggy-backing on the usual fault handling code.
* The overflow checks consider IRQ stacks, by keeping track of which stack a
task is currently using. This assumes all stacks are the same size (which
happens to be true today), but we should make that explicit by using common
definitions. Thanks to James Morse for pointing out that we need to handle
this.
Currently the IRQ stacks don't have a guaranteed guard pages, as they're
regular compile-time percpu reservations. We'll want to rework those so that
they have guards.
I haven't audited the backtracing code, but I suspect we'll need to fix up any
stack walking code up so that it understands there are now three possible
stacks that a task may be using, and so that we can walk emergency->irq->task
stack traces.
Otherwise, this series is rough around the seams, and has seen only the most
trivial of testing on a Juno platform (booting 4K and 64K kernels with and
without a deliberate overflow).
I've pushed the series out to my git repo as arm64/vmap-stack [2].
Thanks,
Mark.
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2017-July/518368.html
[2] git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git arm64/vmap-stack
Mark Rutland (6):
arm64: use tpidr_el1 for current, free sp_el0
arm64: avoid open-coding THREAD_SIZE{,_ORDER}
arm64: pad stacks to PAGE_SIZE for VMAP_STACK
arm64: pass stack base to secondary_start_kernel
arm64: keep track of current stack
arm64: add VMAP_STACK and detect out-of-bounds SP
arch/arm64/Kconfig | 1 +
arch/arm64/include/asm/assembler.h | 11 +++++--
arch/arm64/include/asm/current.h | 6 ++--
arch/arm64/include/asm/percpu.h | 15 +++------
arch/arm64/include/asm/thread_info.h | 22 ++++++++++---
arch/arm64/kernel/asm-offsets.c | 4 +++
arch/arm64/kernel/entry.S | 61 ++++++++++++++++++++++++++++++------
arch/arm64/kernel/head.S | 13 ++++++--
arch/arm64/kernel/process.c | 20 +++++-------
arch/arm64/kernel/smp.c | 2 +-
arch/arm64/kernel/traps.c | 21 +++++++++++++
11 files changed, 130 insertions(+), 46 deletions(-)
--
1.9.1
[toc] | [next] | [standalone]
| From | Mark Rutland <mark.rutland@arm.com> |
|---|---|
| Date | 2017-07-13 00:40 +0200 |
| Subject | [RFC PATCH 5/6] arm64: keep track of current stack |
| Message-ID | <u2ysy-5Ke-15@gated-at.bofh.it> |
| In reply to | #1686080 |
To reliably check stack bounds, we'll need to know whether we're on a
task stack, or an IRQ stack.
Stash the base of the current stack in thread_info so that we have this
information.
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
arch/arm64/include/asm/thread_info.h | 3 +++
arch/arm64/kernel/asm-offsets.c | 3 +++
arch/arm64/kernel/entry.S | 7 +++++++
arch/arm64/kernel/head.S | 6 ++++++
arch/arm64/kernel/process.c | 4 ++++
5 files changed, 23 insertions(+)
diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
index 3684f86..ae4f44b 100644
--- a/arch/arm64/include/asm/thread_info.h
+++ b/arch/arm64/include/asm/thread_info.h
@@ -62,6 +62,9 @@ struct thread_info {
#endif
unsigned long pcp_offset;
int preempt_count; /* 0 => preemptable, <0 => bug */
+#ifdef CONFIG_VMAP_STACK
+ unsigned long current_stack;
+#endif
};
#define INIT_THREAD_INFO(tsk) \
diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index 17001be..10c8ffa 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c
@@ -40,6 +40,9 @@ int main(void)
DEFINE(TSK_TI_PREEMPT, offsetof(struct task_struct, thread_info.preempt_count));
DEFINE(TSK_TI_PCP, offsetof(struct task_struct, thread_info.pcp_offset));
DEFINE(TSK_TI_ADDR_LIMIT, offsetof(struct task_struct, thread_info.addr_limit));
+#ifdef CONFIG_VMAP_STACK
+ DEFINE(TSK_TI_CUR_STK, offsetof(struct task_struct, thread_info.current_stack));
+#endif
#ifdef CONFIG_ARM64_SW_TTBR0_PAN
DEFINE(TSK_TI_TTBR0, offsetof(struct task_struct, thread_info.ttbr0));
#endif
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 773b3fea..7c8b164 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -258,6 +258,9 @@ alternative_else_nop_endif
/* switch to the irq stack */
mov sp, x26
+#ifdef CONFIG_VMAP_STACK
+ str x25, [tsk, #TSK_TI_CUR_STK]
+#endif
/*
* Add a dummy stack frame, this non-standard format is fixed up
@@ -275,6 +278,10 @@ alternative_else_nop_endif
*/
.macro irq_stack_exit
mov sp, x19
+#ifdef CONFIG_VMAP_STACK
+ and x19, x19, #~(THREAD_SIZE - 1)
+ str x19, [tsk, #TSK_TI_CUR_STK]
+#endif
.endm
/*
diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index db77cac..3363846 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -325,6 +325,9 @@ __primary_switched:
add sp, x4, #THREAD_SIZE
adr_l x5, init_task
msr tpidr_el1, x5 // Save thread_info
+#ifdef CONFIG_VMAP_STACK
+ str x4, [x5, #TSK_TI_CUR_STK]
+#endif
adr_l x8, vectors // load VBAR_EL1 with virtual
msr vbar_el1, x8 // vector table address
@@ -616,6 +619,9 @@ __secondary_switched:
mov x3, #THREAD_START_SP
add sp, x1, x3
ldr x2, [x0, #CPU_BOOT_TASK]
+#ifdef CONFIG_VMAP_STACK
+ str x1, [x2, #TSK_TI_CUR_STK]
+#endif
msr tpidr_el1, x2
mov x29, #0
b secondary_start_kernel
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 4212da3..5dc5797 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -294,6 +294,10 @@ int copy_thread(unsigned long clone_flags, unsigned long stack_start,
ptrace_hw_copy_thread(p);
+#ifdef CONFIG_VMAP_STACK
+ p->thread_info.current_stack = (unsigned long)p->stack;
+#endif
+
return 0;
}
--
1.9.1
[toc] | [prev] | [next] | [standalone]
| From | Mark Rutland <mark.rutland@arm.com> |
|---|---|
| Date | 2017-07-13 00:40 +0200 |
| Subject | [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP |
| Message-ID | <u2ysy-5Ke-13@gated-at.bofh.it> |
| In reply to | #1686080 |
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
arch/arm64/Kconfig | 1 +
arch/arm64/kernel/entry.S | 43 +++++++++++++++++++++++++++++++++++++++++++
arch/arm64/kernel/traps.c | 21 +++++++++++++++++++++
3 files changed, 65 insertions(+)
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index b2024db..5cbd961 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1,5 +1,6 @@
config ARM64
def_bool y
+ select HAVE_ARCH_VMAP_STACK
select ACPI_CCA_REQUIRED if ACPI
select ACPI_GENERIC_GSI if ACPI
select ACPI_GTDT if ACPI
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 7c8b164..e0fdb65 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -396,11 +396,54 @@ el1_error_invalid:
inv_entry 1, BAD_ERROR
ENDPROC(el1_error_invalid)
+#ifdef CONFIG_VMAP_STACK
+.macro detect_bad_stack
+ msr sp_el0, x0
+ get_thread_info x0
+ ldr x0, [x0, #TSK_TI_CUR_STK]
+ sub x0, sp, x0
+ and x0, x0, #~(THREAD_SIZE - 1)
+ cbnz x0, __bad_stack
+ mrs x0, sp_el0
+.endm
+
+__bad_stack:
+ /*
+ * Stash the bad SP, and free up another GPR. We no longer care about
+ * EL0 state, since this thread cannot recover.
+ */
+ mov x0, sp
+ msr tpidrro_el0, x0
+ msr tpidr_el0, x1
+
+ /* Move to the emergency stack */
+ adr_this_cpu x0, bad_stack, x1
+ mov x1, #THREAD_START_SP
+ add sp, x0, x1
+
+ /* Restore GPRs and log them to pt_regs */
+ mrs x0, sp_el0
+ mrs x1, tpidr_el0
+ kernel_entry 1
+
+ /* restore the bad SP to pt_regs */
+ mrs x1, tpidrro_el0
+ str x1, [sp, #S_SP]
+
+ /* Time to die */
+ mov x0, sp
+ b handle_bad_stack
+#else
+.macro detect_bad_stack
+.endm
+#endif
+
/*
* EL1 mode handlers.
*/
.align 6
el1_sync:
+ detect_bad_stack
kernel_entry 1
mrs x1, esr_el1 // read the syndrome register
lsr x24, x1, #ESR_ELx_EC_SHIFT // exception class
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 0805b44..84b00e3 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -683,6 +683,27 @@ asmlinkage void bad_el0_sync(struct pt_regs *regs, int reason, unsigned int esr)
force_sig_info(info.si_signo, &info, current);
}
+#ifdef CONFIG_VMAP_STACK
+DEFINE_PER_CPU(unsigned long [IRQ_STACK_SIZE/sizeof(long)], bad_stack) __aligned(16);
+
+asmlinkage void handle_bad_stack(struct pt_regs *regs)
+{
+ unsigned long tsk_stk = (unsigned long)current->stack;
+ unsigned long irq_stk = (unsigned long)per_cpu(irq_stack, smp_processor_id());
+
+ console_verbose();
+ pr_emerg("Stack out-of-bounds!\n"
+ "\tsp: 0x%016lx\n"
+ "\ttsk stack: [0x%016lx..0x%016lx]\n"
+ "\tirq stack: [0x%016lx..0x%016lx]\n",
+ kernel_stack_pointer(regs),
+ tsk_stk, tsk_stk + THREAD_SIZE,
+ irq_stk, irq_stk + THREAD_SIZE);
+ show_regs(regs);
+ panic("stack out-of-bounds");
+}
+#endif
+
void __pte_error(const char *file, int line, unsigned long val)
{
pr_err("%s:%d: bad pte %016lx.\n", file, line, val);
--
1.9.1
[toc] | [prev] | [next] | [standalone]
| From | Ard Biesheuvel <ard.biesheuvel@linaro.org> |
|---|---|
| Date | 2017-07-13 09:00 +0200 |
| Subject | Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP |
| Message-ID | <u2Ggq-2d9-9@gated-at.bofh.it> |
| In reply to | #1686085 |
Hi Mark,
On 12 July 2017 at 23:33, Mark Rutland <mark.rutland@arm.com> wrote:
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> ---
> arch/arm64/Kconfig | 1 +
> arch/arm64/kernel/entry.S | 43 +++++++++++++++++++++++++++++++++++++++++++
> arch/arm64/kernel/traps.c | 21 +++++++++++++++++++++
> 3 files changed, 65 insertions(+)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index b2024db..5cbd961 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1,5 +1,6 @@
> config ARM64
> def_bool y
> + select HAVE_ARCH_VMAP_STACK
> select ACPI_CCA_REQUIRED if ACPI
> select ACPI_GENERIC_GSI if ACPI
> select ACPI_GTDT if ACPI
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 7c8b164..e0fdb65 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -396,11 +396,54 @@ el1_error_invalid:
> inv_entry 1, BAD_ERROR
> ENDPROC(el1_error_invalid)
>
> +#ifdef CONFIG_VMAP_STACK
> +.macro detect_bad_stack
> + msr sp_el0, x0
> + get_thread_info x0
> + ldr x0, [x0, #TSK_TI_CUR_STK]
> + sub x0, sp, x0
> + and x0, x0, #~(THREAD_SIZE - 1)
> + cbnz x0, __bad_stack
> + mrs x0, sp_el0
The typical prologue looks like
stp x29, x30, [sp, #-xxx]!
stp x27, x28, [sp, #xxx]
...
mov x29, sp
which means that in most cases where we do run off the stack, sp will
still be pointing into it when the exception is taken. This means we
will fault recursively in the handler before having had the chance to
accurately record the exception context.
Given that the max displacement of a store instruction is 512 bytes,
and that the frame size we are about to stash exceeds that, should we
already consider it a stack fault if sp is within 512 bytes (or
S_FRAME_SIZE) of the base of the stack?
> +.endm
> +
> +__bad_stack:
> + /*
> + * Stash the bad SP, and free up another GPR. We no longer care about
> + * EL0 state, since this thread cannot recover.
> + */
> + mov x0, sp
> + msr tpidrro_el0, x0
> + msr tpidr_el0, x1
> +
> + /* Move to the emergency stack */
> + adr_this_cpu x0, bad_stack, x1
> + mov x1, #THREAD_START_SP
> + add sp, x0, x1
> +
> + /* Restore GPRs and log them to pt_regs */
> + mrs x0, sp_el0
> + mrs x1, tpidr_el0
> + kernel_entry 1
> +
> + /* restore the bad SP to pt_regs */
> + mrs x1, tpidrro_el0
> + str x1, [sp, #S_SP]
> +
> + /* Time to die */
> + mov x0, sp
> + b handle_bad_stack
> +#else
> +.macro detect_bad_stack
> +.endm
> +#endif
> +
> /*
> * EL1 mode handlers.
> */
> .align 6
> el1_sync:
> + detect_bad_stack
> kernel_entry 1
> mrs x1, esr_el1 // read the syndrome register
> lsr x24, x1, #ESR_ELx_EC_SHIFT // exception class
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index 0805b44..84b00e3 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -683,6 +683,27 @@ asmlinkage void bad_el0_sync(struct pt_regs *regs, int reason, unsigned int esr)
> force_sig_info(info.si_signo, &info, current);
> }
>
> +#ifdef CONFIG_VMAP_STACK
> +DEFINE_PER_CPU(unsigned long [IRQ_STACK_SIZE/sizeof(long)], bad_stack) __aligned(16);
> +
Surely, we don't need a 16 KB or 64 KB stack here?
> +asmlinkage void handle_bad_stack(struct pt_regs *regs)
> +{
> + unsigned long tsk_stk = (unsigned long)current->stack;
> + unsigned long irq_stk = (unsigned long)per_cpu(irq_stack, smp_processor_id());
> +
> + console_verbose();
> + pr_emerg("Stack out-of-bounds!\n"
> + "\tsp: 0x%016lx\n"
> + "\ttsk stack: [0x%016lx..0x%016lx]\n"
> + "\tirq stack: [0x%016lx..0x%016lx]\n",
> + kernel_stack_pointer(regs),
> + tsk_stk, tsk_stk + THREAD_SIZE,
> + irq_stk, irq_stk + THREAD_SIZE);
> + show_regs(regs);
> + panic("stack out-of-bounds");
> +}
> +#endif
> +
> void __pte_error(const char *file, int line, unsigned long val)
> {
> pr_err("%s:%d: bad pte %016lx.\n", file, line, val);
> --
> 1.9.1
>
[toc] | [prev] | [next] | [standalone]
| From | Mark Rutland <mark.rutland@arm.com> |
|---|---|
| Date | 2017-07-13 13:00 +0200 |
| Subject | Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP |
| Message-ID | <u2K0G-4yC-21@gated-at.bofh.it> |
| In reply to | #1686300 |
On Thu, Jul 13, 2017 at 07:58:50AM +0100, Ard Biesheuvel wrote:
> Hi Mark,
Hi,
> On 12 July 2017 at 23:33, Mark Rutland <mark.rutland@arm.com> wrote:
> > +#ifdef CONFIG_VMAP_STACK
> > +.macro detect_bad_stack
> > + msr sp_el0, x0
> > + get_thread_info x0
> > + ldr x0, [x0, #TSK_TI_CUR_STK]
> > + sub x0, sp, x0
> > + and x0, x0, #~(THREAD_SIZE - 1)
> > + cbnz x0, __bad_stack
> > + mrs x0, sp_el0
>
> The typical prologue looks like
>
> stp x29, x30, [sp, #-xxx]!
> stp x27, x28, [sp, #xxx]
> ...
> mov x29, sp
>
> which means that in most cases where we do run off the stack, sp will
> still be pointing into it when the exception is taken. This means we
> will fault recursively in the handler before having had the chance to
> accurately record the exception context.
True; I had mostly been thinking about kernel_entry, where we do an
explicit subtraction from the SP before any stores.
> Given that the max displacement of a store instruction is 512 bytes,
> and that the frame size we are about to stash exceeds that, should we
> already consider it a stack fault if sp is within 512 bytes (or
> S_FRAME_SIZE) of the base of the stack?
Good point.
I've flip-flopped on this point while writing this reply.
My original line of thinking was that it was best to rely on the
recursive fault to push the SP out-of-bounds. That keeps the overflow
detection simple/fast, and hopefully robust to unexpected exceptions,
(expected?) probes to the guard page, etc.
I also agree that it's annoying to lose the information associated with the
initial fault.
My fear is that we can't catch those cases robustly and efficiently. At
minimum, I believe we'd need to check:
* FAR_EL1 is out-of-bounds for the stack. You have a suitable check for
this.
* FAR_EL1 is valid (looking at the ESR_ELx.{EC,ISS}, etc). I'm not sure
exactly what we need to check here, and I'm not sure what we want to
do about reserved ESR_ELx encodings.
* The base register for the access was the SP (e.g. so this isn't a
probe_kernel_read() or similar).
... so my current feeling is that relying on the recursive fault is the
best bet, even if we lose some information from the initial fault.
Along with that, we should ensure that we get a reliable backtrace, so
that we have the PC from the initial fault, and can acquire the relevant
regs from a dump of the stack and/or the regs at the point of the
recursive fault.
FWIW, currently this series gives you something like:
[ 0.263544] Stack out-of-bounds!
[ 0.263544] sp: 0xffff000009fbfed0
[ 0.263544] tsk stack: [0xffff000009fc0000..0xffff000009fd0000]
[ 0.263544] irq stack: [0xffff80097fe100a0..0xffff80097fe200a0]
[ 0.304862] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.12.0-00006-g0c4fb26-dirty #73
[ 0.312830] Hardware name: ARM Juno development board (r1) (DT)
[ 0.318847] task: ffff800940d8a200 task.stack: ffff000009fc0000
[ 0.324872] PC is at el1_sync+0x20/0xc8
[ 0.328773] LR is at force_overflow+0xc/0x18
[ 0.333113] pc : [<ffff000008082460>] lr : [<ffff00000808c75c>] pstate: 600003c5
[ 0.340636] sp : ffff000009fbfed0
[ 0.344004] x29: ffff000009fc0000 x28: 0000000000000000
[ 0.349409] x27: 0000000000000000 x26: 0000000000000000
[ 0.354812] x25: 0000000000000000 x24: 0000000000000000
[ 0.360214] x23: 0000000000000000 x22: 0000000000000000
[ 0.365617] x21: 0000000000000000 x20: 0000000000000001
[ 0.371020] x19: 0000000000000001 x18: 0000000000000030
[ 0.376422] x17: 0000000000000000 x16: 0000000000000000
[ 0.381826] x15: 0000000000000008 x14: 000000000fb506bc
[ 0.387228] x13: 0000000000000000 x12: 0000000000000000
[ 0.392631] x11: 0000000000000000 x10: 0000000000000141
[ 0.398034] x9 : 0000000000000000 x8 : ffff80097fdf93e8
[ 0.403437] x7 : ffff80097fdf9410 x6 : 0000000000000001
[ 0.408839] x5 : ffff000008ebcb80 x4 : ffff000008eb65d8
[ 0.414242] x3 : 00000000000f4240 x2 : 0000000000000002
[ 0.419644] x1 : ffff800940d8a200 x0 : 0000000000000001
[ 0.425048] Kernel panic - not syncing: stack out-of-bounds
[ 0.430714] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.12.0-00006-g0c4fb26-dirty #73
[ 0.438679] Hardware name: ARM Juno development board (r1) (DT)
[ 0.444697] Call trace:
[ 0.447185] [<ffff000008086f68>] dump_backtrace+0x0/0x230
[ 0.452676] [<ffff00000808725c>] show_stack+0x14/0x20
[ 0.457815] [<ffff00000838760c>] dump_stack+0x9c/0xc0
[ 0.462953] [<ffff00000816d5a0>] panic+0x11c/0x294
[ 0.467825] [<ffff000008087a70>] __pte_error+0x0/0x28
[ 0.472961] [<ffff00000808c75c>] force_overflow+0xc/0x18
[ 0.478364] SMP: stopping secondary CPUs
[ 0.482356] ---[ end Kernel panic - not syncing: stack out-of-bounds
... that __pte_error() is because the last instruction in handle_bad_stack is a
tail-call to panic, and __pte_error happens to be next in the text.
I haven't yet dug into why the stacktrace ends abruptly. I think I need
to update stack walkers to understand the new stack, but I may also have
forgotten to do something with the frame record in the entry path.
[...]
> > +#ifdef CONFIG_VMAP_STACK
> > +DEFINE_PER_CPU(unsigned long [IRQ_STACK_SIZE/sizeof(long)], bad_stack) __aligned(16);
> > +
>
> Surely, we don't need a 16 KB or 64 KB stack here?
For most cases, we do not need such a big stack. We can probably drop
this down to something much smaller (1K, as with your series, sounds
sufficient).
The one case I was worried about was overflows on the emergency stack
itself. I believe that for dumping memory we might need to fix up
exceptions, and if that goes wrong we could go recursive.
I'd planned to update current_stack when jumping to the emergency stack,
and use the same (initial) bounds detection, requiring the emergency
stack to be the same size. In the case of an emergency stack overflow,
we'd go to a (stackless) wfi/wfe loop.
However, I deleted bits of that code while trying to debug an unrelated
issue, and didn't restore it.
I guess it depends on whether we want to try to handle that case.
Thanks,
Mark.
[toc] | [prev] | [next] | [standalone]
| From | Ard Biesheuvel <ard.biesheuvel@linaro.org> |
|---|---|
| Date | 2017-07-13 13:50 +0200 |
| Subject | Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP |
| Message-ID | <u2KN4-54K-11@gated-at.bofh.it> |
| In reply to | #1686446 |
On 13 July 2017 at 11:49, Mark Rutland <mark.rutland@arm.com> wrote:
> On Thu, Jul 13, 2017 at 07:58:50AM +0100, Ard Biesheuvel wrote:
>> Hi Mark,
>
> Hi,
>
>> On 12 July 2017 at 23:33, Mark Rutland <mark.rutland@arm.com> wrote:
>> > +#ifdef CONFIG_VMAP_STACK
>> > +.macro detect_bad_stack
>> > + msr sp_el0, x0
>> > + get_thread_info x0
>> > + ldr x0, [x0, #TSK_TI_CUR_STK]
>> > + sub x0, sp, x0
>> > + and x0, x0, #~(THREAD_SIZE - 1)
>> > + cbnz x0, __bad_stack
>> > + mrs x0, sp_el0
>>
>> The typical prologue looks like
>>
>> stp x29, x30, [sp, #-xxx]!
>> stp x27, x28, [sp, #xxx]
>> ...
>> mov x29, sp
>>
>> which means that in most cases where we do run off the stack, sp will
>> still be pointing into it when the exception is taken. This means we
>> will fault recursively in the handler before having had the chance to
>> accurately record the exception context.
>
> True; I had mostly been thinking about kernel_entry, where we do an
> explicit subtraction from the SP before any stores.
>
>> Given that the max displacement of a store instruction is 512 bytes,
>> and that the frame size we are about to stash exceeds that, should we
>> already consider it a stack fault if sp is within 512 bytes (or
>> S_FRAME_SIZE) of the base of the stack?
>
> Good point.
>
> I've flip-flopped on this point while writing this reply.
>
> My original line of thinking was that it was best to rely on the
> recursive fault to push the SP out-of-bounds. That keeps the overflow
> detection simple/fast, and hopefully robust to unexpected exceptions,
> (expected?) probes to the guard page, etc.
>
> I also agree that it's annoying to lose the information associated with the
> initial fault.
>
> My fear is that we can't catch those cases robustly and efficiently. At
> minimum, I believe we'd need to check:
>
> * FAR_EL1 is out-of-bounds for the stack. You have a suitable check for
> this.
>
> * FAR_EL1 is valid (looking at the ESR_ELx.{EC,ISS}, etc). I'm not sure
> exactly what we need to check here, and I'm not sure what we want to
> do about reserved ESR_ELx encodings.
>
> * The base register for the access was the SP (e.g. so this isn't a
> probe_kernel_read() or similar).
>
> ... so my current feeling is that relying on the recursive fault is the
> best bet, even if we lose some information from the initial fault.
>
There are two related issues at play here that we shouldn't conflate:
- checking whether we have sufficient stack space left to be able to
handle the exception in the first place,
- figuring out whether *this* exception was caused by a faulting
dereference of the stack pointer (which could be with writeback, or
even via some intermediate register: x29 is often used as a pseudo
stack pointer IIRC, although it should never point below sp itself)
Given that the very first stp in kernel_entry will fault if we have
less than S_FRAME_SIZE bytes of stack left, I think we should check
that we have at least that much space available. That way, the context
is preserved, and we could restart the outer exception if we wanted
to, or point our pt_regs pointer to it etc.
When and how we diagnose the condition as a kernel stack overflow is a
separate issue, and can well wait until we're in C code.
> Along with that, we should ensure that we get a reliable backtrace, so
> that we have the PC from the initial fault, and can acquire the relevant
> regs from a dump of the stack and/or the regs at the point of the
> recursive fault.
>
> FWIW, currently this series gives you something like:
>
> [ 0.263544] Stack out-of-bounds!
> [ 0.263544] sp: 0xffff000009fbfed0
> [ 0.263544] tsk stack: [0xffff000009fc0000..0xffff000009fd0000]
> [ 0.263544] irq stack: [0xffff80097fe100a0..0xffff80097fe200a0]
> [ 0.304862] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.12.0-00006-g0c4fb26-dirty #73
> [ 0.312830] Hardware name: ARM Juno development board (r1) (DT)
> [ 0.318847] task: ffff800940d8a200 task.stack: ffff000009fc0000
> [ 0.324872] PC is at el1_sync+0x20/0xc8
> [ 0.328773] LR is at force_overflow+0xc/0x18
> [ 0.333113] pc : [<ffff000008082460>] lr : [<ffff00000808c75c>] pstate: 600003c5
> [ 0.340636] sp : ffff000009fbfed0
> [ 0.344004] x29: ffff000009fc0000 x28: 0000000000000000
> [ 0.349409] x27: 0000000000000000 x26: 0000000000000000
> [ 0.354812] x25: 0000000000000000 x24: 0000000000000000
> [ 0.360214] x23: 0000000000000000 x22: 0000000000000000
> [ 0.365617] x21: 0000000000000000 x20: 0000000000000001
> [ 0.371020] x19: 0000000000000001 x18: 0000000000000030
> [ 0.376422] x17: 0000000000000000 x16: 0000000000000000
> [ 0.381826] x15: 0000000000000008 x14: 000000000fb506bc
> [ 0.387228] x13: 0000000000000000 x12: 0000000000000000
> [ 0.392631] x11: 0000000000000000 x10: 0000000000000141
> [ 0.398034] x9 : 0000000000000000 x8 : ffff80097fdf93e8
> [ 0.403437] x7 : ffff80097fdf9410 x6 : 0000000000000001
> [ 0.408839] x5 : ffff000008ebcb80 x4 : ffff000008eb65d8
> [ 0.414242] x3 : 00000000000f4240 x2 : 0000000000000002
> [ 0.419644] x1 : ffff800940d8a200 x0 : 0000000000000001
> [ 0.425048] Kernel panic - not syncing: stack out-of-bounds
> [ 0.430714] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.12.0-00006-g0c4fb26-dirty #73
> [ 0.438679] Hardware name: ARM Juno development board (r1) (DT)
> [ 0.444697] Call trace:
> [ 0.447185] [<ffff000008086f68>] dump_backtrace+0x0/0x230
> [ 0.452676] [<ffff00000808725c>] show_stack+0x14/0x20
> [ 0.457815] [<ffff00000838760c>] dump_stack+0x9c/0xc0
> [ 0.462953] [<ffff00000816d5a0>] panic+0x11c/0x294
> [ 0.467825] [<ffff000008087a70>] __pte_error+0x0/0x28
> [ 0.472961] [<ffff00000808c75c>] force_overflow+0xc/0x18
> [ 0.478364] SMP: stopping secondary CPUs
> [ 0.482356] ---[ end Kernel panic - not syncing: stack out-of-bounds
>
> ... that __pte_error() is because the last instruction in handle_bad_stack is a
> tail-call to panic, and __pte_error happens to be next in the text.
>
> I haven't yet dug into why the stacktrace ends abruptly. I think I need
> to update stack walkers to understand the new stack, but I may also have
> forgotten to do something with the frame record in the entry path.
>
> [...]
>
>> > +#ifdef CONFIG_VMAP_STACK
>> > +DEFINE_PER_CPU(unsigned long [IRQ_STACK_SIZE/sizeof(long)], bad_stack) __aligned(16);
>> > +
>>
>> Surely, we don't need a 16 KB or 64 KB stack here?
>
> For most cases, we do not need such a big stack. We can probably drop
> this down to something much smaller (1K, as with your series, sounds
> sufficient).
>
> The one case I was worried about was overflows on the emergency stack
> itself. I believe that for dumping memory we might need to fix up
> exceptions, and if that goes wrong we could go recursive.
>
> I'd planned to update current_stack when jumping to the emergency stack,
> and use the same (initial) bounds detection, requiring the emergency
> stack to be the same size. In the case of an emergency stack overflow,
> we'd go to a (stackless) wfi/wfe loop.
>
> However, I deleted bits of that code while trying to debug an unrelated
> issue, and didn't restore it.
>
> I guess it depends on whether we want to try to handle that case.
>
> Thanks,
> Mark.
[toc] | [prev] | [next] | [standalone]
| From | Mark Rutland <mark.rutland@arm.com> |
|---|---|
| Date | 2017-07-13 18:20 +0200 |
| Subject | Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP |
| Message-ID | <u2P0m-7Rn-19@gated-at.bofh.it> |
| In reply to | #1686471 |
On Thu, Jul 13, 2017 at 12:49:48PM +0100, Ard Biesheuvel wrote:
> On 13 July 2017 at 11:49, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Thu, Jul 13, 2017 at 07:58:50AM +0100, Ard Biesheuvel wrote:
> >> On 12 July 2017 at 23:33, Mark Rutland <mark.rutland@arm.com> wrote:
> >> The typical prologue looks like
> >>
> >> stp x29, x30, [sp, #-xxx]!
> >> stp x27, x28, [sp, #xxx]
> >> ...
> >> mov x29, sp
> >>
> >> which means that in most cases where we do run off the stack, sp will
> >> still be pointing into it when the exception is taken. This means we
> >> will fault recursively in the handler before having had the chance to
> >> accurately record the exception context.
> >> Given that the max displacement of a store instruction is 512 bytes,
> >> and that the frame size we are about to stash exceeds that, should we
> >> already consider it a stack fault if sp is within 512 bytes (or
> >> S_FRAME_SIZE) of the base of the stack?
> > My original line of thinking was that it was best to rely on the
> > recursive fault to push the SP out-of-bounds. That keeps the overflow
> > detection simple/fast, and hopefully robust to unexpected exceptions,
> > (expected?) probes to the guard page, etc.
> >
> > I also agree that it's annoying to lose the information associated with the
> > initial fault.
> >
> > My fear is that we can't catch those cases robustly and efficiently. At
> > minimum, I believe we'd need to check:
> >
> > * FAR_EL1 is out-of-bounds for the stack. You have a suitable check for
> > this.
> >
> > * FAR_EL1 is valid (looking at the ESR_ELx.{EC,ISS}, etc). I'm not sure
> > exactly what we need to check here, and I'm not sure what we want to
> > do about reserved ESR_ELx encodings.
> >
> > * The base register for the access was the SP (e.g. so this isn't a
> > probe_kernel_read() or similar).
> >
> > ... so my current feeling is that relying on the recursive fault is the
> > best bet, even if we lose some information from the initial fault.
>
> There are two related issues at play here that we shouldn't conflate:
> - checking whether we have sufficient stack space left to be able to
> handle the exception in the first place,
> - figuring out whether *this* exception was caused by a faulting
> dereference of the stack pointer (which could be with writeback, or
> even via some intermediate register: x29 is often used as a pseudo
> stack pointer IIRC, although it should never point below sp itself)
Sure; I agree these are separate properties (my robustness and
efficiency concerns fall with the latter).
> Given that the very first stp in kernel_entry will fault if we have
> less than S_FRAME_SIZE bytes of stack left, I think we should check
> that we have at least that much space available.
I was going to reply saying that I didn't agree, but in writing up
examples, I mostly convinced myself that this is the right thing to do.
So I mostly agree!
This would mean we treat the first impossible-to-handle exception as
that fatal case, which is similar to x86's double-fault, triggered when
the HW can't stack the regs. All other cases are just arbitrary faults.
However, to provide that consistently, we'll need to perform this check
at every exception boundary, or some of those cases will result in a
recursive fault first.
So I think there are three choices:
1) In el1_sync, only check SP bounds, and live with the recursive
faults.
2) in el1_sync, check there's room for the regs, and live with the
recursive faults for overflow on other exceptions.
3) In all EL1 entry paths, check there's room for the regs.
> That way, the context is preserved, and we could restart the outer
> exception if we wanted to, or point our pt_regs pointer to it etc.
>
> When and how we diagnose the condition as a kernel stack overflow is a
> separate issue, and can well wait until we're in C code.
I believe that determining whether the exception was caused by a stack
overflow is not something we can do robustly or efficiently.
You mentioned the x29 pseudo-sp case, and there are other cases where
the SP value is proxied:
mov x0, sp
ldr x0, [x0, x1]
Or unrelated accesses that hit the guard page:
adrp x0, some_vmalloc_object
add x0, x0, #:lo12:some_vmalloc_object
mov x1, #bogus_offset
ldr x0, [x0, x1]
As above, I think it's helpful to think of this as something closer to a
double-fault handler (i.e. aiming to catch when we can't take the
exception safely), rather than something that's trying to catch logical
stack overflows.
Thanks,
Mark.
[toc] | [prev] | [next] | [standalone]
| From | Mark Rutland <mark.rutland@arm.com> |
|---|---|
| Date | 2017-07-13 20:00 +0200 |
| Subject | Re: [kernel-hardening] Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP |
| Message-ID | <u2Qz7-fX-9@gated-at.bofh.it> |
| In reply to | #1686758 |
On Thu, Jul 13, 2017 at 05:10:50PM +0100, Mark Rutland wrote: > On Thu, Jul 13, 2017 at 12:49:48PM +0100, Ard Biesheuvel wrote: > > On 13 July 2017 at 11:49, Mark Rutland <mark.rutland@arm.com> wrote: > > > On Thu, Jul 13, 2017 at 07:58:50AM +0100, Ard Biesheuvel wrote: > > >> On 12 July 2017 at 23:33, Mark Rutland <mark.rutland@arm.com> wrote: > > Given that the very first stp in kernel_entry will fault if we have > > less than S_FRAME_SIZE bytes of stack left, I think we should check > > that we have at least that much space available. > > I was going to reply saying that I didn't agree, but in writing up > examples, I mostly convinced myself that this is the right thing to do. > So I mostly agree! > > This would mean we treat the first impossible-to-handle exception as > that fatal case, which is similar to x86's double-fault, triggered when > the HW can't stack the regs. All other cases are just arbitrary faults. > > However, to provide that consistently, we'll need to perform this check > at every exception boundary, or some of those cases will result in a > recursive fault first. > > So I think there are three choices: > > 1) In el1_sync, only check SP bounds, and live with the recursive > faults. > > 2) in el1_sync, check there's room for the regs, and live with the > recursive faults for overflow on other exceptions. > > 3) In all EL1 entry paths, check there's room for the regs. FWIW, for the moment I've applied (2), as you suggested, to my arm64/vmap-stack branch, adding an additional: sub x0, x0, #S_FRAME_SIZE ... to the entry path. I think it's worth trying (3) so that we consistently report these cases, benchmarks permitting. It's probably worth putting the fast-path check directly into the vectors, where we currently only use 1/32 of the instruction slots available to us. > As above, I think it's helpful to think of this as something closer to a > double-fault handler (i.e. aiming to catch when we can't take the > exception safely), rather than something that's trying to catch logical > stack overflows. Does this make sense to you? I've tried to reword the log output, as below, to give this impression. [ 49.288232] Insufficient stack space to handle exception! [ 49.288245] CPU: 5 PID: 2208 Comm: bash Not tainted 4.12.0-00005-ga781af2 #81 [ 49.300680] Hardware name: ARM Juno development board (r1) (DT) [ 49.306549] task: ffff800974955100 task.stack: ffff00000d6f0000 [ 49.312426] PC is at recursive_loop+0x10/0x50 [ 49.316747] LR is at recursive_loop+0x34/0x50 [ 49.321066] pc : [<ffff000008588aa0>] lr : [<ffff000008588ac4>] pstate: 40000145 [ 49.328398] sp : ffff00000d6eff30 [ 49.331682] x29: ffff00000d6f0350 x28: ffff800974955100 [ 49.336953] x27: ffff000008942000 x26: ffff000008f0d758 [ 49.342223] x25: ffff00000d6f3eb8 x24: ffff00000d6f3eb8 [ 49.347493] x23: ffff000008f0d490 x22: 0000000000000009 [ 49.352764] x21: ffff800974a57000 x20: ffff000008f0d4e0 [ 49.358034] x19: 0000000000000013 x18: 0000ffffe7e2e4f0 [ 49.363304] x17: 0000ffff9c1256a4 x16: ffff0000081f8b88 [ 49.368574] x15: 00002a81b8000000 x14: 00000000fffffff0 [ 49.373845] x13: ffff000008f6278a x12: ffff000008e62818 [ 49.379115] x11: 0000000000000000 x10: 000000000000019e [ 49.384385] x9 : 0000000000000004 x8 : ffff00000d6f0770 [ 49.389656] x7 : 1313131313131313 x6 : 000000000000019e [ 49.394925] x5 : 0000000000000000 x4 : 0000000000000000 [ 49.400205] x3 : 0000000000000000 x2 : 0000000000000400 [ 49.405484] x1 : 0000000000000013 x0 : 0000000000000012 [ 49.410764] Task stack: [0xffff00000d6f0000..0xffff00000d6f4000] [ 49.416728] IRQ stack: [0xffff80097ffb90a0..0xffff80097ffbd0a0] [ 49.422692] ESR: 0x96000047 -- DABT (current EL) [ 49.427277] FAR: 0xffff00000d6eff30 [ 49.430742] Kernel panic - not syncing: kernel stack overflow [ 49.436451] CPU: 5 PID: 2208 Comm: bash Not tainted 4.12.0-00005-ga781af2 #81 [ 49.443534] Hardware name: ARM Juno development board (r1) (DT) [ 49.449412] Call trace: [ 49.451852] [<ffff0000080885f0>] dump_backtrace+0x0/0x230 [ 49.457218] [<ffff0000080888e4>] show_stack+0x14/0x20 [ 49.462240] [<ffff00000839be0c>] dump_stack+0x9c/0xc0 [ 49.467261] [<ffff000008175218>] panic+0x11c/0x294 [ 49.472024] [<ffff000008089184>] handle_bad_stack+0xe4/0xe8 [ 49.477561] [<ffff000008588ac4>] recursive_loop+0x34/0x50 [ 49.482926] SMP: stopping secondary CPUs [ 49.487145] Kernel Offset: disabled [ 49.490609] Memory Limit: none [ 49.493649] ---[ end Kernel panic - not syncing: kernel stack overflow ... I still need to attack the backtracing to walk across stacks. Thanks, Mark.
[toc] | [prev] | [next] | [standalone]
| From | Ard Biesheuvel <ard.biesheuvel@linaro.org> |
|---|---|
| Date | 2017-07-13 20:30 +0200 |
| Subject | Re: [kernel-hardening] Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP |
| Message-ID | <u2R29-FW-21@gated-at.bofh.it> |
| In reply to | #1686822 |
On 13 July 2017 at 18:55, Mark Rutland <mark.rutland@arm.com> wrote: > On Thu, Jul 13, 2017 at 05:10:50PM +0100, Mark Rutland wrote: >> On Thu, Jul 13, 2017 at 12:49:48PM +0100, Ard Biesheuvel wrote: >> > On 13 July 2017 at 11:49, Mark Rutland <mark.rutland@arm.com> wrote: >> > > On Thu, Jul 13, 2017 at 07:58:50AM +0100, Ard Biesheuvel wrote: >> > >> On 12 July 2017 at 23:33, Mark Rutland <mark.rutland@arm.com> wrote: > >> > Given that the very first stp in kernel_entry will fault if we have >> > less than S_FRAME_SIZE bytes of stack left, I think we should check >> > that we have at least that much space available. >> >> I was going to reply saying that I didn't agree, but in writing up >> examples, I mostly convinced myself that this is the right thing to do. >> So I mostly agree! >> >> This would mean we treat the first impossible-to-handle exception as >> that fatal case, which is similar to x86's double-fault, triggered when >> the HW can't stack the regs. All other cases are just arbitrary faults. >> >> However, to provide that consistently, we'll need to perform this check >> at every exception boundary, or some of those cases will result in a >> recursive fault first. >> >> So I think there are three choices: >> >> 1) In el1_sync, only check SP bounds, and live with the recursive >> faults. >> >> 2) in el1_sync, check there's room for the regs, and live with the >> recursive faults for overflow on other exceptions. >> >> 3) In all EL1 entry paths, check there's room for the regs. > > FWIW, for the moment I've applied (2), as you suggested, to my > arm64/vmap-stack branch, adding an additional: > > sub x0, x0, #S_FRAME_SIZE > > ... to the entry path. > > I think it's worth trying (3) so that we consistently report these > cases, benchmarks permitting. > OK, so here's a crazy idea: what if we a) carve out a dedicated range in the VMALLOC area for stacks b) for each stack, allocate a naturally aligned window of 2x the stack size, and map the stack inside it, leaving the remaining space unmapped That way, we can compare SP (minus S_FRAME_SIZE) against a mask that is a build time constant, to decide whether its value points into a stack or not. Of course, it may be pointing into the wrong stack, but that should not prevent us from taking the exception, and we can deal with that later. It would give us a very cheap way to perform this test on the hot paths. >> I believe that determining whether the exception was caused by a stack >> overflow is not something we can do robustly or efficiently. >> Actually, if the stack pointer is within S_FRAME_SIZE of the base, and the faulting address points into the guard page, that is a pretty strong indicator that the stack overflowed. That shouldn't be too costly? > It's probably worth putting the fast-path check directly into the > vectors, where we currently only use 1/32 of the instruction slots > available to us. > >> As above, I think it's helpful to think of this as something closer to a >> double-fault handler (i.e. aiming to catch when we can't take the >> exception safely), rather than something that's trying to catch logical >> stack overflows. > > Does this make sense to you? > > I've tried to reword the log output, as below, to give this impression. > > [ 49.288232] Insufficient stack space to handle exception! This could be a separate warning, if we find out that the actual exception was caused by something else. > [ 49.288245] CPU: 5 PID: 2208 Comm: bash Not tainted 4.12.0-00005-ga781af2 #81 > [ 49.300680] Hardware name: ARM Juno development board (r1) (DT) > [ 49.306549] task: ffff800974955100 task.stack: ffff00000d6f0000 > [ 49.312426] PC is at recursive_loop+0x10/0x50 > [ 49.316747] LR is at recursive_loop+0x34/0x50 > [ 49.321066] pc : [<ffff000008588aa0>] lr : [<ffff000008588ac4>] pstate: 40000145 > [ 49.328398] sp : ffff00000d6eff30 > [ 49.331682] x29: ffff00000d6f0350 x28: ffff800974955100 > [ 49.336953] x27: ffff000008942000 x26: ffff000008f0d758 > [ 49.342223] x25: ffff00000d6f3eb8 x24: ffff00000d6f3eb8 > [ 49.347493] x23: ffff000008f0d490 x22: 0000000000000009 > [ 49.352764] x21: ffff800974a57000 x20: ffff000008f0d4e0 > [ 49.358034] x19: 0000000000000013 x18: 0000ffffe7e2e4f0 > [ 49.363304] x17: 0000ffff9c1256a4 x16: ffff0000081f8b88 > [ 49.368574] x15: 00002a81b8000000 x14: 00000000fffffff0 > [ 49.373845] x13: ffff000008f6278a x12: ffff000008e62818 > [ 49.379115] x11: 0000000000000000 x10: 000000000000019e > [ 49.384385] x9 : 0000000000000004 x8 : ffff00000d6f0770 > [ 49.389656] x7 : 1313131313131313 x6 : 000000000000019e > [ 49.394925] x5 : 0000000000000000 x4 : 0000000000000000 > [ 49.400205] x3 : 0000000000000000 x2 : 0000000000000400 > [ 49.405484] x1 : 0000000000000013 x0 : 0000000000000012 > [ 49.410764] Task stack: [0xffff00000d6f0000..0xffff00000d6f4000] > [ 49.416728] IRQ stack: [0xffff80097ffb90a0..0xffff80097ffbd0a0] > [ 49.422692] ESR: 0x96000047 -- DABT (current EL) > [ 49.427277] FAR: 0xffff00000d6eff30 > [ 49.430742] Kernel panic - not syncing: kernel stack overflow > [ 49.436451] CPU: 5 PID: 2208 Comm: bash Not tainted 4.12.0-00005-ga781af2 #81 > [ 49.443534] Hardware name: ARM Juno development board (r1) (DT) > [ 49.449412] Call trace: > [ 49.451852] [<ffff0000080885f0>] dump_backtrace+0x0/0x230 > [ 49.457218] [<ffff0000080888e4>] show_stack+0x14/0x20 > [ 49.462240] [<ffff00000839be0c>] dump_stack+0x9c/0xc0 > [ 49.467261] [<ffff000008175218>] panic+0x11c/0x294 > [ 49.472024] [<ffff000008089184>] handle_bad_stack+0xe4/0xe8 > [ 49.477561] [<ffff000008588ac4>] recursive_loop+0x34/0x50 > [ 49.482926] SMP: stopping secondary CPUs > [ 49.487145] Kernel Offset: disabled > [ 49.490609] Memory Limit: none > [ 49.493649] ---[ end Kernel panic - not syncing: kernel stack overflow > Yes, this looks nice. > ... I still need to attack the backtracing to walk across stacks. > Yup
[toc] | [prev] | [next] | [standalone]
| From | Mark Rutland <mark.rutland@arm.com> |
|---|---|
| Date | 2017-07-14 12:40 +0200 |
| Subject | Re: [kernel-hardening] Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP |
| Message-ID | <u36aS-28v-15@gated-at.bofh.it> |
| In reply to | #1686836 |
On Thu, Jul 13, 2017 at 07:28:48PM +0100, Ard Biesheuvel wrote: > On 13 July 2017 at 18:55, Mark Rutland <mark.rutland@arm.com> wrote: > > On Thu, Jul 13, 2017 at 05:10:50PM +0100, Mark Rutland wrote: > >> On Thu, Jul 13, 2017 at 12:49:48PM +0100, Ard Biesheuvel wrote: > >> > On 13 July 2017 at 11:49, Mark Rutland <mark.rutland@arm.com> wrote: > >> > > On Thu, Jul 13, 2017 at 07:58:50AM +0100, Ard Biesheuvel wrote: > >> > >> On 12 July 2017 at 23:33, Mark Rutland <mark.rutland@arm.com> wrote: > > > >> > Given that the very first stp in kernel_entry will fault if we have > >> > less than S_FRAME_SIZE bytes of stack left, I think we should check > >> > that we have at least that much space available. > >> > >> I was going to reply saying that I didn't agree, but in writing up > >> examples, I mostly convinced myself that this is the right thing to do. > >> So I mostly agree! > >> > >> This would mean we treat the first impossible-to-handle exception as > >> that fatal case, which is similar to x86's double-fault, triggered when > >> the HW can't stack the regs. All other cases are just arbitrary faults. > >> > >> However, to provide that consistently, we'll need to perform this check > >> at every exception boundary, or some of those cases will result in a > >> recursive fault first. > >> > >> So I think there are three choices: > >> > >> 1) In el1_sync, only check SP bounds, and live with the recursive > >> faults. > >> > >> 2) in el1_sync, check there's room for the regs, and live with the > >> recursive faults for overflow on other exceptions. > >> > >> 3) In all EL1 entry paths, check there's room for the regs. > > > > FWIW, for the moment I've applied (2), as you suggested, to my > > arm64/vmap-stack branch, adding an additional: > > > > sub x0, x0, #S_FRAME_SIZE > > > > ... to the entry path. > > > > I think it's worth trying (3) so that we consistently report these > > cases, benchmarks permitting. > > > > OK, so here's a crazy idea: what if we > a) carve out a dedicated range in the VMALLOC area for stacks > b) for each stack, allocate a naturally aligned window of 2x the stack > size, and map the stack inside it, leaving the remaining space > unmapped This is not such a crazy idea. :) In fact, it was one I toyed with before getting lost on a register juggling tangent (see below). > That way, we can compare SP (minus S_FRAME_SIZE) against a mask that > is a build time constant, to decide whether its value points into a > stack or not. Of course, it may be pointing into the wrong stack, but > that should not prevent us from taking the exception, and we can deal > with that later. It would give us a very cheap way to perform this > test on the hot paths. The logical ops (TST) and conditional branches (TB(N)Z, CB(N)Z) operate on XZR rather than SP, so to do this we need to get the SP value into a GPR. Previously, I assumed this meant we needed to corrupt a GPR (and hence stash that GPR in a sysreg), so I started writing code to free sysregs. However, I now realise I was being thick, since we can stash the GPR in the SP: sub sp, sp, x0 // sp = orig_sp - x0 add x0, sp, x0 // x0 = x0 - (orig_sp - x0) == orig_sp sub x0, x0, #S_FRAME_SIZE tb(nz) x0, #THREAD_SHIFT, overflow add x0, x0, #S_FRAME_SIZE sub x0, sp, x0 add sp, sp, x0 ... so yes, this could work! This means that we have to align the initial task, so the kernel Image will grow by THREAD_SIZE. Likewise for IRQ stacks, unless we can rework things such that we can dynamically allocate all of those. > >> I believe that determining whether the exception was caused by a stack > >> overflow is not something we can do robustly or efficiently. > > Actually, if the stack pointer is within S_FRAME_SIZE of the base, and > the faulting address points into the guard page, that is a pretty > strong indicator that the stack overflowed. That shouldn't be too > costly? Sure, but that's still a a heuristic. For example, that also catches an unrelated vmalloc address gone wrong, while SP was close to the end of the stack. The important thing is whether we can *safely enter the exception* (i.e. stack the regs), or whether this'll push the SP (further) out-of-bounds. I think we agree that we can reliably and efficiently check this. The general case of nominal "stack overflows" (e.g. large preidx decrements, proxied SP values, unrelated guard-page faults) is a semantic minefield. I don't think we should add code to try to distinguish these. For that general case, if we can enter the exception then we can try to handle the exception in the usual way, and either: * The fault code determines the access was bad. We at least kill the thread. * We overflow the stack while trying to handle the exception, triggering a new fault to triage. To make it possible to distinguish and debug these, we need to fix the backtracing code, but that's it. Thanks, Mark.
[toc] | [prev] | [next] | [standalone]
| From | Ard Biesheuvel <ard.biesheuvel@linaro.org> |
|---|---|
| Date | 2017-07-14 12:50 +0200 |
| Subject | Re: [kernel-hardening] Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP |
| Message-ID | <u36kx-2cw-3@gated-at.bofh.it> |
| In reply to | #1687242 |
On 14 July 2017 at 11:32, Mark Rutland <mark.rutland@arm.com> wrote: > On Thu, Jul 13, 2017 at 07:28:48PM +0100, Ard Biesheuvel wrote: >> On 13 July 2017 at 18:55, Mark Rutland <mark.rutland@arm.com> wrote: >> > On Thu, Jul 13, 2017 at 05:10:50PM +0100, Mark Rutland wrote: >> >> On Thu, Jul 13, 2017 at 12:49:48PM +0100, Ard Biesheuvel wrote: >> >> > On 13 July 2017 at 11:49, Mark Rutland <mark.rutland@arm.com> wrote: >> >> > > On Thu, Jul 13, 2017 at 07:58:50AM +0100, Ard Biesheuvel wrote: >> >> > >> On 12 July 2017 at 23:33, Mark Rutland <mark.rutland@arm.com> wrote: >> > >> >> > Given that the very first stp in kernel_entry will fault if we have >> >> > less than S_FRAME_SIZE bytes of stack left, I think we should check >> >> > that we have at least that much space available. >> >> >> >> I was going to reply saying that I didn't agree, but in writing up >> >> examples, I mostly convinced myself that this is the right thing to do. >> >> So I mostly agree! >> >> >> >> This would mean we treat the first impossible-to-handle exception as >> >> that fatal case, which is similar to x86's double-fault, triggered when >> >> the HW can't stack the regs. All other cases are just arbitrary faults. >> >> >> >> However, to provide that consistently, we'll need to perform this check >> >> at every exception boundary, or some of those cases will result in a >> >> recursive fault first. >> >> >> >> So I think there are three choices: >> >> >> >> 1) In el1_sync, only check SP bounds, and live with the recursive >> >> faults. >> >> >> >> 2) in el1_sync, check there's room for the regs, and live with the >> >> recursive faults for overflow on other exceptions. >> >> >> >> 3) In all EL1 entry paths, check there's room for the regs. >> > >> > FWIW, for the moment I've applied (2), as you suggested, to my >> > arm64/vmap-stack branch, adding an additional: >> > >> > sub x0, x0, #S_FRAME_SIZE >> > >> > ... to the entry path. >> > >> > I think it's worth trying (3) so that we consistently report these >> > cases, benchmarks permitting. >> > >> >> OK, so here's a crazy idea: what if we >> a) carve out a dedicated range in the VMALLOC area for stacks >> b) for each stack, allocate a naturally aligned window of 2x the stack >> size, and map the stack inside it, leaving the remaining space >> unmapped > > This is not such a crazy idea. :) > > In fact, it was one I toyed with before getting lost on a register > juggling tangent (see below). > >> That way, we can compare SP (minus S_FRAME_SIZE) against a mask that >> is a build time constant, to decide whether its value points into a >> stack or not. Of course, it may be pointing into the wrong stack, but >> that should not prevent us from taking the exception, and we can deal >> with that later. It would give us a very cheap way to perform this >> test on the hot paths. > > The logical ops (TST) and conditional branches (TB(N)Z, CB(N)Z) operate > on XZR rather than SP, so to do this we need to get the SP value into a > GPR. > > Previously, I assumed this meant we needed to corrupt a GPR (and hence > stash that GPR in a sysreg), so I started writing code to free sysregs. > > However, I now realise I was being thick, since we can stash the GPR > in the SP: > > sub sp, sp, x0 // sp = orig_sp - x0 > add x0, sp, x0 // x0 = x0 - (orig_sp - x0) == orig_sp > sub x0, x0, #S_FRAME_SIZE > tb(nz) x0, #THREAD_SHIFT, overflow > add x0, x0, #S_FRAME_SIZE > sub x0, sp, x0 > add sp, sp, x0 > > ... so yes, this could work! > Nice! > This means that we have to align the initial task, so the kernel Image > will grow by THREAD_SIZE. Likewise for IRQ stacks, unless we can rework > things such that we can dynamically allocate all of those. > We can't currently do that for 64k pages, since the segment alignment is only 64k. But we should be able to patch that up I think >> >> I believe that determining whether the exception was caused by a stack >> >> overflow is not something we can do robustly or efficiently. >> >> Actually, if the stack pointer is within S_FRAME_SIZE of the base, and >> the faulting address points into the guard page, that is a pretty >> strong indicator that the stack overflowed. That shouldn't be too >> costly? > > Sure, but that's still a a heuristic. For example, that also catches an > unrelated vmalloc address gone wrong, while SP was close to the end of > the stack. > Yes, but the likelihood that an unrelated stray vmalloc access is within 16 KB of a stack pointer that is close ot its limit is extremely low, so we should be able to live with the risk of misidentifying it. > The important thing is whether we can *safely enter the exception* (i.e. > stack the regs), or whether this'll push the SP (further) out-of-bounds. > I think we agree that we can reliably and efficiently check this. > Yes. > The general case of nominal "stack overflows" (e.g. large preidx > decrements, proxied SP values, unrelated guard-page faults) is a > semantic minefield. I don't think we should add code to try to > distinguish these. > > For that general case, if we can enter the exception then we can try to > handle the exception in the usual way, and either: > > * The fault code determines the access was bad. We at least kill the > thread. > > * We overflow the stack while trying to handle the exception, triggering > a new fault to triage. > > To make it possible to distinguish and debug these, we need to fix the > backtracing code, but that's it. > > Thanks, > Mark.
[toc] | [prev] | [next] | [standalone]
| From | Ard Biesheuvel <ard.biesheuvel@linaro.org> |
|---|---|
| Date | 2017-07-14 14:30 +0200 |
| Subject | Re: [kernel-hardening] Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP |
| Message-ID | <u37Tk-3mW-13@gated-at.bofh.it> |
| In reply to | #1687243 |
On 14 July 2017 at 11:48, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 14 July 2017 at 11:32, Mark Rutland <mark.rutland@arm.com> wrote: >> On Thu, Jul 13, 2017 at 07:28:48PM +0100, Ard Biesheuvel wrote: >>> On 13 July 2017 at 18:55, Mark Rutland <mark.rutland@arm.com> wrote: >>> > On Thu, Jul 13, 2017 at 05:10:50PM +0100, Mark Rutland wrote: >>> >> On Thu, Jul 13, 2017 at 12:49:48PM +0100, Ard Biesheuvel wrote: >>> >> > On 13 July 2017 at 11:49, Mark Rutland <mark.rutland@arm.com> wrote: >>> >> > > On Thu, Jul 13, 2017 at 07:58:50AM +0100, Ard Biesheuvel wrote: >>> >> > >> On 12 July 2017 at 23:33, Mark Rutland <mark.rutland@arm.com> wrote: >>> > >>> >> > Given that the very first stp in kernel_entry will fault if we have >>> >> > less than S_FRAME_SIZE bytes of stack left, I think we should check >>> >> > that we have at least that much space available. >>> >> >>> >> I was going to reply saying that I didn't agree, but in writing up >>> >> examples, I mostly convinced myself that this is the right thing to do. >>> >> So I mostly agree! >>> >> >>> >> This would mean we treat the first impossible-to-handle exception as >>> >> that fatal case, which is similar to x86's double-fault, triggered when >>> >> the HW can't stack the regs. All other cases are just arbitrary faults. >>> >> >>> >> However, to provide that consistently, we'll need to perform this check >>> >> at every exception boundary, or some of those cases will result in a >>> >> recursive fault first. >>> >> >>> >> So I think there are three choices: >>> >> >>> >> 1) In el1_sync, only check SP bounds, and live with the recursive >>> >> faults. >>> >> >>> >> 2) in el1_sync, check there's room for the regs, and live with the >>> >> recursive faults for overflow on other exceptions. >>> >> >>> >> 3) In all EL1 entry paths, check there's room for the regs. >>> > >>> > FWIW, for the moment I've applied (2), as you suggested, to my >>> > arm64/vmap-stack branch, adding an additional: >>> > >>> > sub x0, x0, #S_FRAME_SIZE >>> > >>> > ... to the entry path. >>> > >>> > I think it's worth trying (3) so that we consistently report these >>> > cases, benchmarks permitting. >>> > >>> >>> OK, so here's a crazy idea: what if we >>> a) carve out a dedicated range in the VMALLOC area for stacks >>> b) for each stack, allocate a naturally aligned window of 2x the stack >>> size, and map the stack inside it, leaving the remaining space >>> unmapped >> >> This is not such a crazy idea. :) >> >> In fact, it was one I toyed with before getting lost on a register >> juggling tangent (see below). >> >>> That way, we can compare SP (minus S_FRAME_SIZE) against a mask that >>> is a build time constant, to decide whether its value points into a >>> stack or not. Of course, it may be pointing into the wrong stack, but >>> that should not prevent us from taking the exception, and we can deal >>> with that later. It would give us a very cheap way to perform this >>> test on the hot paths. >> >> The logical ops (TST) and conditional branches (TB(N)Z, CB(N)Z) operate >> on XZR rather than SP, so to do this we need to get the SP value into a >> GPR. >> >> Previously, I assumed this meant we needed to corrupt a GPR (and hence >> stash that GPR in a sysreg), so I started writing code to free sysregs. >> >> However, I now realise I was being thick, since we can stash the GPR >> in the SP: >> >> sub sp, sp, x0 // sp = orig_sp - x0 >> add x0, sp, x0 // x0 = x0 - (orig_sp - x0) == orig_sp >> sub x0, x0, #S_FRAME_SIZE >> tb(nz) x0, #THREAD_SHIFT, overflow >> add x0, x0, #S_FRAME_SIZE >> sub x0, sp, x0 You need a neg x0, x0 here I think >> add sp, sp, x0 >> >> ... so yes, this could work! >> > > Nice! > ... only, this requires a dedicated stack region, and so we'd need to check whether sp is inside that window as well. The easieast way would be to use a window whose start address is base2 aligned, but that means the beginning of the kernel VA range (where KASAN currently lives, and cannot be moved afaik), or a window at the top of the linear region. Neither look very appealing So that means arbitrary low and high limits to compare against in this entry path. That means more GPRs I'm afraid. >> This means that we have to align the initial task, so the kernel Image >> will grow by THREAD_SIZE. Likewise for IRQ stacks, unless we can rework >> things such that we can dynamically allocate all of those. >> > > We can't currently do that for 64k pages, since the segment alignment > is only 64k. But we should be able to patch that up I think > >>> >> I believe that determining whether the exception was caused by a stack >>> >> overflow is not something we can do robustly or efficiently. >>> >>> Actually, if the stack pointer is within S_FRAME_SIZE of the base, and >>> the faulting address points into the guard page, that is a pretty >>> strong indicator that the stack overflowed. That shouldn't be too >>> costly? >> >> Sure, but that's still a a heuristic. For example, that also catches an >> unrelated vmalloc address gone wrong, while SP was close to the end of >> the stack. >> > > Yes, but the likelihood that an unrelated stray vmalloc access is > within 16 KB of a stack pointer that is close ot its limit is > extremely low, so we should be able to live with the risk of > misidentifying it. > >> The important thing is whether we can *safely enter the exception* (i.e. >> stack the regs), or whether this'll push the SP (further) out-of-bounds. >> I think we agree that we can reliably and efficiently check this. >> > > Yes. > >> The general case of nominal "stack overflows" (e.g. large preidx >> decrements, proxied SP values, unrelated guard-page faults) is a >> semantic minefield. I don't think we should add code to try to >> distinguish these. >> >> For that general case, if we can enter the exception then we can try to >> handle the exception in the usual way, and either: >> >> * The fault code determines the access was bad. We at least kill the >> thread. >> >> * We overflow the stack while trying to handle the exception, triggering >> a new fault to triage. >> >> To make it possible to distinguish and debug these, we need to fix the >> backtracing code, but that's it. >> >> Thanks, >> Mark.
[toc] | [prev] | [next] | [standalone]
| From | Mark Rutland <mark.rutland@arm.com> |
|---|---|
| Date | 2017-07-14 16:10 +0200 |
| Subject | Re: [kernel-hardening] Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP |
| Message-ID | <u39s7-4wc-23@gated-at.bofh.it> |
| In reply to | #1687332 |
On Fri, Jul 14, 2017 at 01:27:14PM +0100, Ard Biesheuvel wrote: > On 14 July 2017 at 11:48, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > On 14 July 2017 at 11:32, Mark Rutland <mark.rutland@arm.com> wrote: > >> On Thu, Jul 13, 2017 at 07:28:48PM +0100, Ard Biesheuvel wrote: > >>> OK, so here's a crazy idea: what if we > >>> a) carve out a dedicated range in the VMALLOC area for stacks > >>> b) for each stack, allocate a naturally aligned window of 2x the stack > >>> size, and map the stack inside it, leaving the remaining space > >>> unmapped > >> The logical ops (TST) and conditional branches (TB(N)Z, CB(N)Z) operate > >> on XZR rather than SP, so to do this we need to get the SP value into a > >> GPR. > >> > >> Previously, I assumed this meant we needed to corrupt a GPR (and hence > >> stash that GPR in a sysreg), so I started writing code to free sysregs. > >> > >> However, I now realise I was being thick, since we can stash the GPR > >> in the SP: > >> > >> sub sp, sp, x0 // sp = orig_sp - x0 > >> add x0, sp, x0 // x0 = x0 - (orig_sp - x0) == orig_sp That comment is off, and should say x0 = x0 + (orig_sp - x0) == orig_sp > >> sub x0, x0, #S_FRAME_SIZE > >> tb(nz) x0, #THREAD_SHIFT, overflow > >> add x0, x0, #S_FRAME_SIZE > >> sub x0, sp, x0 > > You need a neg x0, x0 here I think Oh, whoops. I'd mis-simplified things. We can avoid that by storing orig_sp + orig_x0 in sp: add sp, sp, x0 // sp = orig_sp + orig_x0 sub x0, sp, x0 // x0 = orig_sp < check > sub x0, sp, x0 // x0 = orig_x0 sub sp, sp, x0 // sp = orig_sp ... which works in a locally-built kernel where I've aligned all the stacks. > ... only, this requires a dedicated stack region, and so we'd need to > check whether sp is inside that window as well. > > The easieast way would be to use a window whose start address is base2 > aligned, but that means the beginning of the kernel VA range (where > KASAN currently lives, and cannot be moved afaik), or a window at the > top of the linear region. Neither look very appealing > > So that means arbitrary low and high limits to compare against in this > entry path. That means more GPRs I'm afraid. Could you elaborate on that? I'm not sure that I follow. My understanding was that the comprimise with this approach is that we only catch overflow/underflow within THREAD_SIZE of the stack, and can get false-negatives elsewhere. Otherwise, IIUC this is sufficient Are you after a more stringent check (like those from the two existing proposals that caught all out-of-bounds accesses)? Or am I missing something else? Thanks, Mark.
[toc] | [prev] | [next] | [standalone]
| From | Ard Biesheuvel <ard.biesheuvel@linaro.org> |
|---|---|
| Date | 2017-07-14 16:20 +0200 |
| Subject | Re: [kernel-hardening] Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP |
| Message-ID | <u39BM-4A9-21@gated-at.bofh.it> |
| In reply to | #1687461 |
On 14 July 2017 at 15:06, Mark Rutland <mark.rutland@arm.com> wrote: > On Fri, Jul 14, 2017 at 01:27:14PM +0100, Ard Biesheuvel wrote: >> On 14 July 2017 at 11:48, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> > On 14 July 2017 at 11:32, Mark Rutland <mark.rutland@arm.com> wrote: >> >> On Thu, Jul 13, 2017 at 07:28:48PM +0100, Ard Biesheuvel wrote: > >> >>> OK, so here's a crazy idea: what if we >> >>> a) carve out a dedicated range in the VMALLOC area for stacks >> >>> b) for each stack, allocate a naturally aligned window of 2x the stack >> >>> size, and map the stack inside it, leaving the remaining space >> >>> unmapped > >> >> The logical ops (TST) and conditional branches (TB(N)Z, CB(N)Z) operate >> >> on XZR rather than SP, so to do this we need to get the SP value into a >> >> GPR. >> >> >> >> Previously, I assumed this meant we needed to corrupt a GPR (and hence >> >> stash that GPR in a sysreg), so I started writing code to free sysregs. >> >> >> >> However, I now realise I was being thick, since we can stash the GPR >> >> in the SP: >> >> >> >> sub sp, sp, x0 // sp = orig_sp - x0 >> >> add x0, sp, x0 // x0 = x0 - (orig_sp - x0) == orig_sp > > That comment is off, and should say x0 = x0 + (orig_sp - x0) == orig_sp > >> >> sub x0, x0, #S_FRAME_SIZE >> >> tb(nz) x0, #THREAD_SHIFT, overflow >> >> add x0, x0, #S_FRAME_SIZE >> >> sub x0, sp, x0 >> >> You need a neg x0, x0 here I think > > Oh, whoops. I'd mis-simplified things. > > We can avoid that by storing orig_sp + orig_x0 in sp: > > add sp, sp, x0 // sp = orig_sp + orig_x0 > sub x0, sp, x0 // x0 = orig_sp > < check > > sub x0, sp, x0 // x0 = orig_x0 > sub sp, sp, x0 // sp = orig_sp > > ... which works in a locally-built kernel where I've aligned all the > stacks. > Yes, that looks correct to me now. >> ... only, this requires a dedicated stack region, and so we'd need to >> check whether sp is inside that window as well. >> >> The easieast way would be to use a window whose start address is base2 >> aligned, but that means the beginning of the kernel VA range (where >> KASAN currently lives, and cannot be moved afaik), or a window at the >> top of the linear region. Neither look very appealing >> >> So that means arbitrary low and high limits to compare against in this >> entry path. That means more GPRs I'm afraid. > > Could you elaborate on that? I'm not sure that I follow. > > My understanding was that the comprimise with this approach is that we > only catch overflow/underflow within THREAD_SIZE of the stack, and can > get false-negatives elsewhere. Otherwise, IIUC this is sufficient > > Are you after a more stringent check (like those from the two existing > proposals that caught all out-of-bounds accesses)? > > Or am I missing something else? > No, not at all. I managed to confuse myself into thinking that we need to validate the value of SP in some way, i.e., as we would when dealing with an arbitrary faulting address.
[toc] | [prev] | [next] | [standalone]
| From | Robin Murphy <robin.murphy@arm.com> |
|---|---|
| Date | 2017-07-14 16:40 +0200 |
| Subject | Re: [kernel-hardening] Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP |
| Message-ID | <u39V8-4Hr-19@gated-at.bofh.it> |
| In reply to | #1687461 |
On 14/07/17 15:06, Mark Rutland wrote: > On Fri, Jul 14, 2017 at 01:27:14PM +0100, Ard Biesheuvel wrote: >> On 14 July 2017 at 11:48, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >>> On 14 July 2017 at 11:32, Mark Rutland <mark.rutland@arm.com> wrote: >>>> On Thu, Jul 13, 2017 at 07:28:48PM +0100, Ard Biesheuvel wrote: > >>>>> OK, so here's a crazy idea: what if we >>>>> a) carve out a dedicated range in the VMALLOC area for stacks >>>>> b) for each stack, allocate a naturally aligned window of 2x the stack >>>>> size, and map the stack inside it, leaving the remaining space >>>>> unmapped > >>>> The logical ops (TST) and conditional branches (TB(N)Z, CB(N)Z) operate >>>> on XZR rather than SP, so to do this we need to get the SP value into a >>>> GPR. >>>> >>>> Previously, I assumed this meant we needed to corrupt a GPR (and hence >>>> stash that GPR in a sysreg), so I started writing code to free sysregs. >>>> >>>> However, I now realise I was being thick, since we can stash the GPR >>>> in the SP: >>>> >>>> sub sp, sp, x0 // sp = orig_sp - x0 >>>> add x0, sp, x0 // x0 = x0 - (orig_sp - x0) == orig_sp > > That comment is off, and should say x0 = x0 + (orig_sp - x0) == orig_sp > >>>> sub x0, x0, #S_FRAME_SIZE >>>> tb(nz) x0, #THREAD_SHIFT, overflow >>>> add x0, x0, #S_FRAME_SIZE >>>> sub x0, sp, x0 >> >> You need a neg x0, x0 here I think > > Oh, whoops. I'd mis-simplified things. > > We can avoid that by storing orig_sp + orig_x0 in sp: > > add sp, sp, x0 // sp = orig_sp + orig_x0 > sub x0, sp, x0 // x0 = orig_sp > < check > > sub x0, sp, x0 // x0 = orig_x0 Haven't you now forcibly cleared the top bit of x0 thanks to overflow? Robin. > sub sp, sp, x0 // sp = orig_sp > > ... which works in a locally-built kernel where I've aligned all the > stacks. > >> ... only, this requires a dedicated stack region, and so we'd need to >> check whether sp is inside that window as well. >> >> The easieast way would be to use a window whose start address is base2 >> aligned, but that means the beginning of the kernel VA range (where >> KASAN currently lives, and cannot be moved afaik), or a window at the >> top of the linear region. Neither look very appealing >> >> So that means arbitrary low and high limits to compare against in this >> entry path. That means more GPRs I'm afraid. > > Could you elaborate on that? I'm not sure that I follow. > > My understanding was that the comprimise with this approach is that we > only catch overflow/underflow within THREAD_SIZE of the stack, and can > get false-negatives elsewhere. Otherwise, IIUC this is sufficient > > Are you after a more stringent check (like those from the two existing > proposals that caught all out-of-bounds accesses)? > > Or am I missing something else? > > Thanks, > Mark. > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
[toc] | [prev] | [next] | [standalone]
| From | Robin Murphy <robin.murphy@arm.com> |
|---|---|
| Date | 2017-07-14 17:10 +0200 |
| Subject | Re: [kernel-hardening] Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP |
| Message-ID | <u3aoa-58i-19@gated-at.bofh.it> |
| In reply to | #1687478 |
On 14/07/17 15:39, Robin Murphy wrote:
> On 14/07/17 15:06, Mark Rutland wrote:
>> On Fri, Jul 14, 2017 at 01:27:14PM +0100, Ard Biesheuvel wrote:
>>> On 14 July 2017 at 11:48, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>> On 14 July 2017 at 11:32, Mark Rutland <mark.rutland@arm.com> wrote:
>>>>> On Thu, Jul 13, 2017 at 07:28:48PM +0100, Ard Biesheuvel wrote:
>>
>>>>>> OK, so here's a crazy idea: what if we
>>>>>> a) carve out a dedicated range in the VMALLOC area for stacks
>>>>>> b) for each stack, allocate a naturally aligned window of 2x the stack
>>>>>> size, and map the stack inside it, leaving the remaining space
>>>>>> unmapped
>>
>>>>> The logical ops (TST) and conditional branches (TB(N)Z, CB(N)Z) operate
>>>>> on XZR rather than SP, so to do this we need to get the SP value into a
>>>>> GPR.
>>>>>
>>>>> Previously, I assumed this meant we needed to corrupt a GPR (and hence
>>>>> stash that GPR in a sysreg), so I started writing code to free sysregs.
>>>>>
>>>>> However, I now realise I was being thick, since we can stash the GPR
>>>>> in the SP:
>>>>>
>>>>> sub sp, sp, x0 // sp = orig_sp - x0
>>>>> add x0, sp, x0 // x0 = x0 - (orig_sp - x0) == orig_sp
>>
>> That comment is off, and should say x0 = x0 + (orig_sp - x0) == orig_sp
>>
>>>>> sub x0, x0, #S_FRAME_SIZE
>>>>> tb(nz) x0, #THREAD_SHIFT, overflow
>>>>> add x0, x0, #S_FRAME_SIZE
>>>>> sub x0, sp, x0
>>>
>>> You need a neg x0, x0 here I think
>>
>> Oh, whoops. I'd mis-simplified things.
>>
>> We can avoid that by storing orig_sp + orig_x0 in sp:
>>
>> add sp, sp, x0 // sp = orig_sp + orig_x0
>> sub x0, sp, x0 // x0 = orig_sp
>> < check >
>> sub x0, sp, x0 // x0 = orig_x0
>
> Haven't you now forcibly cleared the top bit of x0 thanks to overflow?
...or maybe not. I still can't quite see it, but I suppose it must
cancel out somewhere, since Mr. Helpful C Program[1] has apparently
proven me mistaken :(
I guess that means I approve!
Robin.
[1]:
#include <assert.h>
#include <stdint.h>
int main(void) {
for (int i = 0; i < 256; i++) {
for (int j = 0; j < 256; j++) {
uint8_t x = i;
uint8_t y = j;
y = y + x;
x = y - x;
x = y - x;
y = y - x;
assert(x == i && y == j);
}
}
}
>> sub sp, sp, x0 // sp = orig_sp
>>
>> ... which works in a locally-built kernel where I've aligned all the
>> stacks.
>>
>>> ... only, this requires a dedicated stack region, and so we'd need to
>>> check whether sp is inside that window as well.
>>>
>>> The easieast way would be to use a window whose start address is base2
>>> aligned, but that means the beginning of the kernel VA range (where
>>> KASAN currently lives, and cannot be moved afaik), or a window at the
>>> top of the linear region. Neither look very appealing
>>>
>>> So that means arbitrary low and high limits to compare against in this
>>> entry path. That means more GPRs I'm afraid.
>>
>> Could you elaborate on that? I'm not sure that I follow.
>>
>> My understanding was that the comprimise with this approach is that we
>> only catch overflow/underflow within THREAD_SIZE of the stack, and can
>> get false-negatives elsewhere. Otherwise, IIUC this is sufficient
>>
>> Are you after a more stringent check (like those from the two existing
>> proposals that caught all out-of-bounds accesses)?
>>
>> Or am I missing something else?
>>
>> Thanks,
>> Mark.
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
[toc] | [prev] | [next] | [standalone]
| From | Ard Biesheuvel <ard.biesheuvel@linaro.org> |
|---|---|
| Date | 2017-07-14 17:20 +0200 |
| Subject | Re: [kernel-hardening] Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP |
| Message-ID | <u3axQ-5e2-5@gated-at.bofh.it> |
| In reply to | #1687501 |
On 14 July 2017 at 16:03, Robin Murphy <robin.murphy@arm.com> wrote:
> On 14/07/17 15:39, Robin Murphy wrote:
>> On 14/07/17 15:06, Mark Rutland wrote:
>>> On Fri, Jul 14, 2017 at 01:27:14PM +0100, Ard Biesheuvel wrote:
>>>> On 14 July 2017 at 11:48, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>>> On 14 July 2017 at 11:32, Mark Rutland <mark.rutland@arm.com> wrote:
>>>>>> On Thu, Jul 13, 2017 at 07:28:48PM +0100, Ard Biesheuvel wrote:
>>>
>>>>>>> OK, so here's a crazy idea: what if we
>>>>>>> a) carve out a dedicated range in the VMALLOC area for stacks
>>>>>>> b) for each stack, allocate a naturally aligned window of 2x the stack
>>>>>>> size, and map the stack inside it, leaving the remaining space
>>>>>>> unmapped
>>>
>>>>>> The logical ops (TST) and conditional branches (TB(N)Z, CB(N)Z) operate
>>>>>> on XZR rather than SP, so to do this we need to get the SP value into a
>>>>>> GPR.
>>>>>>
>>>>>> Previously, I assumed this meant we needed to corrupt a GPR (and hence
>>>>>> stash that GPR in a sysreg), so I started writing code to free sysregs.
>>>>>>
>>>>>> However, I now realise I was being thick, since we can stash the GPR
>>>>>> in the SP:
>>>>>>
>>>>>> sub sp, sp, x0 // sp = orig_sp - x0
>>>>>> add x0, sp, x0 // x0 = x0 - (orig_sp - x0) == orig_sp
>>>
>>> That comment is off, and should say x0 = x0 + (orig_sp - x0) == orig_sp
>>>
>>>>>> sub x0, x0, #S_FRAME_SIZE
>>>>>> tb(nz) x0, #THREAD_SHIFT, overflow
>>>>>> add x0, x0, #S_FRAME_SIZE
>>>>>> sub x0, sp, x0
>>>>
>>>> You need a neg x0, x0 here I think
>>>
>>> Oh, whoops. I'd mis-simplified things.
>>>
>>> We can avoid that by storing orig_sp + orig_x0 in sp:
>>>
>>> add sp, sp, x0 // sp = orig_sp + orig_x0
>>> sub x0, sp, x0 // x0 = orig_sp
>>> < check >
>>> sub x0, sp, x0 // x0 = orig_x0
>>
>> Haven't you now forcibly cleared the top bit of x0 thanks to overflow?
>
> ...or maybe not. I still can't quite see it, but I suppose it must
> cancel out somewhere, since Mr. Helpful C Program[1] has apparently
> proven me mistaken :(
>
> I guess that means I approve!
>
> Robin.
>
> [1]:
> #include <assert.h>
> #include <stdint.h>
>
> int main(void) {
> for (int i = 0; i < 256; i++) {
> for (int j = 0; j < 256; j++) {
> uint8_t x = i;
> uint8_t y = j;
> y = y + x;
> x = y - x;
> x = y - x;
> y = y - x;
> assert(x == i && y == j);
> }
> }
> }
>
Yeah, I think the carry out in the first instruction can be ignored,
given that we don't care about the magnitude of the result, only about
the lower 64-bits. The subtraction that inverts it will be off by
exactly 2^64
[toc] | [prev] | [next] | [standalone]
| From | Mark Rutland <mark.rutland@arm.com> |
|---|---|
| Date | 2017-07-14 17:30 +0200 |
| Subject | Re: [kernel-hardening] Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP |
| Message-ID | <u3aHw-5hS-1@gated-at.bofh.it> |
| In reply to | #1687501 |
On Fri, Jul 14, 2017 at 04:03:51PM +0100, Robin Murphy wrote:
> On 14/07/17 15:39, Robin Murphy wrote:
> > On 14/07/17 15:06, Mark Rutland wrote:
> >> add sp, sp, x0 // sp = orig_sp + orig_x0
> >> sub x0, sp, x0 // x0 = orig_sp
> >> < check >
> >> sub x0, sp, x0 // x0 = orig_x0
> >
> > Haven't you now forcibly cleared the top bit of x0 thanks to overflow?
>
> ...or maybe not. I still can't quite see it, but I suppose it must
> cancel out somewhere, since Mr. Helpful C Program[1] has apparently
> proven me mistaken :(
>
> I guess that means I approve!
>
> Robin.
>
> [1]:
> #include <assert.h>
> #include <stdint.h>
>
> int main(void) {
> for (int i = 0; i < 256; i++) {
> for (int j = 0; j < 256; j++) {
> uint8_t x = i;
> uint8_t y = j;
> y = y + x;
> x = y - x;
> x = y - x;
> y = y - x;
> assert(x == i && y == j);
> }
> }
> }
I guess we have our first Tested-by for this series. :)
Thanks for taking a look!
Mark.
[toc] | [prev] | [next] | [standalone]
| From | Mark Rutland <mark.rutland@arm.com> |
|---|---|
| Date | 2017-07-14 23:30 +0200 |
| Subject | Re: [kernel-hardening] Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP |
| Message-ID | <u3gjU-H8-5@gated-at.bofh.it> |
| In reply to | #1687461 |
On Fri, Jul 14, 2017 at 03:06:06PM +0100, Mark Rutland wrote: > On Fri, Jul 14, 2017 at 01:27:14PM +0100, Ard Biesheuvel wrote: > > On 14 July 2017 at 11:48, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > > On 14 July 2017 at 11:32, Mark Rutland <mark.rutland@arm.com> wrote: > > >> On Thu, Jul 13, 2017 at 07:28:48PM +0100, Ard Biesheuvel wrote: > > > >>> OK, so here's a crazy idea: what if we > > >>> a) carve out a dedicated range in the VMALLOC area for stacks > > >>> b) for each stack, allocate a naturally aligned window of 2x the stack > > >>> size, and map the stack inside it, leaving the remaining space > > >>> unmapped > > > >> The logical ops (TST) and conditional branches (TB(N)Z, CB(N)Z) operate > > >> on XZR rather than SP, so to do this we need to get the SP value into a > > >> GPR. > > >> > > >> Previously, I assumed this meant we needed to corrupt a GPR (and hence > > >> stash that GPR in a sysreg), so I started writing code to free sysregs. > > >> > > >> However, I now realise I was being thick, since we can stash the GPR > > >> in the SP: > > >> > > >> sub sp, sp, x0 // sp = orig_sp - x0 > > >> add x0, sp, x0 // x0 = x0 - (orig_sp - x0) == orig_sp > > That comment is off, and should say x0 = x0 + (orig_sp - x0) == orig_sp > > > >> sub x0, x0, #S_FRAME_SIZE > > >> tb(nz) x0, #THREAD_SHIFT, overflow > > >> add x0, x0, #S_FRAME_SIZE > > >> sub x0, sp, x0 > > > > You need a neg x0, x0 here I think > > Oh, whoops. I'd mis-simplified things. > > We can avoid that by storing orig_sp + orig_x0 in sp: > > add sp, sp, x0 // sp = orig_sp + orig_x0 > sub x0, sp, x0 // x0 = orig_sp > < check > > sub x0, sp, x0 // x0 = orig_x0 > sub sp, sp, x0 // sp = orig_sp > > ... which works in a locally-built kernel where I've aligned all the > stacks. FWIW, I've pushed out a somewhat cleaned-up (and slightly broken!) version of said kernel source to my arm64/vmap-stack-align branch [1]. That's still missing the backtrace handling, IRQ stack alignment is broken at least on 64K pages, and there's still more cleanup and rework to do. Thanks, Mark. [1] git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git arm64/vmap-stack-align
[toc] | [prev] | [next] | [standalone]
| From | Ard Biesheuvel <ard.biesheuvel@linaro.org> |
|---|---|
| Date | 2017-07-16 02:10 +0200 |
| Subject | Re: [kernel-hardening] Re: [RFC PATCH 6/6] arm64: add VMAP_STACK and detect out-of-bounds SP |
| Message-ID | <u3Fih-8fm-1@gated-at.bofh.it> |
| In reply to | #1687673 |
On 14 July 2017 at 22:27, Mark Rutland <mark.rutland@arm.com> wrote: > On Fri, Jul 14, 2017 at 03:06:06PM +0100, Mark Rutland wrote: >> On Fri, Jul 14, 2017 at 01:27:14PM +0100, Ard Biesheuvel wrote: >> > On 14 July 2017 at 11:48, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> > > On 14 July 2017 at 11:32, Mark Rutland <mark.rutland@arm.com> wrote: >> > >> On Thu, Jul 13, 2017 at 07:28:48PM +0100, Ard Biesheuvel wrote: >> >> > >>> OK, so here's a crazy idea: what if we >> > >>> a) carve out a dedicated range in the VMALLOC area for stacks >> > >>> b) for each stack, allocate a naturally aligned window of 2x the stack >> > >>> size, and map the stack inside it, leaving the remaining space >> > >>> unmapped >> >> > >> The logical ops (TST) and conditional branches (TB(N)Z, CB(N)Z) operate >> > >> on XZR rather than SP, so to do this we need to get the SP value into a >> > >> GPR. >> > >> >> > >> Previously, I assumed this meant we needed to corrupt a GPR (and hence >> > >> stash that GPR in a sysreg), so I started writing code to free sysregs. >> > >> >> > >> However, I now realise I was being thick, since we can stash the GPR >> > >> in the SP: >> > >> >> > >> sub sp, sp, x0 // sp = orig_sp - x0 >> > >> add x0, sp, x0 // x0 = x0 - (orig_sp - x0) == orig_sp >> >> That comment is off, and should say x0 = x0 + (orig_sp - x0) == orig_sp >> >> > >> sub x0, x0, #S_FRAME_SIZE >> > >> tb(nz) x0, #THREAD_SHIFT, overflow >> > >> add x0, x0, #S_FRAME_SIZE >> > >> sub x0, sp, x0 >> > >> > You need a neg x0, x0 here I think >> >> Oh, whoops. I'd mis-simplified things. >> >> We can avoid that by storing orig_sp + orig_x0 in sp: >> >> add sp, sp, x0 // sp = orig_sp + orig_x0 >> sub x0, sp, x0 // x0 = orig_sp >> < check > >> sub x0, sp, x0 // x0 = orig_x0 >> sub sp, sp, x0 // sp = orig_sp >> >> ... which works in a locally-built kernel where I've aligned all the >> stacks. > > FWIW, I've pushed out a somewhat cleaned-up (and slightly broken!) > version of said kernel source to my arm64/vmap-stack-align branch [1]. > That's still missing the backtrace handling, IRQ stack alignment is > broken at least on 64K pages, and there's still more cleanup and rework > to do. > I have spent some time addressing the issues mentioned in the commit log. Please take a look. git://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git vmap-arm64-mark
[toc] | [prev] | [next] | [standalone]
Page 1 of 2 [1] 2 Next page →
Back to top | Article view | linux.kernel
csiph-web