On 02/21/2018 07:38 AM, Mark Rutland wrote:
Hi Laura,
On Tue, Feb 20, 2018 at 05:13:03PM -0800, Laura Abbott wrote:
Implementation of stackleak based heavily on the x86 version
Neat!
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index ec2ee720e33e..b909b436293a 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -401,6 +401,11 @@ tsk .req x28 // current thread_info
.text
+ .macro erase_kstack
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+ bl __erase_kstack
+#endif
+ .endm
/*
* Exception vectors.
*/
@@ -901,6 +906,7 @@ work_pending:
*/
ret_to_user:
disable_daif
+ erase_kstack
I *think* this should happen in finish_ret_to_user a few lines down, since we
can call C code if we branch to work_pending, dirtying the stack.
I think you're right but this didn't immediately work when I tried it.
I'll have to dig into this some more.
ldr x1, [tsk, #TSK_TI_FLAGS]
and x2, x1, #_TIF_WORK_MASK
cbnz x2, work_pending
@@ -1337,3 +1343,105 @@ alternative_else_nop_endif
ENDPROC(__sdei_asm_handler)
NOKPROBE(__sdei_asm_handler)
#endif /* CONFIG_ARM_SDE_INTERFACE */
+
+/*
+ * This is what the stack looks like
+ *
+ * +---+ <- task_stack_page(p) + THREAD_SIZE
+ * | |
+ * +---+ <- task_stack_page(p) + THREAD_START_SP
+ * | |
+ * | |
+ * +---+ <- task_pt_regs(p)
THREAD_START_SP got killed off in commit 34be98f4944f9907 as part of the
VMAP_STACK rework, so this can be:
+---+ <- task_stack_page(p) + THREAD_SIZE
| |
| |
+---+ <- task_pt_regs(p)
...
Good point.
+ * | |
+ * | |
+ * | | <- current_sp
+ * ~~~~~
+ *
+ * ~~~~~
+ * | | <- lowest_stack
+ * | |
+ * | |
+ * +---+ <- task_stack_page(p)
+ *
+ * This function is desgned to poison the memory between the lowest_stack
+ * and the current stack pointer. After clearing the stack, the lowest
+ * stack is reset.
+ */
+
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+ENTRY(__erase_kstack)
+ mov x10, x0 // save x0 for the fast path
AFAICT, we only call this from ret_to_user, where x0 doesn't need to be
preserved.
Is that for ret_fast_syscall? In some cases, ret_fast_syscall can bypass
ret_to_user and calls kernel_exit directly, so we might need a call there.
This was a hold over when I was experimenting with calling erase_kstack
more places, one of which came through ret_fast_syscall. I realized
later that the erase was unnecessary but accidentally kept the saving
in. I'll see about removing it assuming we don't decide later to put
a call on the fast path.
+
+ get_thread_info x0
+ ldr x1, [x0, #TSK_TI_LOWEST_STACK]
+
+ /* get the number of bytes to check for lowest stack */
+ mov x3, x1
+ and x3, x3, #THREAD_SIZE - 1
+ lsr x3, x3, #3
+
+ /* generate addresses from the bottom of the stack */
+ mov x4, sp
+ movn x2, #THREAD_SIZE - 1
+ and x1, x4, x2
Can we replace the MOVN;AND with a single instruction to clear the low bits?
e.g.
mov x4, sp
bic x1, x4, #THREAD_SIZE - 1
... IIUC BIC is an alias for the bitfield instructions, though I can't recall
exactly which one(s).
Good suggestion.
+
+ mov x2, #STACKLEAK_POISON
+
+ mov x5, #0
+1:
+ /*
+ * As borrowed from the x86 logic, start from the lowest_stack
+ * and go to the bottom to find the poison value.
+ * The check of 16 is to hopefully avoid false positives.
+ */
+ cbz x3, 4f
+ ldr x4, [x1, x3, lsl #3]
+ cmp x4, x2
+ csinc x5, xzr, x5, ne
+ tbnz x5, #STACKLEAK_POISON_CHECK_DEPTH/4, 4f // found 16 poisons?
+ sub x3, x3, #1
+ b 1b
+
+4:
+ /* total number of bytes to poison */
+ add x5, x1, x3, lsl #3
+ mov x4, sp
+ sub x8, x4, x5
+
+ cmp x8, #THREAD_SIZE // sanity check the range
+ b.lo 5f
+ ASM_BUG()
+
+5:
+ /*
+ * We may have hit a path where the stack did not get used,
+ * no need to do anything here
+ */
+ cbz x8, 7f
+
+ sub x8, x8, #1 // don't poison the current stack pointer
+
+ lsr x8, x8, #3
+ add x3, x3, x8
+
+ /*
+ * The logic of this loop ensures the last stack word isn't
+ * ovewritten.
+ */
Is that to ensure that we don't clobber the word at the current sp value?
Correct.
+6:
+ cbz x8, 7f
+ str x2, [x1, x3, lsl #3]
+ sub x3, x3, #1
+ sub x8, x8, #1
+ b 6b
+
+ /* Reset the lowest stack to the top of the stack */
+7:
+ mov x1, sp
+ str x1, [x0, #TSK_TI_LOWEST_STACK]
+
+ mov x0, x10
+ ret
+ENDPROC(__erase_kstack)
+#endif
[...]
diff --git a/drivers/firmware/efi/libstub/Makefile
b/drivers/firmware/efi/libstub/Makefile
index 7b3ba40f0745..35ebbc1b17ff 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -20,7 +20,8 @@ cflags-$(CONFIG_EFI_ARMSTUB) +=
-I$(srctree)/scripts/dtc/libfdt
KBUILD_CFLAGS := $(cflags-y) -DDISABLE_BRANCH_PROFILING \
-D__NO_FORTIFY \
$(call cc-option,-ffreestanding) \
- $(call cc-option,-fno-stack-protector)
+ $(call cc-option,-fno-stack-protector) \
+ $(DISABLE_STACKLEAK_PLUGIN)
I believe the KVM hyp code will also need to opt-out of this.
I'll double check that.
Thanks,
Mark.
Thanks,
Laura
