[PATCH] arm64: Clear the stack

[PATCH] arm64: Clear the stack

[Laura Abbott]

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
+ *
+ * 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.
-- 
2.17.1

Re: [PATCH] arm64: Clear the stack

[Kees Cook]

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

-- 
Kees Cook
Pixel Security

Re: [PATCH] arm64: Clear the stack

[Kees Cook]

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

-- 
Kees Cook
Pixel Security

Re: [PATCH] arm64: Clear the stack

[Laura Abbott]

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

Re: [PATCH] arm64: Clear the stack

[Kees Cook]

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

-- 
Kees Cook
Pixel Security

Re: [PATCH] arm64: Clear the stack

[Will Deacon]

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

Re: [PATCH] arm64: Clear the stack

[Alexander Popov]

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.
> 

Re: [PATCH] arm64: Clear the stack

[Kees Cook]

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

-- 
Kees Cook
Pixel Security

Re: [PATCH] arm64: Clear the stack

[Laura Abbott]

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.
>>
> 

Re: [PATCH] arm64: Clear the stack

[Mark Rutland]

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.

Re: [PATCH] arm64: Clear the stack

[Catalin Marinas]

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.

-- 
Catalin

Re: [PATCH] arm64: Clear the stack

[Alexander Popov]

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

Re: [PATCH] arm64: Clear the stack

[Will Deacon]

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

Re: [PATCH] arm64: Clear the stack

[Kees Cook]

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

-- 
Kees Cook
Pixel Security

Re: [PATCH] arm64: Clear the stack

[Will Deacon]

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

Re: [PATCH] arm64: Clear the stack

[Laura Abbott]

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

Re: [PATCH] arm64: Clear the stack

[Mark Rutland]

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.