From: Laura Abbott <labbott@redhat.com> To: Mark Rutland <mark.rutland@arm.com> Cc: Alexander Popov <alex.popov@linux.com>, Kees Cook <keescook@chromium.org>, Ard Biesheuvel <ard.biesheuvel@linaro.org>, kernel-hardening@lists.openwall.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/2] arm64: Clear the stack Date: Wed, 21 Feb 2018 15:53:35 -0800 [thread overview] Message-ID: <3b76d238-e10a-9abf-c9cb-6d3738eb7896@redhat.com> (raw) In-Reply-To: <20180221153850.ywpzsigfnz3etoun@salmiak> 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
WARNING: multiple messages have this Message-ID (diff)
From: labbott@redhat.com (Laura Abbott) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 2/2] arm64: Clear the stack Date: Wed, 21 Feb 2018 15:53:35 -0800 [thread overview] Message-ID: <3b76d238-e10a-9abf-c9cb-6d3738eb7896@redhat.com> (raw) In-Reply-To: <20180221153850.ywpzsigfnz3etoun@salmiak> 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
next prev parent reply other threads:[~2018-02-21 23:53 UTC|newest] Thread overview: 113+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-02-16 18:10 [PATCH RFC v8 0/6] Introduce the STACKLEAK feature and a test for it Alexander Popov 2018-02-16 18:10 ` [PATCH RFC v8 1/6] x86/entry: Add STACKLEAK erasing the kernel stack at the end of syscalls Alexander Popov 2018-02-21 13:24 ` Borislav Petkov 2018-02-21 21:49 ` Alexander Popov 2018-02-22 19:14 ` Borislav Petkov 2018-02-22 20:24 ` Alexander Popov 2018-02-16 18:10 ` [PATCH RFC v8 2/6] gcc-plugins: Add STACKLEAK plugin for tracking the kernel stack Alexander Popov 2018-02-16 18:10 ` [PATCH RFC v8 3/6] x86/entry: Erase kernel stack in syscall_trace_enter() Alexander Popov 2018-02-16 18:10 ` [PATCH RFC v8 4/6] lkdtm: Add a test for STACKLEAK Alexander Popov 2018-02-16 18:10 ` [PATCH RFC v8 5/6] fs/proc: Show STACKLEAK metrics in the /proc file system Alexander Popov 2018-02-16 18:10 ` [PATCH RFC v8 6/6] doc: self-protection: Add information about STACKLEAK feature Alexander Popov 2018-02-20 10:29 ` [PATCH RFC v8 0/6] Introduce the STACKLEAK feature and a test for it Alexander Popov 2018-02-20 23:17 ` Kees Cook 2018-02-20 23:33 ` Laura Abbott 2018-02-21 1:13 ` [PATCH 0/2] Stackleak for arm64 Laura Abbott 2018-02-21 1:13 ` Laura Abbott 2018-02-21 1:13 ` [PATCH 1/2] stackleak: Update " Laura Abbott 2018-02-21 1:13 ` Laura Abbott 2018-02-22 16:58 ` Will Deacon 2018-02-22 16:58 ` Will Deacon 2018-02-22 19:22 ` Alexander Popov 2018-02-22 19:22 ` Alexander Popov 2018-02-27 10:21 ` Richard Sandiford 2018-02-27 10:21 ` Richard Sandiford 2018-02-27 10:21 ` Richard Sandiford 2018-02-28 15:09 ` Alexander Popov 2018-02-28 15:09 ` Alexander Popov 2018-03-01 10:33 ` Richard Sandiford 2018-03-01 10:33 ` Richard Sandiford 2018-03-01 10:33 ` Richard Sandiford 2018-03-02 11:14 ` Alexander Popov 2018-03-02 11:14 ` Alexander Popov 2018-02-22 19:38 ` Laura Abbott 2018-02-22 19:38 ` Laura Abbott 2018-02-21 1:13 ` [PATCH 2/2] arm64: Clear the stack Laura Abbott 2018-02-21 1:13 ` Laura Abbott 2018-02-21 15:38 ` Mark Rutland 2018-02-21 15:38 ` Mark Rutland 2018-02-21 23:53 ` Laura Abbott [this message] 2018-02-21 23:53 ` Laura Abbott 2018-02-22 1:35 ` Laura Abbott 2018-02-22 1:35 ` Laura Abbott 2018-02-21 14:48 ` [PATCH 0/2] Stackleak for arm64 Alexander Popov 2018-02-21 14:48 ` Alexander Popov 2018-02-21 10:05 ` [PATCH RFC v8 0/6] Introduce the STACKLEAK feature and a test for it Borislav Petkov 2018-02-21 15:09 ` Alexander Popov 2018-02-21 14:43 ` Alexander Popov 2018-02-22 1:43 ` Laura Abbott 2018-02-22 23:14 ` [PATCH 0/2] Update stackleak for gcc-8 Laura Abbott 2018-02-22 23:14 ` [PATCH 1/2] gcc-plugins: Update cgraph_create_edge " Laura Abbott 2018-02-22 23:40 ` Kees Cook 2018-02-23 17:30 ` Laura Abbott 2018-02-24 12:36 ` Alexander Popov 2018-02-22 23:14 ` [PATCH 2/2] gcc-plugins: stackleak: Update " Laura Abbott 2018-02-24 14:04 ` Alexander Popov 2018-02-26 21:51 ` Laura Abbott 2018-02-27 10:30 ` Richard Sandiford 2018-02-28 10:27 ` Alexander Popov 2018-02-22 23:43 ` [PATCH 0/2] Update stackleak " Kees Cook 2018-05-02 20:33 [PATCH 0/2] Stackleak for arm64 Laura Abbott 2018-05-02 20:33 ` [PATCH 2/2] arm64: Clear the stack Laura Abbott 2018-05-02 20:33 ` Laura Abbott 2018-05-02 21:31 ` Kees Cook 2018-05-02 21:31 ` Kees Cook 2018-05-02 23:07 ` Laura Abbott 2018-05-02 23:07 ` Laura Abbott 2018-05-02 23:37 ` Kees Cook 2018-05-02 23:37 ` Kees Cook 2018-05-03 16:05 ` Alexander Popov 2018-05-03 16:05 ` Alexander Popov 2018-05-03 16:45 ` Kees Cook 2018-05-03 16:45 ` Kees Cook 2018-05-03 7:19 ` Mark Rutland 2018-05-03 7:19 ` Mark Rutland 2018-05-03 11:37 ` Ard Biesheuvel 2018-05-03 11:37 ` Ard Biesheuvel 2018-05-03 17:33 ` Alexander Popov 2018-05-03 17:33 ` Alexander Popov 2018-05-03 19:09 ` Laura Abbott 2018-05-03 19:09 ` Laura Abbott 2018-05-04 8:30 ` Alexander Popov 2018-05-04 8:30 ` Alexander Popov 2018-05-04 11:09 ` Mark Rutland 2018-05-04 11:09 ` Mark Rutland 2018-05-06 8:22 ` Alexander Popov 2018-05-06 8:22 ` Alexander Popov 2018-05-11 15:50 ` Alexander Popov 2018-05-11 15:50 ` Alexander Popov 2018-05-11 16:13 ` Mark Rutland 2018-05-11 16:13 ` Mark Rutland 2018-05-13 8:40 ` Alexander Popov 2018-05-13 8:40 ` Alexander Popov 2018-05-14 5:15 ` Mark Rutland 2018-05-14 5:15 ` Mark Rutland 2018-05-14 9:35 ` Alexander Popov 2018-05-14 9:35 ` Alexander Popov 2018-05-14 10:06 ` Mark Rutland 2018-05-14 10:06 ` Mark Rutland 2018-05-14 13:53 ` Alexander Popov 2018-05-14 13:53 ` Alexander Popov 2018-05-14 14:07 ` Mark Rutland 2018-05-14 14:07 ` Mark Rutland 2018-05-03 19:00 ` Laura Abbott 2018-05-03 19:00 ` Laura Abbott 2018-05-04 11:16 ` Mark Rutland 2018-05-04 11:16 ` Mark Rutland 2018-07-18 21:10 [PATCH 0/2] Stackleak for arm64 Laura Abbott 2018-07-18 21:10 ` [PATCH 2/2] arm64: Clear the stack Laura Abbott 2018-07-18 21:10 ` Laura Abbott 2018-07-19 2:20 ` Kees Cook 2018-07-19 2:20 ` Kees Cook 2018-07-19 10:41 ` Alexander Popov 2018-07-19 10:41 ` Alexander Popov 2018-07-19 11:41 ` Mark Rutland 2018-07-19 11:41 ` Mark Rutland
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=3b76d238-e10a-9abf-c9cb-6d3738eb7896@redhat.com \ --to=labbott@redhat.com \ --cc=alex.popov@linux.com \ --cc=ard.biesheuvel@linaro.org \ --cc=keescook@chromium.org \ --cc=kernel-hardening@lists.openwall.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mark.rutland@arm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.