Date: Wed, 21 Feb 2018 15:53:35 -0800 From: Laura Abbott <labbott@...hat.com> To: Mark Rutland <mark.rutland@....com> Cc: Alexander Popov <alex.popov@...ux.com>, Kees Cook <keescook@...omium.org>, Ard Biesheuvel <ard.biesheuvel@...aro.org>, kernel-hardening@...ts.openwall.com, linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH 2/2] arm64: Clear the stack 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
Powered by blists - more mailing lists
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.