| Message ID | 20180629190553.7282-1-labbott@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jun 29, 2018 at 12:05 PM, Laura Abbott <labbott@redhat.com> wrote:
> Implementation of stackleak based heavily on the x86 version
Awesome!
Now I just have to figure out how to unbreak cross-compilation after
the kconfig changes to gcc-plugins. Whoops. :)
-Kees
On Fri, Jun 29, 2018 at 12:05 PM, Laura Abbott <labbott@redhat.com> wrote: > Implementation of stackleak based heavily on the x86 version > > Signed-off-by: Laura Abbott <labbott@redhat.com> > [...] > +#define current_top_of_stack() (task_stack_page(current) + THREAD_SIZE) > +#define on_thread_stack() (on_task_stack(current, current_stack_pointer)) nit on types here. I get some warnings: kernel/stackleak.c:55:12: warning: assignment makes integer from pointer without a cast [-Wint-conversion] boundary = current_top_of_stack(); ^ kernel/stackleak.c:65:24: warning: assignment makes integer from pointer without a cast [-Wint-conversion] current->lowest_stack = current_top_of_stack() - THREAD_SIZE / 64; ^ So I think this needs to be: +#define current_top_of_stack() ((unsigned long)task_stack_page(current) + \ + THREAD_SIZE) > diff --git a/scripts/Makefile.gcc-plugins b/scripts/Makefile.gcc-plugins > index a535742a1c06..972ce4ca7f6a 100644 > --- a/scripts/Makefile.gcc-plugins > +++ b/scripts/Makefile.gcc-plugins > @@ -37,11 +37,14 @@ ifdef CONFIG_GCC_PLUGINS > > gcc-plugin-$(CONFIG_GCC_PLUGIN_STACKLEAK) += stackleak_plugin.so > gcc-plugin-cflags-$(CONFIG_GCC_PLUGIN_STACKLEAK) += -DSTACKLEAK_PLUGIN -fplugin-arg-stackleak_plugin-track-min-size=$(CONFIG_STACKLEAK_TRACK_MIN_SIZE) > + ifdef CONFIG_GCC_PLUGIN_STACKLEAK > + DISABLE_STACKLEAK_PLUGIN += -fplugin-arg-stackleak_plugin-disable > + endif > > GCC_PLUGINS_CFLAGS := $(strip $(addprefix -fplugin=$(objtree)/scripts/gcc-plugins/, $(gcc-plugin-y)) $(gcc-plugin-cflags-y)) > > export PLUGINCC GCC_PLUGINS_CFLAGS GCC_PLUGIN GCC_PLUGIN_SUBDIR > - export SANCOV_PLUGIN DISABLE_LATENT_ENTROPY_PLUGIN > + export SANCOV_PLUGIN DISABLE_LATENT_ENTROPY_PLUGIN DISABLE_STACKLEAK_PLUGIN > > ifneq ($(PLUGINCC),) > # SANCOV_PLUGIN can be only in CFLAGS_KCOV because avoid duplication. If there is a v14, I think this hunk should be taken there, since it's part of the common code. Otherwise, this works for me and passes the lkdtm tests. -Kees
On 06/29/2018 01:19 PM, Kees Cook wrote: > On Fri, Jun 29, 2018 at 12:05 PM, Laura Abbott <labbott@redhat.com> wrote: >> Implementation of stackleak based heavily on the x86 version >> >> Signed-off-by: Laura Abbott <labbott@redhat.com> >> [...] >> +#define current_top_of_stack() (task_stack_page(current) + THREAD_SIZE) >> +#define on_thread_stack() (on_task_stack(current, current_stack_pointer)) > > nit on types here. I get some warnings: > > kernel/stackleak.c:55:12: warning: assignment makes integer from > pointer without a cast [-Wint-conversion] > boundary = current_top_of_stack(); > ^ > kernel/stackleak.c:65:24: warning: assignment makes integer from > pointer without a cast [-Wint-conversion] > current->lowest_stack = current_top_of_stack() - THREAD_SIZE / 64; > ^ > > So I think this needs to be: > > +#define current_top_of_stack() ((unsigned long)task_stack_page(current) + \ > + THREAD_SIZE) > Argh, missed that in an amend, can fix for next version if there are no other objections to this approach. >> diff --git a/scripts/Makefile.gcc-plugins b/scripts/Makefile.gcc-plugins >> index a535742a1c06..972ce4ca7f6a 100644 >> --- a/scripts/Makefile.gcc-plugins >> +++ b/scripts/Makefile.gcc-plugins >> @@ -37,11 +37,14 @@ ifdef CONFIG_GCC_PLUGINS >> >> gcc-plugin-$(CONFIG_GCC_PLUGIN_STACKLEAK) += stackleak_plugin.so >> gcc-plugin-cflags-$(CONFIG_GCC_PLUGIN_STACKLEAK) += -DSTACKLEAK_PLUGIN -fplugin-arg-stackleak_plugin-track-min-size=$(CONFIG_STACKLEAK_TRACK_MIN_SIZE) >> + ifdef CONFIG_GCC_PLUGIN_STACKLEAK >> + DISABLE_STACKLEAK_PLUGIN += -fplugin-arg-stackleak_plugin-disable >> + endif >> >> GCC_PLUGINS_CFLAGS := $(strip $(addprefix -fplugin=$(objtree)/scripts/gcc-plugins/, $(gcc-plugin-y)) $(gcc-plugin-cflags-y)) >> >> export PLUGINCC GCC_PLUGINS_CFLAGS GCC_PLUGIN GCC_PLUGIN_SUBDIR >> - export SANCOV_PLUGIN DISABLE_LATENT_ENTROPY_PLUGIN >> + export SANCOV_PLUGIN DISABLE_LATENT_ENTROPY_PLUGIN DISABLE_STACKLEAK_PLUGIN >> >> ifneq ($(PLUGINCC),) >> # SANCOV_PLUGIN can be only in CFLAGS_KCOV because avoid duplication. > > If there is a v14, I think this hunk should be taken there, since it's > part of the common code. > > Otherwise, this works for me and passes the lkdtm tests. > > -Kees > Thanks, Laura
On Fri, Jun 29, 2018 at 1:22 PM, Laura Abbott <labbott@redhat.com> wrote: > On 06/29/2018 01:19 PM, Kees Cook wrote: >> >> On Fri, Jun 29, 2018 at 12:05 PM, Laura Abbott <labbott@redhat.com> wrote: >>> >>> Implementation of stackleak based heavily on the x86 version >>> >>> Signed-off-by: Laura Abbott <labbott@redhat.com> >>> [...] >>> +#define current_top_of_stack() (task_stack_page(current) + THREAD_SIZE) >>> +#define on_thread_stack() (on_task_stack(current, >>> current_stack_pointer)) >> >> >> nit on types here. I get some warnings: >> >> kernel/stackleak.c:55:12: warning: assignment makes integer from >> pointer without a cast [-Wint-conversion] >> boundary = current_top_of_stack(); >> ^ >> kernel/stackleak.c:65:24: warning: assignment makes integer from >> pointer without a cast [-Wint-conversion] >> current->lowest_stack = current_top_of_stack() - THREAD_SIZE / 64; >> ^ >> >> So I think this needs to be: >> >> +#define current_top_of_stack() ((unsigned long)task_stack_page(current) + >> \ >> + THREAD_SIZE) >> > > Argh, missed that in an amend, can fix for next version if there > are no other objections to this approach. No worries! I've made the change locally and will push this out to -next unless there are objections? -Kees
Hi Kees, On Fri, Jun 29, 2018 at 01:25:20PM -0700, Kees Cook wrote: > On Fri, Jun 29, 2018 at 1:22 PM, Laura Abbott <labbott@redhat.com> wrote: > > On 06/29/2018 01:19 PM, Kees Cook wrote: > >> > >> On Fri, Jun 29, 2018 at 12:05 PM, Laura Abbott <labbott@redhat.com> wrote: > >>> > >>> Implementation of stackleak based heavily on the x86 version > >>> > >>> Signed-off-by: Laura Abbott <labbott@redhat.com> > >>> [...] > >>> +#define current_top_of_stack() (task_stack_page(current) + THREAD_SIZE) > >>> +#define on_thread_stack() (on_task_stack(current, > >>> current_stack_pointer)) > >> > >> > >> nit on types here. I get some warnings: > >> > >> kernel/stackleak.c:55:12: warning: assignment makes integer from > >> pointer without a cast [-Wint-conversion] > >> boundary = current_top_of_stack(); > >> ^ > >> kernel/stackleak.c:65:24: warning: assignment makes integer from > >> pointer without a cast [-Wint-conversion] > >> current->lowest_stack = current_top_of_stack() - THREAD_SIZE / 64; > >> ^ > >> > >> So I think this needs to be: > >> > >> +#define current_top_of_stack() ((unsigned long)task_stack_page(current) + > >> \ > >> + THREAD_SIZE) > >> > > > > Argh, missed that in an amend, can fix for next version if there > > are no other objections to this approach. > > No worries! I've made the change locally and will push this out to > -next unless there are objections? I'm a bit wary of conflicts in entry.S, since it's likely that we're going to have a lot going on in there for 4.19. Could I take this via arm64 instead, please, or are there dependencies on other parts of your tree? Will
Hello Laura, Thanks for your work! Please see my comments below. On 29.06.2018 22:05, Laura Abbott wrote: > Implementation of stackleak based heavily on the x86 version > > Signed-off-by: Laura Abbott <labbott@redhat.com> > --- > Changes since last time: > - Minor name change in entry.S > - Converted to use the generic interfaces so there's minimal additions. > - Added the fast syscall path. > - Addition of on_thread_stack and current_top_of_stack > - Disable stackleak on hyp per suggestion > - Added a define for check_alloca. I'm still not sure about keeping it > since the x86 version got reworked? > > I've mostly kept this as one patch with a minimal commit text. I can > split it up and elaborate more before final merging. > --- > arch/arm64/Kconfig | 1 + > arch/arm64/include/asm/processor.h | 9 +++++++++ > arch/arm64/kernel/entry.S | 7 +++++++ > arch/arm64/kernel/process.c | 16 ++++++++++++++++ > arch/arm64/kvm/hyp/Makefile | 3 ++- > drivers/firmware/efi/libstub/Makefile | 3 ++- > include/linux/stackleak.h | 1 + > scripts/Makefile.gcc-plugins | 5 ++++- > 8 files changed, 42 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > index eb2cf4938f6d..b0221db95dc9 100644 > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -92,6 +92,7 @@ config ARM64 > select HAVE_ARCH_MMAP_RND_BITS > select HAVE_ARCH_MMAP_RND_COMPAT_BITS if COMPAT > select HAVE_ARCH_SECCOMP_FILTER > + select HAVE_ARCH_STACKLEAK > select HAVE_ARCH_THREAD_STRUCT_WHITELIST > select HAVE_ARCH_TRACEHOOK > select HAVE_ARCH_TRANSPARENT_HUGEPAGE > diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h > index 767598932549..73914fd7e142 100644 > --- a/arch/arm64/include/asm/processor.h > +++ b/arch/arm64/include/asm/processor.h > @@ -248,5 +248,14 @@ void cpu_clear_disr(const struct arm64_cpu_capabilities *__unused); > #define SVE_SET_VL(arg) sve_set_current_vl(arg) > #define SVE_GET_VL() sve_get_current_vl() > > +/* > + * For stackleak May I ask you to call it STACKLEAK here and in other places for easier grep? > + * > + * These need to be macros because otherwise we get stuck in a nightmare > + * of header definitions for the use of task_stack_page. > + */ > +#define current_top_of_stack() (task_stack_page(current) + THREAD_SIZE) > +#define on_thread_stack() (on_task_stack(current, current_stack_pointer)) > + > #endif /* __ASSEMBLY__ */ > #endif /* __ASM_PROCESSOR_H */ > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S > index ec2ee720e33e..31c9da7d401e 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 stackleak_erase Could you rename the macro to STACKLEAK_ERASE for similarity with x86? > +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK > + bl stackleak_erase_kstack > +#endif > + .endm > /* > * Exception vectors. > */ > @@ -880,6 +885,7 @@ ret_fast_syscall: > and x2, x1, #_TIF_WORK_MASK > cbnz x2, work_pending > enable_step_tsk x1, x2 > + stackleak_erase > kernel_exit 0 > ret_fast_syscall_trace: > enable_daif > @@ -906,6 +912,7 @@ ret_to_user: > cbnz x2, work_pending > finish_ret_to_user: > enable_step_tsk x1, x2 > + stackleak_erase > kernel_exit 0 > ENDPROC(ret_to_user) > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > index f08a2ed9db0d..9f0f135f8b66 100644 > --- a/arch/arm64/kernel/process.c > +++ b/arch/arm64/kernel/process.c > @@ -493,3 +493,19 @@ void arch_setup_new_exec(void) > { > current->mm->context.flags = is_compat_task() ? MMCF_AARCH32 : 0; > } > + > +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK > +#define MIN_STACK_LEFT 256 > + > +void __used stackleak_check_alloca(unsigned long size) > +{ > + unsigned long sp, stack_left; > + > + sp = current_stack_pointer; > + > + stack_left = sp & (THREAD_SIZE - 1); > + BUG_ON(stack_left < MIN_STACK_LEFT || > + size >= stack_left - MIN_STACK_LEFT); > +} > +EXPORT_SYMBOL(stackleak_check_alloca); > +#endif This code should be updated. You may remember the troubles I had with MIN_STACK_LEFT: http://openwall.com/lists/kernel-hardening/2018/05/11/12 Please see that thread where Mark Rutland and I worked out the solution. By the way, different stacks on x86_64 have different sizes. Is it false for arm64? > diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile > index 4313f7475333..2fabc2dc1966 100644 > --- a/arch/arm64/kvm/hyp/Makefile > +++ b/arch/arm64/kvm/hyp/Makefile > @@ -3,7 +3,8 @@ > # Makefile for Kernel-based Virtual Machine module, HYP part > # > > -ccflags-y += -fno-stack-protector -DDISABLE_BRANCH_PROFILING > +ccflags-y += -fno-stack-protector -DDISABLE_BRANCH_PROFILING \ > + $(DISABLE_STACKLEAK_PLUGIN) > > KVM=../../../../virt/kvm > > diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile > index a34e9290a699..25dd2a14560d 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) > > GCOV_PROFILE := n > KASAN_SANITIZE := n > diff --git a/include/linux/stackleak.h b/include/linux/stackleak.h > index e2da99b3a191..00d62b302efb 100644 > --- a/include/linux/stackleak.h > +++ b/include/linux/stackleak.h > @@ -5,6 +5,7 @@ > #include <linux/sched.h> > #include <linux/sched/task_stack.h> > > +#include <asm/stacktrace.h> > /* > * Check that the poison value points to the unused hole in the > * virtual memory map for your platform. > diff --git a/scripts/Makefile.gcc-plugins b/scripts/Makefile.gcc-plugins > index a535742a1c06..972ce4ca7f6a 100644 > --- a/scripts/Makefile.gcc-plugins > +++ b/scripts/Makefile.gcc-plugins > @@ -37,11 +37,14 @@ ifdef CONFIG_GCC_PLUGINS > > gcc-plugin-$(CONFIG_GCC_PLUGIN_STACKLEAK) += stackleak_plugin.so > gcc-plugin-cflags-$(CONFIG_GCC_PLUGIN_STACKLEAK) += -DSTACKLEAK_PLUGIN -fplugin-arg-stackleak_plugin-track-min-size=$(CONFIG_STACKLEAK_TRACK_MIN_SIZE) > + ifdef CONFIG_GCC_PLUGIN_STACKLEAK > + DISABLE_STACKLEAK_PLUGIN += -fplugin-arg-stackleak_plugin-disable > + endif > > GCC_PLUGINS_CFLAGS := $(strip $(addprefix -fplugin=$(objtree)/scripts/gcc-plugins/, $(gcc-plugin-y)) $(gcc-plugin-cflags-y)) > > export PLUGINCC GCC_PLUGINS_CFLAGS GCC_PLUGIN GCC_PLUGIN_SUBDIR > - export SANCOV_PLUGIN DISABLE_LATENT_ENTROPY_PLUGIN > + export SANCOV_PLUGIN DISABLE_LATENT_ENTROPY_PLUGIN DISABLE_STACKLEAK_PLUGIN > > ifneq ($(PLUGINCC),) > # SANCOV_PLUGIN can be only in CFLAGS_KCOV because avoid duplication. >
Hi Will, On Mon, Jul 2, 2018 at 2:59 AM, Will Deacon <will.deacon@arm.com> wrote: > On Fri, Jun 29, 2018 at 01:25:20PM -0700, Kees Cook wrote: >> No worries! I've made the change locally and will push this out to >> -next unless there are objections? > > I'm a bit wary of conflicts in entry.S, since it's likely that we're going > to have a lot going on in there for 4.19. > > Could I take this via arm64 instead, please, or are there dependencies > on other parts of your tree? It depends on the plugin existing, but we could split it up so the arm64 part could go separately. It would just be a no-op in the arm64 tree since CONFIG_GCC_PLUGIN_STACKLEAK won't exist there. Whatever works best for you! -Kees
On 07/02/2018 06:02 AM, Alexander Popov wrote: > Hello Laura, > > Thanks for your work! > Please see my comments below. > > On 29.06.2018 22:05, Laura Abbott wrote: >> Implementation of stackleak based heavily on the x86 version >> >> Signed-off-by: Laura Abbott <labbott@redhat.com> >> --- >> Changes since last time: >> - Minor name change in entry.S >> - Converted to use the generic interfaces so there's minimal additions. >> - Added the fast syscall path. >> - Addition of on_thread_stack and current_top_of_stack >> - Disable stackleak on hyp per suggestion >> - Added a define for check_alloca. I'm still not sure about keeping it >> since the x86 version got reworked? >> >> I've mostly kept this as one patch with a minimal commit text. I can >> split it up and elaborate more before final merging. >> --- >> arch/arm64/Kconfig | 1 + >> arch/arm64/include/asm/processor.h | 9 +++++++++ >> arch/arm64/kernel/entry.S | 7 +++++++ >> arch/arm64/kernel/process.c | 16 ++++++++++++++++ >> arch/arm64/kvm/hyp/Makefile | 3 ++- >> drivers/firmware/efi/libstub/Makefile | 3 ++- >> include/linux/stackleak.h | 1 + >> scripts/Makefile.gcc-plugins | 5 ++++- >> 8 files changed, 42 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig >> index eb2cf4938f6d..b0221db95dc9 100644 >> --- a/arch/arm64/Kconfig >> +++ b/arch/arm64/Kconfig >> @@ -92,6 +92,7 @@ config ARM64 >> select HAVE_ARCH_MMAP_RND_BITS >> select HAVE_ARCH_MMAP_RND_COMPAT_BITS if COMPAT >> select HAVE_ARCH_SECCOMP_FILTER >> + select HAVE_ARCH_STACKLEAK >> select HAVE_ARCH_THREAD_STRUCT_WHITELIST >> select HAVE_ARCH_TRACEHOOK >> select HAVE_ARCH_TRANSPARENT_HUGEPAGE >> diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h >> index 767598932549..73914fd7e142 100644 >> --- a/arch/arm64/include/asm/processor.h >> +++ b/arch/arm64/include/asm/processor.h >> @@ -248,5 +248,14 @@ void cpu_clear_disr(const struct arm64_cpu_capabilities *__unused); >> #define SVE_SET_VL(arg) sve_set_current_vl(arg) >> #define SVE_GET_VL() sve_get_current_vl() >> >> +/* >> + * For stackleak > > May I ask you to call it STACKLEAK here and in other places for easier grep? > Sure >> + * >> + * These need to be macros because otherwise we get stuck in a nightmare >> + * of header definitions for the use of task_stack_page. >> + */ >> +#define current_top_of_stack() (task_stack_page(current) + THREAD_SIZE) >> +#define on_thread_stack() (on_task_stack(current, current_stack_pointer)) >> + >> #endif /* __ASSEMBLY__ */ >> #endif /* __ASM_PROCESSOR_H */ >> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S >> index ec2ee720e33e..31c9da7d401e 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 stackleak_erase > > Could you rename the macro to STACKLEAK_ERASE for similarity with x86? > Mark Rutland had previously asked for this to be lowercase. I really don't care one way or the other so I'll defer to someone else to have the final word. >> +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK >> + bl stackleak_erase_kstack >> +#endif >> + .endm >> /* >> * Exception vectors. >> */ >> @@ -880,6 +885,7 @@ ret_fast_syscall: >> and x2, x1, #_TIF_WORK_MASK >> cbnz x2, work_pending >> enable_step_tsk x1, x2 >> + stackleak_erase >> kernel_exit 0 >> ret_fast_syscall_trace: >> enable_daif >> @@ -906,6 +912,7 @@ ret_to_user: >> cbnz x2, work_pending >> finish_ret_to_user: >> enable_step_tsk x1, x2 >> + stackleak_erase >> kernel_exit 0 >> ENDPROC(ret_to_user) >> >> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c >> index f08a2ed9db0d..9f0f135f8b66 100644 >> --- a/arch/arm64/kernel/process.c >> +++ b/arch/arm64/kernel/process.c >> @@ -493,3 +493,19 @@ void arch_setup_new_exec(void) >> { >> current->mm->context.flags = is_compat_task() ? MMCF_AARCH32 : 0; >> } >> + >> +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK >> +#define MIN_STACK_LEFT 256 >> + >> +void __used stackleak_check_alloca(unsigned long size) >> +{ >> + unsigned long sp, stack_left; >> + >> + sp = current_stack_pointer; >> + >> + stack_left = sp & (THREAD_SIZE - 1); >> + BUG_ON(stack_left < MIN_STACK_LEFT || >> + size >= stack_left - MIN_STACK_LEFT); >> +} >> +EXPORT_SYMBOL(stackleak_check_alloca); >> +#endif > > This code should be updated. > You may remember the troubles I had with MIN_STACK_LEFT: > http://openwall.com/lists/kernel-hardening/2018/05/11/12 > Please see that thread where Mark Rutland and I worked out the solution. > Ah yeah, I missed the details in that thread. Thanks for that pointer. > By the way, different stacks on x86_64 have different sizes. Is it false for arm64? > Currently everything except the overflow stack looks to be the same size but there's also another stack I missed. It might be cleaner just to use on_accessible_stack and then another function to get the top of stack. This also might just be reimplementing what x86 already has? (Mark, Ard?) Thanks, Laura >> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile >> index 4313f7475333..2fabc2dc1966 100644 >> --- a/arch/arm64/kvm/hyp/Makefile >> +++ b/arch/arm64/kvm/hyp/Makefile >> @@ -3,7 +3,8 @@ >> # Makefile for Kernel-based Virtual Machine module, HYP part >> # >> >> -ccflags-y += -fno-stack-protector -DDISABLE_BRANCH_PROFILING >> +ccflags-y += -fno-stack-protector -DDISABLE_BRANCH_PROFILING \ >> + $(DISABLE_STACKLEAK_PLUGIN) >> >> KVM=../../../../virt/kvm >> >> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile >> index a34e9290a699..25dd2a14560d 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) >> >> GCOV_PROFILE := n >> KASAN_SANITIZE := n >> diff --git a/include/linux/stackleak.h b/include/linux/stackleak.h >> index e2da99b3a191..00d62b302efb 100644 >> --- a/include/linux/stackleak.h >> +++ b/include/linux/stackleak.h >> @@ -5,6 +5,7 @@ >> #include <linux/sched.h> >> #include <linux/sched/task_stack.h> >> >> +#include <asm/stacktrace.h> >> /* >> * Check that the poison value points to the unused hole in the >> * virtual memory map for your platform. >> diff --git a/scripts/Makefile.gcc-plugins b/scripts/Makefile.gcc-plugins >> index a535742a1c06..972ce4ca7f6a 100644 >> --- a/scripts/Makefile.gcc-plugins >> +++ b/scripts/Makefile.gcc-plugins >> @@ -37,11 +37,14 @@ ifdef CONFIG_GCC_PLUGINS >> >> gcc-plugin-$(CONFIG_GCC_PLUGIN_STACKLEAK) += stackleak_plugin.so >> gcc-plugin-cflags-$(CONFIG_GCC_PLUGIN_STACKLEAK) += -DSTACKLEAK_PLUGIN -fplugin-arg-stackleak_plugin-track-min-size=$(CONFIG_STACKLEAK_TRACK_MIN_SIZE) >> + ifdef CONFIG_GCC_PLUGIN_STACKLEAK >> + DISABLE_STACKLEAK_PLUGIN += -fplugin-arg-stackleak_plugin-disable >> + endif >> >> GCC_PLUGINS_CFLAGS := $(strip $(addprefix -fplugin=$(objtree)/scripts/gcc-plugins/, $(gcc-plugin-y)) $(gcc-plugin-cflags-y)) >> >> export PLUGINCC GCC_PLUGINS_CFLAGS GCC_PLUGIN GCC_PLUGIN_SUBDIR >> - export SANCOV_PLUGIN DISABLE_LATENT_ENTROPY_PLUGIN >> + export SANCOV_PLUGIN DISABLE_LATENT_ENTROPY_PLUGIN DISABLE_STACKLEAK_PLUGIN >> >> ifneq ($(PLUGINCC),) >> # SANCOV_PLUGIN can be only in CFLAGS_KCOV because avoid duplication. >> >
On Mon, Jul 02, 2018 at 11:48:05AM -0700, Laura Abbott wrote: > On 07/02/2018 06:02 AM, Alexander Popov wrote: > > On 29.06.2018 22:05, Laura Abbott wrote: > > > Implementation of stackleak based heavily on the x86 version > > > > > > Signed-off-by: Laura Abbott <labbott@redhat.com> > > > --- > > > Changes since last time: > > > - Minor name change in entry.S > > > - Converted to use the generic interfaces so there's minimal additions. > > > - Added the fast syscall path. > > > - Addition of on_thread_stack and current_top_of_stack > > > - Disable stackleak on hyp per suggestion > > > - Added a define for check_alloca. I'm still not sure about keeping it > > > since the x86 version got reworked? > > > > > > I've mostly kept this as one patch with a minimal commit text. I can > > > split it up and elaborate more before final merging. > > > --- [...] > > > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S > > > index ec2ee720e33e..31c9da7d401e 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 stackleak_erase > > > > Could you rename the macro to STACKLEAK_ERASE for similarity with x86? > > > > Mark Rutland had previously asked for this to be lowercase. > I really don't care one way or the other so I'll defer to > someone else to have the final word. Will, Catalin, could you chime in either way? I'd previously asked for lower-case for consistency with our other assembly macros. [...] > > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > > > index f08a2ed9db0d..9f0f135f8b66 100644 > > > --- a/arch/arm64/kernel/process.c > > > +++ b/arch/arm64/kernel/process.c > > > @@ -493,3 +493,19 @@ void arch_setup_new_exec(void) > > > { > > > current->mm->context.flags = is_compat_task() ? MMCF_AARCH32 : 0; > > > } > > > + > > > +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK > > > +#define MIN_STACK_LEFT 256 > > > + > > > +void __used stackleak_check_alloca(unsigned long size) > > > +{ > > > + unsigned long sp, stack_left; > > > + > > > + sp = current_stack_pointer; > > > + > > > + stack_left = sp & (THREAD_SIZE - 1); > > > + BUG_ON(stack_left < MIN_STACK_LEFT || > > > + size >= stack_left - MIN_STACK_LEFT); > > > +} > > > +EXPORT_SYMBOL(stackleak_check_alloca); > > > +#endif > > > > This code should be updated. > > You may remember the troubles I had with MIN_STACK_LEFT: > > http://openwall.com/lists/kernel-hardening/2018/05/11/12 > > Please see that thread where Mark Rutland and I worked out the solution. > > > > Ah yeah, I missed the details in that thread. Thanks for > that pointer. > > > By the way, different stacks on x86_64 have different sizes. Is it false for arm64? > > Currently everything except the overflow stack looks to be > the same size but there's also another stack I missed. Assuming I've followed the code correctly, we currently have: stack size alignment (minimum) --------------------------------------------------- task THREAD_SIZE THREAD_ALIGN irq THREAD_SIZE 16 overflow SZ_4K 16 sdei_normal THREAD_SIZE THREAD_ALIGN sdei_critical THREAD_SIZE THREAD_ALIGN ... since IRQ_STACK_SIZE is defined as THREAD_SIZE, and SDEI_STACK_SIZE is defined as IRQ_STACK_SIZE. So we can't just mask the sp, unfortunately. > It might be cleaner just to use on_accessible_stack and then another > function to get the top of stack. This also might just be > reimplementing what x86 already has? (Mark, Ard?) It looks like we could build a get_stack_info() as they have. We could probably clean up our stack traced atop of that, too. Thanks, Mark.
On Tue, Jul 03, 2018 at 01:14:41PM +0100, Mark Rutland wrote: > On Mon, Jul 02, 2018 at 11:48:05AM -0700, Laura Abbott wrote: > > On 07/02/2018 06:02 AM, Alexander Popov wrote: > > > On 29.06.2018 22:05, Laura Abbott wrote: > > > > Implementation of stackleak based heavily on the x86 version > > > > > > > > Signed-off-by: Laura Abbott <labbott@redhat.com> > > > > --- > > > > Changes since last time: > > > > - Minor name change in entry.S > > > > - Converted to use the generic interfaces so there's minimal additions. > > > > - Added the fast syscall path. > > > > - Addition of on_thread_stack and current_top_of_stack > > > > - Disable stackleak on hyp per suggestion > > > > - Added a define for check_alloca. I'm still not sure about keeping it > > > > since the x86 version got reworked? > > > > > > > > I've mostly kept this as one patch with a minimal commit text. I can > > > > split it up and elaborate more before final merging. > > > > --- > > [...] > > > > > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S > > > > index ec2ee720e33e..31c9da7d401e 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 stackleak_erase > > > > > > Could you rename the macro to STACKLEAK_ERASE for similarity with x86? > > > > > > > Mark Rutland had previously asked for this to be lowercase. > > I really don't care one way or the other so I'll defer to > > someone else to have the final word. > > Will, Catalin, could you chime in either way? > > I'd previously asked for lower-case for consistency with our other > assembly macros. I'd keep it lowercase as the other arm64 macros in this file.
On 03.07.2018 18:03, Catalin Marinas wrote: > On Tue, Jul 03, 2018 at 01:14:41PM +0100, Mark Rutland wrote: >> On Mon, Jul 02, 2018 at 11:48:05AM -0700, Laura Abbott wrote: >>> On 07/02/2018 06:02 AM, Alexander Popov wrote: >>>> Could you rename the macro to STACKLEAK_ERASE for similarity with x86? >>> >>> Mark Rutland had previously asked for this to be lowercase. >>> I really don't care one way or the other so I'll defer to >>> someone else to have the final word. >> >> Will, Catalin, could you chime in either way? >> >> I'd previously asked for lower-case for consistency with our other >> assembly macros. > > I'd keep it lowercase as the other arm64 macros in this file. Ok, thanks, I'm fine with it. Best regards, Alexander
Hi Kees, On Mon, Jul 02, 2018 at 10:29:24AM -0700, Kees Cook wrote: > On Mon, Jul 2, 2018 at 2:59 AM, Will Deacon <will.deacon@arm.com> wrote: > > On Fri, Jun 29, 2018 at 01:25:20PM -0700, Kees Cook wrote: > >> No worries! I've made the change locally and will push this out to > >> -next unless there are objections? > > > > I'm a bit wary of conflicts in entry.S, since it's likely that we're going > > to have a lot going on in there for 4.19. > > > > Could I take this via arm64 instead, please, or are there dependencies > > on other parts of your tree? > > It depends on the plugin existing, but we could split it up so the > arm64 part could go separately. It would just be a no-op in the arm64 > tree since CONFIG_GCC_PLUGIN_STACKLEAK won't exist there. Whatever > works best for you! If you could split it up then that would be really helpful, please. Thanks, Will
On Fri, Jun 29, 2018 at 12:05 PM, Laura Abbott <labbott@redhat.com> wrote: > include/linux/stackleak.h | 1 + > [...] > diff --git a/include/linux/stackleak.h b/include/linux/stackleak.h > index e2da99b3a191..00d62b302efb 100644 > --- a/include/linux/stackleak.h > +++ b/include/linux/stackleak.h > @@ -5,6 +5,7 @@ > #include <linux/sched.h> > #include <linux/sched/task_stack.h> > > +#include <asm/stacktrace.h> > /* > * Check that the poison value points to the unused hole in the > * virtual memory map for your platform. FYI, I squashed this change into my copy of the stackleak v14 series. (And I just sent this arm64 patch with the adjustments for it to be stand-alone.) -Kees
On Wed, Jul 11, 2018 at 05:05:32PM -0700, Kees Cook wrote: > On Fri, Jun 29, 2018 at 12:05 PM, Laura Abbott <labbott@redhat.com> wrote: > > include/linux/stackleak.h | 1 + > > [...] > > diff --git a/include/linux/stackleak.h b/include/linux/stackleak.h > > index e2da99b3a191..00d62b302efb 100644 > > --- a/include/linux/stackleak.h > > +++ b/include/linux/stackleak.h > > @@ -5,6 +5,7 @@ > > #include <linux/sched.h> > > #include <linux/sched/task_stack.h> > > > > +#include <asm/stacktrace.h> > > /* > > * Check that the poison value points to the unused hole in the > > * virtual memory map for your platform. > > FYI, I squashed this change into my copy of the stackleak v14 series. > (And I just sent this arm64 patch with the adjustments for it to be > stand-alone.) I can't find that -- can you point me to it, please? Will
On 07/03/2018 05:14 AM, Mark Rutland wrote: >> It might be cleaner just to use on_accessible_stack and then another >> function to get the top of stack. This also might just be >> reimplementing what x86 already has? (Mark, Ard?) > It looks like we could build a get_stack_info() as they have. > > We could probably clean up our stack traced atop of that, too. So I spent some time looking at this and I'm not 100% clear if there would actually be much benefit to re-writing with get_stack_info. Most of that design seems to come from x86 needing to handle multiple unwind options which arm64 doesn't need to worry about. Any rework ended up with roughly the same code without any notable benefit that I could see. It's possible I'm missing what kind of cleanup you're suggesting but I think just going with a tweaked version of on_accessible_stack would be fine. Thanks, Laura
Hi, On Tue, Jul 17, 2018 at 03:58:19PM -0700, Laura Abbott wrote: > On 07/03/2018 05:14 AM, Mark Rutland wrote: > > > It might be cleaner just to use on_accessible_stack and then another > > > function to get the top of stack. This also might just be > > > reimplementing what x86 already has? (Mark, Ard?) > > It looks like we could build a get_stack_info() as they have. > > > > We could probably clean up our stack traced atop of that, too. > > So I spent some time looking at this and I'm not 100% clear > if there would actually be much benefit to re-writing with > get_stack_info. Most of that design seems to come from x86 > needing to handle multiple unwind options which arm64 doesn't > need to worry about. Any rework ended up with roughly > the same code without any notable benefit that I could see. > It's possible I'm missing what kind of cleanup you're suggesting > but I think just going with a tweaked version of on_accessible_stack > would be fine. I was mostly thinking that a struct stack_info with stack type enumeration would also be helpful for ensuring that we terminated stack traces when we had a loop. I'll reply on your new thread. Thanks, Mark.
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index eb2cf4938f6d..b0221db95dc9 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -92,6 +92,7 @@ config ARM64 select HAVE_ARCH_MMAP_RND_BITS select HAVE_ARCH_MMAP_RND_COMPAT_BITS if COMPAT select HAVE_ARCH_SECCOMP_FILTER + select HAVE_ARCH_STACKLEAK select HAVE_ARCH_THREAD_STRUCT_WHITELIST select HAVE_ARCH_TRACEHOOK select HAVE_ARCH_TRANSPARENT_HUGEPAGE diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h index 767598932549..73914fd7e142 100644 --- a/arch/arm64/include/asm/processor.h +++ b/arch/arm64/include/asm/processor.h @@ -248,5 +248,14 @@ void cpu_clear_disr(const struct arm64_cpu_capabilities *__unused); #define SVE_SET_VL(arg) sve_set_current_vl(arg) #define SVE_GET_VL() sve_get_current_vl() +/* + * For stackleak + * + * These need to be macros because otherwise we get stuck in a nightmare + * of header definitions for the use of task_stack_page. + */ +#define current_top_of_stack() (task_stack_page(current) + THREAD_SIZE) +#define on_thread_stack() (on_task_stack(current, current_stack_pointer)) + #endif /* __ASSEMBLY__ */ #endif /* __ASM_PROCESSOR_H */ diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S index ec2ee720e33e..31c9da7d401e 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 stackleak_erase +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK + bl stackleak_erase_kstack +#endif + .endm /* * Exception vectors. */ @@ -880,6 +885,7 @@ ret_fast_syscall: and x2, x1, #_TIF_WORK_MASK cbnz x2, work_pending enable_step_tsk x1, x2 + stackleak_erase kernel_exit 0 ret_fast_syscall_trace: enable_daif @@ -906,6 +912,7 @@ ret_to_user: cbnz x2, work_pending finish_ret_to_user: enable_step_tsk x1, x2 + stackleak_erase kernel_exit 0 ENDPROC(ret_to_user) diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c index f08a2ed9db0d..9f0f135f8b66 100644 --- a/arch/arm64/kernel/process.c +++ b/arch/arm64/kernel/process.c @@ -493,3 +493,19 @@ void arch_setup_new_exec(void) { current->mm->context.flags = is_compat_task() ? MMCF_AARCH32 : 0; } + +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK +#define MIN_STACK_LEFT 256 + +void __used stackleak_check_alloca(unsigned long size) +{ + unsigned long sp, stack_left; + + sp = current_stack_pointer; + + stack_left = sp & (THREAD_SIZE - 1); + BUG_ON(stack_left < MIN_STACK_LEFT || + size >= stack_left - MIN_STACK_LEFT); +} +EXPORT_SYMBOL(stackleak_check_alloca); +#endif diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile index 4313f7475333..2fabc2dc1966 100644 --- a/arch/arm64/kvm/hyp/Makefile +++ b/arch/arm64/kvm/hyp/Makefile @@ -3,7 +3,8 @@ # Makefile for Kernel-based Virtual Machine module, HYP part # -ccflags-y += -fno-stack-protector -DDISABLE_BRANCH_PROFILING +ccflags-y += -fno-stack-protector -DDISABLE_BRANCH_PROFILING \ + $(DISABLE_STACKLEAK_PLUGIN) KVM=../../../../virt/kvm diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile index a34e9290a699..25dd2a14560d 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) GCOV_PROFILE := n KASAN_SANITIZE := n diff --git a/include/linux/stackleak.h b/include/linux/stackleak.h index e2da99b3a191..00d62b302efb 100644 --- a/include/linux/stackleak.h +++ b/include/linux/stackleak.h @@ -5,6 +5,7 @@ #include <linux/sched.h> #include <linux/sched/task_stack.h> +#include <asm/stacktrace.h> /* * Check that the poison value points to the unused hole in the * virtual memory map for your platform. diff --git a/scripts/Makefile.gcc-plugins b/scripts/Makefile.gcc-plugins index a535742a1c06..972ce4ca7f6a 100644 --- a/scripts/Makefile.gcc-plugins +++ b/scripts/Makefile.gcc-plugins @@ -37,11 +37,14 @@ ifdef CONFIG_GCC_PLUGINS gcc-plugin-$(CONFIG_GCC_PLUGIN_STACKLEAK) += stackleak_plugin.so gcc-plugin-cflags-$(CONFIG_GCC_PLUGIN_STACKLEAK) += -DSTACKLEAK_PLUGIN -fplugin-arg-stackleak_plugin-track-min-size=$(CONFIG_STACKLEAK_TRACK_MIN_SIZE) + ifdef CONFIG_GCC_PLUGIN_STACKLEAK + DISABLE_STACKLEAK_PLUGIN += -fplugin-arg-stackleak_plugin-disable + endif GCC_PLUGINS_CFLAGS := $(strip $(addprefix -fplugin=$(objtree)/scripts/gcc-plugins/, $(gcc-plugin-y)) $(gcc-plugin-cflags-y)) export PLUGINCC GCC_PLUGINS_CFLAGS GCC_PLUGIN GCC_PLUGIN_SUBDIR - export SANCOV_PLUGIN DISABLE_LATENT_ENTROPY_PLUGIN + export SANCOV_PLUGIN DISABLE_LATENT_ENTROPY_PLUGIN DISABLE_STACKLEAK_PLUGIN ifneq ($(PLUGINCC),) # SANCOV_PLUGIN can be only in CFLAGS_KCOV because avoid duplication.
Implementation of stackleak based heavily on the x86 version Signed-off-by: Laura Abbott <labbott@redhat.com> --- Changes since last time: - Minor name change in entry.S - Converted to use the generic interfaces so there's minimal additions. - Added the fast syscall path. - Addition of on_thread_stack and current_top_of_stack - Disable stackleak on hyp per suggestion - Added a define for check_alloca. I'm still not sure about keeping it since the x86 version got reworked? I've mostly kept this as one patch with a minimal commit text. I can split it up and elaborate more before final merging. --- arch/arm64/Kconfig | 1 + arch/arm64/include/asm/processor.h | 9 +++++++++ arch/arm64/kernel/entry.S | 7 +++++++ arch/arm64/kernel/process.c | 16 ++++++++++++++++ arch/arm64/kvm/hyp/Makefile | 3 ++- drivers/firmware/efi/libstub/Makefile | 3 ++- include/linux/stackleak.h | 1 + scripts/Makefile.gcc-plugins | 5 ++++- 8 files changed, 42 insertions(+), 3 deletions(-)