[PATCH 0/2] Stackleak for arm64

[PATCH 0/2] Stackleak for arm64

[Laura Abbott]

Hi,

This is the new version of stackleak for arm64 to go with v14 of the
series for x86. I have a cover letter and few more cc's to go along with
some of the prep work.

I also apologize for terrible versioning on these series. Usually I try
to just reply to the top level patch with this addition and not actually
put a version on it. I negelcted to do that last time but as mentioned
this is inteded to go with v14.

Laura Abbott (2):
  arm64: Introduce current_stack_type
  arm64: Clear the stack

 arch/arm64/Kconfig                    |  1 +
 arch/arm64/include/asm/processor.h    | 17 +++++
 arch/arm64/include/asm/sdei.h         |  8 ++-
 arch/arm64/include/asm/stacktrace.h   | 94 ++++++++++++++++++++++-----
 arch/arm64/kernel/entry.S             |  7 ++
 arch/arm64/kernel/process.c           | 32 +++++++++
 arch/arm64/kernel/ptrace.c            |  2 +-
 arch/arm64/kernel/sdei.c              | 21 +++++-
 arch/arm64/kvm/hyp/Makefile           |  3 +-
 drivers/firmware/efi/libstub/Makefile |  3 +-
 include/linux/stackleak.h             |  1 +
 11 files changed, 165 insertions(+), 24 deletions(-)

-- 
2.17.1

[PATCH 1/2] arm64: Introduce current_stack_type

[Laura Abbott]

In preparation for enabling the stackleak plugin on arm64,
we need a way to get the bounds of the current stack.
Introduce a new primitive current_stack_type which is similar
to x86's get_stack_info. Utilize that to rework
on_accessible_stack slightly as well.

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
So this did end up looking quite a bit like get_stack_info but I didn't
really see the need to integrate this more than this. I do think
actually enumerating the types makes things a bit cleaner.
---
 arch/arm64/include/asm/sdei.h       |  8 ++-
 arch/arm64/include/asm/stacktrace.h | 94 ++++++++++++++++++++++++-----
 arch/arm64/kernel/ptrace.c          |  2 +-
 arch/arm64/kernel/sdei.c            | 21 ++++++-
 4 files changed, 103 insertions(+), 22 deletions(-)

diff --git a/arch/arm64/include/asm/sdei.h b/arch/arm64/include/asm/sdei.h
index e073e6886685..34f7b203845b 100644
--- a/arch/arm64/include/asm/sdei.h
+++ b/arch/arm64/include/asm/sdei.h
@@ -40,15 +40,17 @@ asmlinkage unsigned long __sdei_handler(struct pt_regs *regs,
 unsigned long sdei_arch_get_entry_point(int conduit);
 #define sdei_arch_get_entry_point(x)	sdei_arch_get_entry_point(x)
 
-bool _on_sdei_stack(unsigned long sp);
-static inline bool on_sdei_stack(unsigned long sp)
+bool _on_sdei_stack(unsigned long sp, unsigned long *, unsigned long *);
+static inline bool on_sdei_stack(unsigned long sp,
+				unsigned long *stack_low,
+				unsigned long *stack_high)
 {
 	if (!IS_ENABLED(CONFIG_VMAP_STACK))
 		return false;
 	if (!IS_ENABLED(CONFIG_ARM_SDE_INTERFACE))
 		return false;
 	if (in_nmi())
-		return _on_sdei_stack(sp);
+		return _on_sdei_stack(sp, stack_low, stack_high);
 
 	return false;
 }
diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
index 902f9edacbea..9855a0425e64 100644
--- a/arch/arm64/include/asm/stacktrace.h
+++ b/arch/arm64/include/asm/stacktrace.h
@@ -39,7 +39,9 @@ extern void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk);
 
 DECLARE_PER_CPU(unsigned long *, irq_stack_ptr);
 
-static inline bool on_irq_stack(unsigned long sp)
+static inline bool on_irq_stack(unsigned long sp,
+				unsigned long *stack_low,
+				unsigned long *stack_high)
 {
 	unsigned long low = (unsigned long)raw_cpu_read(irq_stack_ptr);
 	unsigned long high = low + IRQ_STACK_SIZE;
@@ -47,47 +49,109 @@ static inline bool on_irq_stack(unsigned long sp)
 	if (!low)
 		return false;
 
-	return (low <= sp && sp < high);
+	if (sp < low || sp >= high)
+		return false;
+
+	if (stack_low && stack_high) {
+		*stack_low = low;
+		*stack_high = high;
+	}
+
+	return true;
 }
 
-static inline bool on_task_stack(struct task_struct *tsk, unsigned long sp)
+static inline bool on_task_stack(struct task_struct *tsk, unsigned long sp,
+				unsigned long *stack_low,
+				unsigned long *stack_high)
 {
 	unsigned long low = (unsigned long)task_stack_page(tsk);
 	unsigned long high = low + THREAD_SIZE;
 
-	return (low <= sp && sp < high);
+	if (sp < low || sp >= high)
+		return false;
+
+	if (stack_low && stack_high) {
+		*stack_low = low;
+		*stack_high = high;
+	}
+
+	return true;
 }
 
 #ifdef CONFIG_VMAP_STACK
 DECLARE_PER_CPU(unsigned long [OVERFLOW_STACK_SIZE/sizeof(long)], overflow_stack);
 
-static inline bool on_overflow_stack(unsigned long sp)
+static inline bool on_overflow_stack(unsigned long sp,
+				unsigned long *stack_low,
+				unsigned long *stack_high)
 {
 	unsigned long low = (unsigned long)raw_cpu_ptr(overflow_stack);
 	unsigned long high = low + OVERFLOW_STACK_SIZE;
 
-	return (low <= sp && sp < high);
+	if (sp < low || sp >= high)
+		return false;
+
+	if (stack_low && stack_high) {
+		*stack_low = low;
+		*stack_high = high;
+	}
+
+	return true;
 }
 #else
-static inline bool on_overflow_stack(unsigned long sp) { return false; }
+static inline bool on_overflow_stack(unsigned long sp,
+			unsigned long *stack_low,
+			unsigned long *stack_high) { return false; }
 #endif
 
+enum stack_type {
+	STACK_TYPE_UNKNOWN,
+	STACK_TYPE_TASK,
+	STACK_TYPE_IRQ,
+	STACK_TYPE_OVERFLOW,
+	STACK_TYPE_SDEI,
+};
+
+static inline enum stack_type current_stack_type(struct task_struct *tsk,
+						unsigned long sp,
+						unsigned long *stack_low,
+						unsigned long *stack_high)
+{
+	if (on_task_stack(tsk, sp, stack_low, stack_high))
+		return STACK_TYPE_TASK;
+	if (on_irq_stack(sp, stack_low, stack_high))
+		return STACK_TYPE_IRQ;
+	if (on_overflow_stack(sp, stack_low, stack_high))
+		return STACK_TYPE_OVERFLOW;
+	if (on_sdei_stack(sp, stack_low, stack_high))
+		return STACK_TYPE_SDEI;
+	return STACK_TYPE_UNKNOWN;
+}
+
 /*
  * We can only safely access per-cpu stacks from current in a non-preemptible
  * context.
  */
 static inline bool on_accessible_stack(struct task_struct *tsk, unsigned long sp)
 {
-	if (on_task_stack(tsk, sp))
+	enum stack_type type;
+	unsigned long low, high;
+
+	type = current_stack_type(tsk, sp, &low, &high);
+
+	switch (type) {
+	case STACK_TYPE_TASK:
 		return true;
-	if (tsk != current || preemptible())
+	case STACK_TYPE_IRQ:
+	case STACK_TYPE_OVERFLOW:
+	case STACK_TYPE_SDEI:
+		if (tsk != current || preemptible())
+			return false;
+		else
+			return true;
+	case STACK_TYPE_UNKNOWN:
 		return false;
-	if (on_irq_stack(sp))
-		return true;
-	if (on_overflow_stack(sp))
-		return true;
-	if (on_sdei_stack(sp))
-		return true;
+	}
 
 	return false;
 }
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 5c338ce5a7fa..a6b3a2be7561 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -132,7 +132,7 @@ static bool regs_within_kernel_stack(struct pt_regs *regs, unsigned long addr)
 {
 	return ((addr & ~(THREAD_SIZE - 1))  ==
 		(kernel_stack_pointer(regs) & ~(THREAD_SIZE - 1))) ||
-		on_irq_stack(addr);
+		on_irq_stack(addr, NULL, NULL);
 }
 
 /**
diff --git a/arch/arm64/kernel/sdei.c b/arch/arm64/kernel/sdei.c
index 6b8d90d5ceae..8e18913a53fd 100644
--- a/arch/arm64/kernel/sdei.c
+++ b/arch/arm64/kernel/sdei.c
@@ -88,7 +88,9 @@ static int init_sdei_stacks(void)
 	return err;
 }
 
-bool _on_sdei_stack(unsigned long sp)
+bool _on_sdei_stack(unsigned long sp,
+		unsigned long *stack_low,
+		unsigned long *stack_high)
 {
 	unsigned long low, high;
 
@@ -98,13 +100,26 @@ bool _on_sdei_stack(unsigned long sp)
 	low = (unsigned long)raw_cpu_read(sdei_stack_critical_ptr);
 	high = low + SDEI_STACK_SIZE;
 
-	if (low <= sp && sp < high)
+	if (low <= sp && sp < high) {
+		if (stack_low && stack_high) {
+			*stack_low = low;
+			*stack_high = high;
+		}
 		return true;
+	}
 
 	low = (unsigned long)raw_cpu_read(sdei_stack_normal_ptr);
 	high = low + SDEI_STACK_SIZE;
 
-	return (low <= sp && sp < high);
+	if (low <= sp && sp < high) {
+		if (stack_low && stack_high) {
+			*stack_low = low;
+			*stack_high = high;
+		}
+		return true;
+	}
+
+	return false;
 }
 
 unsigned long sdei_arch_get_entry_point(int conduit)
-- 
2.17.1

Re: [PATCH 1/2] arm64: Introduce current_stack_type

[Mark Rutland]

Hi Laura,

On Wed, Jul 18, 2018 at 02:10:12PM -0700, Laura Abbott wrote:
> 
> In preparation for enabling the stackleak plugin on arm64,
> we need a way to get the bounds of the current stack.
> Introduce a new primitive current_stack_type which is similar
> to x86's get_stack_info. Utilize that to rework
> on_accessible_stack slightly as well.
> 
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> So this did end up looking quite a bit like get_stack_info but I didn't
> really see the need to integrate this more than this. I do think
> actually enumerating the types makes things a bit cleaner.
> ---
>  arch/arm64/include/asm/sdei.h       |  8 ++-
>  arch/arm64/include/asm/stacktrace.h | 94 ++++++++++++++++++++++++-----
>  arch/arm64/kernel/ptrace.c          |  2 +-
>  arch/arm64/kernel/sdei.c            | 21 ++++++-
>  4 files changed, 103 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/sdei.h b/arch/arm64/include/asm/sdei.h
> index e073e6886685..34f7b203845b 100644
> --- a/arch/arm64/include/asm/sdei.h
> +++ b/arch/arm64/include/asm/sdei.h
> @@ -40,15 +40,17 @@ asmlinkage unsigned long __sdei_handler(struct pt_regs *regs,
>  unsigned long sdei_arch_get_entry_point(int conduit);
>  #define sdei_arch_get_entry_point(x)	sdei_arch_get_entry_point(x)
>  
> -bool _on_sdei_stack(unsigned long sp);
> -static inline bool on_sdei_stack(unsigned long sp)
> +bool _on_sdei_stack(unsigned long sp, unsigned long *, unsigned long *);
> +static inline bool on_sdei_stack(unsigned long sp,
> +				unsigned long *stack_low,
> +				unsigned long *stack_high)
>  {
>  	if (!IS_ENABLED(CONFIG_VMAP_STACK))
>  		return false;
>  	if (!IS_ENABLED(CONFIG_ARM_SDE_INTERFACE))
>  		return false;
>  	if (in_nmi())
> -		return _on_sdei_stack(sp);
> +		return _on_sdei_stack(sp, stack_low, stack_high);
>  
>  	return false;
>  }
> diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
> index 902f9edacbea..9855a0425e64 100644
> --- a/arch/arm64/include/asm/stacktrace.h
> +++ b/arch/arm64/include/asm/stacktrace.h
> @@ -39,7 +39,9 @@ extern void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk);
>  
>  DECLARE_PER_CPU(unsigned long *, irq_stack_ptr);
>  
> -static inline bool on_irq_stack(unsigned long sp)
> +static inline bool on_irq_stack(unsigned long sp,
> +				unsigned long *stack_low,
> +				unsigned long *stack_high)
>  {
>  	unsigned long low = (unsigned long)raw_cpu_read(irq_stack_ptr);
>  	unsigned long high = low + IRQ_STACK_SIZE;
> @@ -47,47 +49,109 @@ static inline bool on_irq_stack(unsigned long sp)
>  	if (!low)
>  		return false;
>  
> -	return (low <= sp && sp < high);
> +	if (sp < low || sp >= high)
> +		return false;
> +
> +	if (stack_low && stack_high) {
> +		*stack_low = low;
> +		*stack_high = high;
> +	}
> +
> +	return true;
>  }

Rather than having to pass separete pointers to low/high, could we
please wrap them up as a struct, e.g.

struct stack_info {
	unsigned long low, high;
	stack_type type;
}

... and pass a single pointer to that? e.g.

static inline bool on_irq_stack(unsigned long sp, struct stack_info *info)
{
	unsigned long low = (unsigned long)raw_cpu_read(irq_stack_ptr);
	unsigned long high = low + IRQ_STACK_SIZE;

	if (!low)
		return false;
	
	if (sp < low || sp >= high)
		return false;
	
	if (info) {
		info->low = low;
		info->high = high;
		info->type = STACK_TYPE_IRQ;
	}

	return true;
}

That simplified the prototypes, and will allow us to distinguish the two
SDEI stacks (which we'll need for making stack unwiding robust to
cross-stack loops).

> -static inline bool on_task_stack(struct task_struct *tsk, unsigned long sp)
> +static inline bool on_task_stack(struct task_struct *tsk, unsigned long sp,
> +				unsigned long *stack_low,
> +				unsigned long *stack_high)
>  {
>  	unsigned long low = (unsigned long)task_stack_page(tsk);
>  	unsigned long high = low + THREAD_SIZE;
>  
> -	return (low <= sp && sp < high);
> +	if (sp < low || sp >= high)
> +		return false;
> +
> +	if (stack_low && stack_high) {
> +		*stack_low = low;
> +		*stack_high = high;
> +	}
> +
> +	return true;
>  }
>  
>  #ifdef CONFIG_VMAP_STACK
>  DECLARE_PER_CPU(unsigned long [OVERFLOW_STACK_SIZE/sizeof(long)], overflow_stack);
>  
> -static inline bool on_overflow_stack(unsigned long sp)
> +static inline bool on_overflow_stack(unsigned long sp,
> +				unsigned long *stack_low,
> +				unsigned long *stack_high)
>  {
>  	unsigned long low = (unsigned long)raw_cpu_ptr(overflow_stack);
>  	unsigned long high = low + OVERFLOW_STACK_SIZE;
>  
> -	return (low <= sp && sp < high);
> +	if (sp < low || sp >= high)
> +		return false;
> +
> +	if (stack_low && stack_high) {
> +		*stack_low = low;
> +		*stack_high = high;
> +	}
> +
> +	return true;
>  }
>  #else
> -static inline bool on_overflow_stack(unsigned long sp) { return false; }
> +static inline bool on_overflow_stack(unsigned long sp,
> +			unsigned long *stack_low,
> +			unsigned long *stack_high) { return false; }
>  #endif
>  
> +enum stack_type {
> +	STACK_TYPE_UNKNOWN,
> +	STACK_TYPE_TASK,
> +	STACK_TYPE_IRQ,
> +	STACK_TYPE_OVERFLOW,
> +	STACK_TYPE_SDEI,
> +};

For SDEI we'll need STACK_TYPE_SDEI_NORMAL and STACK_TYPE_SDEI_CRITICAL,
at least for stack unwinding.

> +
> +static inline enum stack_type current_stack_type(struct task_struct *tsk,
> +						unsigned long sp,
> +						unsigned long *stack_low,
> +						unsigned long *stack_high)
> +{
> +	if (on_task_stack(tsk, sp, stack_low, stack_high))
> +		return STACK_TYPE_TASK;
> +	if (on_irq_stack(sp, stack_low, stack_high))
> +		return STACK_TYPE_IRQ;
> +	if (on_overflow_stack(sp, stack_low, stack_high))
> +		return STACK_TYPE_OVERFLOW;
> +	if (on_sdei_stack(sp, stack_low, stack_high))
> +		return STACK_TYPE_SDEI;
> +	return STACK_TYPE_UNKNOWN;
> +}
> +
>  /*
>   * We can only safely access per-cpu stacks from current in a non-preemptible
>   * context.
>   */
>  static inline bool on_accessible_stack(struct task_struct *tsk, unsigned long sp)
>  {
> -	if (on_task_stack(tsk, sp))
> +	enum stack_type type;
> +	unsigned long low, high;
> +
> +	type = current_stack_type(tsk, sp, &low, &high);
> +
> +	switch (type) {
> +	case STACK_TYPE_TASK:
>  		return true;
> -	if (tsk != current || preemptible())
> +	case STACK_TYPE_IRQ:
> +	case STACK_TYPE_OVERFLOW:
> +	case STACK_TYPE_SDEI:
> +		if (tsk != current || preemptible())
> +			return false;
> +		else
> +			return true;
> +	case STACK_TYPE_UNKNOWN:
>  		return false;
> -	if (on_irq_stack(sp))
> -		return true;
> -	if (on_overflow_stack(sp))
> -		return true;
> -	if (on_sdei_stack(sp))
> -		return true;
> +	}
>  
>  	return false;
>  }

With the stacut stack_info, I think we can leave the logic of
on_accessible_stack() as-is, modulo a new info parameter that we pass on
into each on_<foo>_stack(), and then we don't neeed a separate
current_stack_type() function.

> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> index 5c338ce5a7fa..a6b3a2be7561 100644
> --- a/arch/arm64/kernel/ptrace.c
> +++ b/arch/arm64/kernel/ptrace.c
> @@ -132,7 +132,7 @@ static bool regs_within_kernel_stack(struct pt_regs *regs, unsigned long addr)
>  {
>  	return ((addr & ~(THREAD_SIZE - 1))  ==
>  		(kernel_stack_pointer(regs) & ~(THREAD_SIZE - 1))) ||
> -		on_irq_stack(addr);
> +		on_irq_stack(addr, NULL, NULL);
>  }
>  
>  /**
> diff --git a/arch/arm64/kernel/sdei.c b/arch/arm64/kernel/sdei.c
> index 6b8d90d5ceae..8e18913a53fd 100644
> --- a/arch/arm64/kernel/sdei.c
> +++ b/arch/arm64/kernel/sdei.c
> @@ -88,7 +88,9 @@ static int init_sdei_stacks(void)
>  	return err;
>  }
>  
> -bool _on_sdei_stack(unsigned long sp)
> +bool _on_sdei_stack(unsigned long sp,
> +		unsigned long *stack_low,
> +		unsigned long *stack_high)
>  {
>  	unsigned long low, high;
>  
> @@ -98,13 +100,26 @@ bool _on_sdei_stack(unsigned long sp)
>  	low = (unsigned long)raw_cpu_read(sdei_stack_critical_ptr);
>  	high = low + SDEI_STACK_SIZE;
>  
> -	if (low <= sp && sp < high)
> +	if (low <= sp && sp < high) {
> +		if (stack_low && stack_high) {
> +			*stack_low = low;
> +			*stack_high = high;
> +		}
>  		return true;
> +	}
>  
>  	low = (unsigned long)raw_cpu_read(sdei_stack_normal_ptr);
>  	high = low + SDEI_STACK_SIZE;
>  
> -	return (low <= sp && sp < high);
> +	if (low <= sp && sp < high) {
> +		if (stack_low && stack_high) {
> +			*stack_low = low;
> +			*stack_high = high;
> +		}
> +		return true;
> +	}
> +
> +	return false;
>  }

We should probably split this into separate on_sdei_normal_stack() and
on_sdei_critical_stack() functions.

Then we can do:

bool on_sdei_<foo>_stack(...)
{
	if (<out of bounds>)
		return false;
	
	if (info) {
		<assign info fields>;
	}

	return true;
}

bool _on_sdei_stack(unsigned long sp, struct stack_info *info)
{
	if (on_sedi_critical_stack(sp, info))
		return true;
	if (on_sdei_normal_stack(sp, info))
		return true;
	
	return false;
}

... which is a little nicer for legibility.

Otherwise, this looks good to me.

Thanks,
Mark.

[PATCH 2/2] arm64: Clear the stack

[Laura Abbott]

Implementation of stackleak based heavily on the x86 version

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
Since last time: Minor style cleanups. Re-wrote check_alloca to
correctly handle all stack types. While doing that, I also realized
current_top_of_stack was incorrect so I fixed that as well.
---
 arch/arm64/Kconfig                    |  1 +
 arch/arm64/include/asm/processor.h    | 17 ++++++++++++++
 arch/arm64/kernel/entry.S             |  7 ++++++
 arch/arm64/kernel/process.c           | 32 +++++++++++++++++++++++++++
 arch/arm64/kvm/hyp/Makefile           |  3 ++-
 drivers/firmware/efi/libstub/Makefile |  3 ++-
 include/linux/stackleak.h             |  1 +
 7 files changed, 62 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 42c090cf0292..216d36a49ab5 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -96,6 +96,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 a73ae1e49200..4f3062ee22c6 100644
--- a/arch/arm64/include/asm/processor.h
+++ b/arch/arm64/include/asm/processor.h
@@ -266,5 +266,22 @@ extern void __init minsigstksz_setup(void);
 #define SVE_SET_VL(arg)	sve_set_current_vl(arg)
 #define SVE_GET_VL()	sve_get_current_vl()
 
+/*
+ * For CONFIG_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()		\
+({					\
+	unsigned long _low = 0;		\
+	unsigned long _high = 0;	\
+					\
+	current_stack_type(current, current_stack_pointer, &_low, &_high); \
+	_high;	\
+})
+#define on_thread_stack()	(on_task_stack(current, current_stack_pointer, NULL, NULL))
+
 #endif /* __ASSEMBLY__ */
 #endif /* __ASM_PROCESSOR_H */
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 28ad8799406f..67d12016063d 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -431,6 +431,11 @@ tsk	.req	x28		// current thread_info
 
 	.text
 
+	.macro	stackleak_erase
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+	bl	stackleak_erase
+#endif
+	.endm
 /*
  * Exception vectors.
  */
@@ -910,6 +915,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
@@ -936,6 +942,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 e10bc363f533..904defa36689 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -493,3 +493,35 @@ void arch_setup_new_exec(void)
 {
 	current->mm->context.flags = is_compat_task() ? MMCF_AARCH32 : 0;
 }
+
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+void __used stackleak_check_alloca(unsigned long size)
+{
+	unsigned long stack_left;
+	enum stack_type type;
+	unsigned long current_sp = current_stack_pointer;
+	unsigned long low, high;
+
+	type = current_stack_type(current, current_sp, &low, &high);
+	BUG_ON(type == STACK_TYPE_UNKNOWN);
+
+	stack_left = current_sp - low;
+
+	if (size >= stack_left) {
+		/*
+		 * Kernel stack depth overflow is detected, let's report that.
+		 * If CONFIG_VMAP_STACK is enabled, we can safely use BUG().
+		 * If CONFIG_VMAP_STACK is disabled, BUG() handling can corrupt
+		 * the neighbour memory. CONFIG_SCHED_STACK_END_CHECK calls
+		 * panic() in a similar situation, so let's do the same if that
+		 * option is on. Otherwise just use BUG() and hope for the best.
+		 */
+#if !defined(CONFIG_VMAP_STACK) && defined(CONFIG_SCHED_STACK_END_CHECK)
+		panic("alloca() over the kernel stack boundary\n");
+#else
+		BUG();
+#endif
+	}
+}
+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 b911b973d328..08420ec6b7c3 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.
-- 
2.17.1

Re: [PATCH 2/2] arm64: Clear the stack

[Kees Cook]

On Wed, Jul 18, 2018 at 2:10 PM, Laura Abbott <labbott@redhat.com> wrote:
>
> Implementation of stackleak based heavily on the x86 version
>
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> Since last time: Minor style cleanups. Re-wrote check_alloca to
> correctly handle all stack types. While doing that, I also realized
> current_top_of_stack was incorrect so I fixed that as well.

Can you drop the include/linux/stackleak.h change from this series?
I've included that in the v14 in linux-next already, so that these
patches can land entirely separately in the arm64 tree (which was the
request).

Otherwise, this looks great!

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  arch/arm64/Kconfig                    |  1 +
>  arch/arm64/include/asm/processor.h    | 17 ++++++++++++++
>  arch/arm64/kernel/entry.S             |  7 ++++++
>  arch/arm64/kernel/process.c           | 32 +++++++++++++++++++++++++++
>  arch/arm64/kvm/hyp/Makefile           |  3 ++-
>  drivers/firmware/efi/libstub/Makefile |  3 ++-
>  include/linux/stackleak.h             |  1 +
>  7 files changed, 62 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 42c090cf0292..216d36a49ab5 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -96,6 +96,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 a73ae1e49200..4f3062ee22c6 100644
> --- a/arch/arm64/include/asm/processor.h
> +++ b/arch/arm64/include/asm/processor.h
> @@ -266,5 +266,22 @@ extern void __init minsigstksz_setup(void);
>  #define SVE_SET_VL(arg)        sve_set_current_vl(arg)
>  #define SVE_GET_VL()   sve_get_current_vl()
>
> +/*
> + * For CONFIG_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()         \
> +({                                     \
> +       unsigned long _low = 0;         \
> +       unsigned long _high = 0;        \
> +                                       \
> +       current_stack_type(current, current_stack_pointer, &_low, &_high); \
> +       _high;  \
> +})
> +#define on_thread_stack()      (on_task_stack(current, current_stack_pointer, NULL, NULL))
> +
>  #endif /* __ASSEMBLY__ */
>  #endif /* __ASM_PROCESSOR_H */
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 28ad8799406f..67d12016063d 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -431,6 +431,11 @@ tsk        .req    x28             // current thread_info
>
>         .text
>
> +       .macro  stackleak_erase
> +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
> +       bl      stackleak_erase
> +#endif
> +       .endm
>  /*
>   * Exception vectors.
>   */
> @@ -910,6 +915,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
> @@ -936,6 +942,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 e10bc363f533..904defa36689 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -493,3 +493,35 @@ void arch_setup_new_exec(void)
>  {
>         current->mm->context.flags = is_compat_task() ? MMCF_AARCH32 : 0;
>  }
> +
> +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
> +void __used stackleak_check_alloca(unsigned long size)
> +{
> +       unsigned long stack_left;
> +       enum stack_type type;
> +       unsigned long current_sp = current_stack_pointer;
> +       unsigned long low, high;
> +
> +       type = current_stack_type(current, current_sp, &low, &high);
> +       BUG_ON(type == STACK_TYPE_UNKNOWN);
> +
> +       stack_left = current_sp - low;
> +
> +       if (size >= stack_left) {
> +               /*
> +                * Kernel stack depth overflow is detected, let's report that.
> +                * If CONFIG_VMAP_STACK is enabled, we can safely use BUG().
> +                * If CONFIG_VMAP_STACK is disabled, BUG() handling can corrupt
> +                * the neighbour memory. CONFIG_SCHED_STACK_END_CHECK calls
> +                * panic() in a similar situation, so let's do the same if that
> +                * option is on. Otherwise just use BUG() and hope for the best.
> +                */
> +#if !defined(CONFIG_VMAP_STACK) && defined(CONFIG_SCHED_STACK_END_CHECK)
> +               panic("alloca() over the kernel stack boundary\n");
> +#else
> +               BUG();
> +#endif
> +       }
> +}
> +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 b911b973d328..08420ec6b7c3 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.
> --
> 2.17.1
>



-- 
Kees Cook
Pixel Security

Re: [PATCH 2/2] arm64: Clear the stack

[Alexander Popov]

Hello Laura,

Thanks again for your work.
Please see some comments below.

On 19.07.2018 00:10, Laura Abbott wrote:
> Implementation of stackleak based heavily on the x86 version
> 
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> Since last time: Minor style cleanups. Re-wrote check_alloca to
> correctly handle all stack types. While doing that, I also realized
> current_top_of_stack was incorrect so I fixed that as well.
> ---
>  arch/arm64/Kconfig                    |  1 +
>  arch/arm64/include/asm/processor.h    | 17 ++++++++++++++
>  arch/arm64/kernel/entry.S             |  7 ++++++
>  arch/arm64/kernel/process.c           | 32 +++++++++++++++++++++++++++
>  arch/arm64/kvm/hyp/Makefile           |  3 ++-
>  drivers/firmware/efi/libstub/Makefile |  3 ++-
>  include/linux/stackleak.h             |  1 +
>  7 files changed, 62 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 42c090cf0292..216d36a49ab5 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -96,6 +96,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 a73ae1e49200..4f3062ee22c6 100644
> --- a/arch/arm64/include/asm/processor.h
> +++ b/arch/arm64/include/asm/processor.h
> @@ -266,5 +266,22 @@ extern void __init minsigstksz_setup(void);
>  #define SVE_SET_VL(arg)	sve_set_current_vl(arg)
>  #define SVE_GET_VL()	sve_get_current_vl()
>  
> +/*
> + * For CONFIG_STACKLEAK

Our config option is called CONFIG_GCC_PLUGIN_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()		\
> +({					\
> +	unsigned long _low = 0;		\
> +	unsigned long _high = 0;	\
> +					\
> +	current_stack_type(current, current_stack_pointer, &_low, &_high); \
> +	_high;	\
> +})

Do you really need _low here? Ah, I see this in the previous patch:
+	if (stack_low && stack_high) {
+		*stack_low = low;
+		*stack_high = high;
+	}

How about checking them against NULL separately? That would allow
+	current_stack_type(current, current_stack_pointer, NULL, &_high);

Also a minor comment - how about aligning backslashes?

> +#define on_thread_stack()	(on_task_stack(current, current_stack_pointer, NULL, NULL))
> +
>  #endif /* __ASSEMBLY__ */
>  #endif /* __ASM_PROCESSOR_H */
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 28ad8799406f..67d12016063d 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -431,6 +431,11 @@ tsk	.req	x28		// current thread_info
>  
>  	.text
>  
> +	.macro	stackleak_erase
> +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
> +	bl	stackleak_erase
> +#endif
> +	.endm
>  /*
>   * Exception vectors.
>   */
> @@ -910,6 +915,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
> @@ -936,6 +942,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 e10bc363f533..904defa36689 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -493,3 +493,35 @@ void arch_setup_new_exec(void)
>  {
>  	current->mm->context.flags = is_compat_task() ? MMCF_AARCH32 : 0;
>  }
> +
> +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
> +void __used stackleak_check_alloca(unsigned long size)
> +{
> +	unsigned long stack_left;
> +	enum stack_type type;
> +	unsigned long current_sp = current_stack_pointer;
> +	unsigned long low, high;
> +
> +	type = current_stack_type(current, current_sp, &low, &high);
> +	BUG_ON(type == STACK_TYPE_UNKNOWN);
> +
> +	stack_left = current_sp - low;
> +
> +	if (size >= stack_left) {
> +		/*
> +		 * Kernel stack depth overflow is detected, let's report that.
> +		 * If CONFIG_VMAP_STACK is enabled, we can safely use BUG().
> +		 * If CONFIG_VMAP_STACK is disabled, BUG() handling can corrupt
> +		 * the neighbour memory. CONFIG_SCHED_STACK_END_CHECK calls
> +		 * panic() in a similar situation, so let's do the same if that
> +		 * option is on. Otherwise just use BUG() and hope for the best.
> +		 */
> +#if !defined(CONFIG_VMAP_STACK) && defined(CONFIG_SCHED_STACK_END_CHECK)
> +		panic("alloca() over the kernel stack boundary\n");
> +#else
> +		BUG();
> +#endif

This comment and #if logic should be dropped, we should always use panic() here
on arm64. Mark Rutland and I have worked out the solution for arm64 in this thread:
http://openwall.com/lists/kernel-hardening/2018/05/11/12

Rationale: on arm64 with CONFIG_VMAP_STACK, a stack overflow results in panic()
anyway.

> +	}
> +}
> +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 b911b973d328..08420ec6b7c3 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.
> 

Re: [PATCH 2/2] arm64: Clear the stack

[Mark Rutland]

On Wed, Jul 18, 2018 at 02:10:13PM -0700, Laura Abbott wrote:
> 
> Implementation of stackleak based heavily on the x86 version
> 
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> Since last time: Minor style cleanups. Re-wrote check_alloca to
> correctly handle all stack types. While doing that, I also realized
> current_top_of_stack was incorrect so I fixed that as well.
> ---
>  arch/arm64/Kconfig                    |  1 +
>  arch/arm64/include/asm/processor.h    | 17 ++++++++++++++
>  arch/arm64/kernel/entry.S             |  7 ++++++
>  arch/arm64/kernel/process.c           | 32 +++++++++++++++++++++++++++
>  arch/arm64/kvm/hyp/Makefile           |  3 ++-
>  drivers/firmware/efi/libstub/Makefile |  3 ++-
>  include/linux/stackleak.h             |  1 +
>  7 files changed, 62 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 42c090cf0292..216d36a49ab5 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -96,6 +96,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 a73ae1e49200..4f3062ee22c6 100644
> --- a/arch/arm64/include/asm/processor.h
> +++ b/arch/arm64/include/asm/processor.h
> @@ -266,5 +266,22 @@ extern void __init minsigstksz_setup(void);
>  #define SVE_SET_VL(arg)	sve_set_current_vl(arg)
>  #define SVE_GET_VL()	sve_get_current_vl()
>  
> +/*
> + * For CONFIG_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()		\
> +({					\
> +	unsigned long _low = 0;		\
> +	unsigned long _high = 0;	\
> +					\
> +	current_stack_type(current, current_stack_pointer, &_low, &_high); \
> +	_high;	\
> +})

... with the info changes, this could be:

#define current_top_of_stack()						\
({									\
	struct stack_info _info;					\
	BUG_ON(!on_accessible_stack(current_stack_pinter, &_info));	\
	_info->high;							\
})

> +#define on_thread_stack()	(on_task_stack(current, current_stack_pointer, NULL, NULL))

... and one fewer NULL here.

> +
>  #endif /* __ASSEMBLY__ */
>  #endif /* __ASM_PROCESSOR_H */
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 28ad8799406f..67d12016063d 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -431,6 +431,11 @@ tsk	.req	x28		// current thread_info
>  
>  	.text
>  
> +	.macro	stackleak_erase
> +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
> +	bl	stackleak_erase
> +#endif
> +	.endm
>  /*
>   * Exception vectors.
>   */
> @@ -910,6 +915,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
> @@ -936,6 +942,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 e10bc363f533..904defa36689 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -493,3 +493,35 @@ void arch_setup_new_exec(void)
>  {
>  	current->mm->context.flags = is_compat_task() ? MMCF_AARCH32 : 0;
>  }
> +
> +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
> +void __used stackleak_check_alloca(unsigned long size)
> +{
> +	unsigned long stack_left;
> +	enum stack_type type;
> +	unsigned long current_sp = current_stack_pointer;
> +	unsigned long low, high;
> +
> +	type = current_stack_type(current, current_sp, &low, &high);
> +	BUG_ON(type == STACK_TYPE_UNKNOWN);
> +
> +	stack_left = current_sp - low;

... similarly with the info changes, this could be:

	unsigned long current_sp = current_stack_pointer;
	unsigned long stack_left;
	struct stack_info info;

	BUG_ON(!on_accessible_stack(current, current_sp, &info));

	stack_lead = current_sp - info->low;

Otherwise, this looks good to me.

Thanks,
Mark.

> +
> +	if (size >= stack_left) {
> +		/*
> +		 * Kernel stack depth overflow is detected, let's report that.
> +		 * If CONFIG_VMAP_STACK is enabled, we can safely use BUG().
> +		 * If CONFIG_VMAP_STACK is disabled, BUG() handling can corrupt
> +		 * the neighbour memory. CONFIG_SCHED_STACK_END_CHECK calls
> +		 * panic() in a similar situation, so let's do the same if that
> +		 * option is on. Otherwise just use BUG() and hope for the best.
> +		 */
> +#if !defined(CONFIG_VMAP_STACK) && defined(CONFIG_SCHED_STACK_END_CHECK)
> +		panic("alloca() over the kernel stack boundary\n");
> +#else
> +		BUG();
> +#endif
> +	}
> +}
> +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 b911b973d328..08420ec6b7c3 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.
> -- 
> 2.17.1
> 

[PATCHv2 0/2] Stackleak for arm64

[Laura Abbott]

Hi,

This is hopefully the final version of the stackleak plugin for arm64.
Acks are appreciated!

Laura Abbott (2):
  arm64: Add stack information to on_accessible_stack
  arm64: Clear the stack

 arch/arm64/Kconfig                    |  1 +
 arch/arm64/include/asm/processor.h    | 15 ++++++
 arch/arm64/include/asm/sdei.h         |  9 ++--
 arch/arm64/include/asm/stacktrace.h   | 73 ++++++++++++++++++++++-----
 arch/arm64/kernel/entry.S             |  7 +++
 arch/arm64/kernel/process.c           | 17 +++++++
 arch/arm64/kernel/ptrace.c            |  2 +-
 arch/arm64/kernel/sdei.c              | 51 ++++++++++++++++---
 arch/arm64/kernel/stacktrace.c        |  2 +-
 arch/arm64/kvm/hyp/Makefile           |  3 +-
 drivers/firmware/efi/libstub/Makefile |  3 +-
 11 files changed, 156 insertions(+), 27 deletions(-)

-- 
2.17.1

[PATCHv2 1/2] arm64: Add stack information to on_accessible_stack

[Laura Abbott]

In preparation for enabling the stackleak plugin on arm64,
we need a way to get the bounds of the current stack. Extend
on_accessible_stack to get this information.

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
v2: Switched to using struct stack_info for argument passing.
on_accessible_stack is now the primary API. Split STACK_TYPE_SDEI
into STACK_TYPE_SDEI_NORMAL and STACK_TYPE_SDEI_CRITICAL.
---
 arch/arm64/include/asm/sdei.h       |  9 ++--
 arch/arm64/include/asm/stacktrace.h | 73 ++++++++++++++++++++++++-----
 arch/arm64/kernel/ptrace.c          |  2 +-
 arch/arm64/kernel/sdei.c            | 51 ++++++++++++++++----
 arch/arm64/kernel/stacktrace.c      |  2 +-
 5 files changed, 112 insertions(+), 25 deletions(-)

diff --git a/arch/arm64/include/asm/sdei.h b/arch/arm64/include/asm/sdei.h
index e073e6886685..ffe47d766c25 100644
--- a/arch/arm64/include/asm/sdei.h
+++ b/arch/arm64/include/asm/sdei.h
@@ -40,15 +40,18 @@ asmlinkage unsigned long __sdei_handler(struct pt_regs *regs,
 unsigned long sdei_arch_get_entry_point(int conduit);
 #define sdei_arch_get_entry_point(x)	sdei_arch_get_entry_point(x)
 
-bool _on_sdei_stack(unsigned long sp);
-static inline bool on_sdei_stack(unsigned long sp)
+struct stack_info;
+
+bool _on_sdei_stack(unsigned long sp, struct stack_info *info);
+static inline bool on_sdei_stack(unsigned long sp,
+				struct stack_info *info)
 {
 	if (!IS_ENABLED(CONFIG_VMAP_STACK))
 		return false;
 	if (!IS_ENABLED(CONFIG_ARM_SDE_INTERFACE))
 		return false;
 	if (in_nmi())
-		return _on_sdei_stack(sp);
+		return _on_sdei_stack(sp, info);
 
 	return false;
 }
diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
index 902f9edacbea..e86737b7c924 100644
--- a/arch/arm64/include/asm/stacktrace.h
+++ b/arch/arm64/include/asm/stacktrace.h
@@ -32,6 +32,21 @@ struct stackframe {
 #endif
 };
 
+enum stack_type {
+	STACK_TYPE_UNKNOWN,
+	STACK_TYPE_TASK,
+	STACK_TYPE_IRQ,
+	STACK_TYPE_OVERFLOW,
+	STACK_TYPE_SDEI_NORMAL,
+	STACK_TYPE_SDEI_CRITICAL,
+};
+
+struct stack_info {
+	unsigned long low;
+	unsigned long high;
+	enum stack_type type;
+};
+
 extern int unwind_frame(struct task_struct *tsk, struct stackframe *frame);
 extern void walk_stackframe(struct task_struct *tsk, struct stackframe *frame,
 			    int (*fn)(struct stackframe *, void *), void *data);
@@ -39,7 +54,8 @@ extern void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk);
 
 DECLARE_PER_CPU(unsigned long *, irq_stack_ptr);
 
-static inline bool on_irq_stack(unsigned long sp)
+static inline bool on_irq_stack(unsigned long sp,
+				struct stack_info *info)
 {
 	unsigned long low = (unsigned long)raw_cpu_read(irq_stack_ptr);
 	unsigned long high = low + IRQ_STACK_SIZE;
@@ -47,46 +63,79 @@ static inline bool on_irq_stack(unsigned long sp)
 	if (!low)
 		return false;
 
-	return (low <= sp && sp < high);
+	if (sp < low || sp >= high)
+		return false;
+
+	if (info) {
+		info->low = low;
+		info->high = high;
+		info->type = STACK_TYPE_IRQ;
+	}
+
+	return true;
 }
 
-static inline bool on_task_stack(struct task_struct *tsk, unsigned long sp)
+static inline bool on_task_stack(struct task_struct *tsk, unsigned long sp,
+				struct stack_info *info)
 {
 	unsigned long low = (unsigned long)task_stack_page(tsk);
 	unsigned long high = low + THREAD_SIZE;
 
-	return (low <= sp && sp < high);
+	if (sp < low || sp >= high)
+		return false;
+
+	if (info) {
+		info->low = low;
+		info->high = high;
+		info->type = STACK_TYPE_TASK;
+	}
+
+	return true;
 }
 
 #ifdef CONFIG_VMAP_STACK
 DECLARE_PER_CPU(unsigned long [OVERFLOW_STACK_SIZE/sizeof(long)], overflow_stack);
 
-static inline bool on_overflow_stack(unsigned long sp)
+static inline bool on_overflow_stack(unsigned long sp,
+				struct stack_info *info)
 {
 	unsigned long low = (unsigned long)raw_cpu_ptr(overflow_stack);
 	unsigned long high = low + OVERFLOW_STACK_SIZE;
 
-	return (low <= sp && sp < high);
+	if (sp < low || sp >= high)
+		return false;
+
+	if (info) {
+		info->low = low;
+		info->high = high;
+		info->type = STACK_TYPE_OVERFLOW;
+	}
+
+	return true;
 }
 #else
-static inline bool on_overflow_stack(unsigned long sp) { return false; }
+static inline bool on_overflow_stack(unsigned long sp,
+			struct stack_info *info) { return false; }
 #endif
 
+
 /*
  * We can only safely access per-cpu stacks from current in a non-preemptible
  * context.
  */
-static inline bool on_accessible_stack(struct task_struct *tsk, unsigned long sp)
+static inline bool on_accessible_stack(struct task_struct *tsk,
+					unsigned long sp,
+					struct stack_info *info)
 {
-	if (on_task_stack(tsk, sp))
+	if (on_task_stack(tsk, sp, info))
 		return true;
 	if (tsk != current || preemptible())
 		return false;
-	if (on_irq_stack(sp))
+	if (on_irq_stack(sp, info))
 		return true;
-	if (on_overflow_stack(sp))
+	if (on_overflow_stack(sp, info))
 		return true;
-	if (on_sdei_stack(sp))
+	if (on_sdei_stack(sp, info))
 		return true;
 
 	return false;
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 5c338ce5a7fa..cf94e1498ba6 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -132,7 +132,7 @@ static bool regs_within_kernel_stack(struct pt_regs *regs, unsigned long addr)
 {
 	return ((addr & ~(THREAD_SIZE - 1))  ==
 		(kernel_stack_pointer(regs) & ~(THREAD_SIZE - 1))) ||
-		on_irq_stack(addr);
+		on_irq_stack(addr, NULL);
 }
 
 /**
diff --git a/arch/arm64/kernel/sdei.c b/arch/arm64/kernel/sdei.c
index 6b8d90d5ceae..f0787610f82e 100644
--- a/arch/arm64/kernel/sdei.c
+++ b/arch/arm64/kernel/sdei.c
@@ -88,23 +88,58 @@ static int init_sdei_stacks(void)
 	return err;
 }
 
-bool _on_sdei_stack(unsigned long sp)
+bool on_sdei_normal_stack(unsigned long sp,
+			struct stack_info *info)
+{
+	unsigned long low = (unsigned long)raw_cpu_read(sdei_stack_normal_ptr);
+	unsigned long high = low + SDEI_STACK_SIZE;
+
+	if (low <= sp && sp < high) {
+		if (info) {
+			info->low = low;
+			info->high = high;
+			info->type = STACK_TYPE_SDEI_NORMAL;
+		}
+		return true;
+	}
+
+	return false;
+}
+
+bool on_sdei_critical_stack(unsigned long sp,
+			struct stack_info *info)
+{
+	unsigned long low = (unsigned long)raw_cpu_read(sdei_stack_critical_ptr);
+	unsigned long high = low + SDEI_STACK_SIZE;
+
+	if (low <= sp && sp < high) {
+		if (info) {
+			info->low = low;
+			info->high = high;
+			info->type = STACK_TYPE_SDEI_CRITICAL;
+		}
+		return true;
+	}
+
+	return false;
+}
+
+bool _on_sdei_stack(unsigned long sp,
+		unsigned long *stack_low,
+		unsigned long *stack_high)
 {
 	unsigned long low, high;
 
 	if (!IS_ENABLED(CONFIG_VMAP_STACK))
 		return false;
 
-	low = (unsigned long)raw_cpu_read(sdei_stack_critical_ptr);
-	high = low + SDEI_STACK_SIZE;
-
-	if (low <= sp && sp < high)
+	if (on_sdei_critical_stack(sp, info))
 		return true;
 
-	low = (unsigned long)raw_cpu_read(sdei_stack_normal_ptr);
-	high = low + SDEI_STACK_SIZE;
+	if (on_sdei_normal_stack(sp, info))
+		return true;
 
-	return (low <= sp && sp < high);
+	return false;
 }
 
 unsigned long sdei_arch_get_entry_point(int conduit)
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index d5718a060672..4989f7ea1e59 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -50,7 +50,7 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
 	if (!tsk)
 		tsk = current;
 
-	if (!on_accessible_stack(tsk, fp))
+	if (!on_accessible_stack(tsk, fp, NULL))
 		return -EINVAL;
 
 	frame->fp = READ_ONCE_NOCHECK(*(unsigned long *)(fp));
-- 
2.17.1

Re: [PATCHv2 1/2] arm64: Add stack information to on_accessible_stack

[Mark Rutland]

On Thu, Jul 19, 2018 at 04:28:05PM -0700, Laura Abbott wrote:
> 
> In preparation for enabling the stackleak plugin on arm64,
> we need a way to get the bounds of the current stack. Extend
> on_accessible_stack to get this information.
> 
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> v2: Switched to using struct stack_info for argument passing.
> on_accessible_stack is now the primary API. Split STACK_TYPE_SDEI
> into STACK_TYPE_SDEI_NORMAL and STACK_TYPE_SDEI_CRITICAL.

[...]

> -static inline bool on_irq_stack(unsigned long sp)
> +static inline bool on_irq_stack(unsigned long sp,
> +				struct stack_info *info)
>  {
>  	unsigned long low = (unsigned long)raw_cpu_read(irq_stack_ptr);
>  	unsigned long high = low + IRQ_STACK_SIZE;
> @@ -47,46 +63,79 @@ static inline bool on_irq_stack(unsigned long sp)
>  	if (!low)
>  		return false;
>  
> -	return (low <= sp && sp < high);
> +	if (sp < low || sp >= high)
> +		return false;
> +
> +	if (info) {
> +		info->low = low;
> +		info->high = high;
> +		info->type = STACK_TYPE_IRQ;
> +	}
> +
> +	return true;
>  }

[...]

> -bool _on_sdei_stack(unsigned long sp)
> +bool on_sdei_normal_stack(unsigned long sp,
> +			struct stack_info *info)
> +{
> +	unsigned long low = (unsigned long)raw_cpu_read(sdei_stack_normal_ptr);
> +	unsigned long high = low + SDEI_STACK_SIZE;
> +
> +	if (low <= sp && sp < high) {
> +		if (info) {
> +			info->low = low;
> +			info->high = high;
> +			info->type = STACK_TYPE_SDEI_NORMAL;
> +		}
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +bool on_sdei_critical_stack(unsigned long sp,
> +			struct stack_info *info)
> +{
> +	unsigned long low = (unsigned long)raw_cpu_read(sdei_stack_critical_ptr);
> +	unsigned long high = low + SDEI_STACK_SIZE;
> +
> +	if (low <= sp && sp < high) {
> +		if (info) {
> +			info->low = low;
> +			info->high = high;
> +			info->type = STACK_TYPE_SDEI_CRITICAL;
> +		}
> +		return true;
> +	}
> +
> +	return false;
> +}

Minor nit, but it would be good to avoid the nested conditionals for these two
by bailing out early when the SP is out of bounds, as with the other
on_<foo>_stack() functions, e.g.

bool on_sdei_normal_stack(unsigned long sp,
			struct stack_info *info)
{
	unsigned long low = (unsigned long)raw_cpu_read(sdei_stack_normal_ptr);
	unsigned long high = low + SDEI_STACK_SIZE;

	if (sp < low || sp >= high)
		return false;

	if (info) {
		info->low = low;
		info->high = high;
		info->type = STACK_TYPE_SDEI_NORMAL;
	}

	return true;
}

Otherwise, this all looks good to me. With that:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

Thanks for working on this!

Mark.

[PATCHv2 2/2] arm64: Clear the stack

[Laura Abbott]

Implementation of stackleak based heavily on the x86 version

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
v2: Convert to adjusted on_acessible_stack APIs. Fixed alloca check to
just panic. Dropped the extra include per Kees. I also didn't add the
Reviewed-by since the APIs did change and I wanted another pass.
---
 arch/arm64/Kconfig                    |  1 +
 arch/arm64/include/asm/processor.h    | 15 +++++++++++++++
 arch/arm64/kernel/entry.S             |  7 +++++++
 arch/arm64/kernel/process.c           | 17 +++++++++++++++++
 arch/arm64/kvm/hyp/Makefile           |  3 ++-
 drivers/firmware/efi/libstub/Makefile |  3 ++-
 6 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 42c090cf0292..216d36a49ab5 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -96,6 +96,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 a73ae1e49200..0061450a793b 100644
--- a/arch/arm64/include/asm/processor.h
+++ b/arch/arm64/include/asm/processor.h
@@ -266,5 +266,20 @@ extern void __init minsigstksz_setup(void);
 #define SVE_SET_VL(arg)	sve_set_current_vl(arg)
 #define SVE_GET_VL()	sve_get_current_vl()
 
+/*
+ * For CONFIG_GCC_PLUGIN_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()							\
+({										\
+	struct stack_info _info;						\
+	BUG_ON(!on_accessible_stack(current, current_stack_pointer, &_info));	\
+	_info.high;								\
+})
+#define on_thread_stack()	(on_task_stack(current, current_stack_pointer, NULL))
+
 #endif /* __ASSEMBLY__ */
 #endif /* __ASM_PROCESSOR_H */
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 28ad8799406f..67d12016063d 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -431,6 +431,11 @@ tsk	.req	x28		// current thread_info
 
 	.text
 
+	.macro	stackleak_erase
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+	bl	stackleak_erase
+#endif
+	.endm
 /*
  * Exception vectors.
  */
@@ -910,6 +915,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
@@ -936,6 +942,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 e10bc363f533..2724e4d31b16 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -493,3 +493,20 @@ void arch_setup_new_exec(void)
 {
 	current->mm->context.flags = is_compat_task() ? MMCF_AARCH32 : 0;
 }
+
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+void __used stackleak_check_alloca(unsigned long size)
+{
+	unsigned long stack_left;
+	unsigned long current_sp = current_stack_pointer;
+	struct stack_info info;
+
+	BUG_ON(!on_accessible_stack(current, current_sp, &info));
+
+	stack_left = current_sp - info.low;
+
+	if (size >= stack_left)
+		panic("alloca() over the kernel stack boundary\n");
+}
+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
-- 
2.17.1

Re: [PATCHv2 2/2] arm64: Clear the stack

[Kees Cook]

On Thu, Jul 19, 2018 at 4:28 PM, Laura Abbott <labbott@redhat.com> wrote:
>
> Implementation of stackleak based heavily on the x86 version
>
> Signed-off-by: Laura Abbott <labbott@redhat.com>

This is the commit message I wrote when I was using an earlier
version, which I think is more descriptive:

    arm64: Add support for STACKLEAK gcc plugin

    This adds support for the STACKLEAK gcc plugin to arm64 by implementing
    stackleak_check_alloca(), based heavily on the x86 version, and adding the
    two helpers used by the stackleak common code: current_top_of_stack() and
    on_thread_stack(). The stack erasure calls are made at syscall returns.
    Additionally, this disables the plugin in hypervisor and EFI stub code,
    which are out of scope for the protection.

Either way:

Reviewed-by: Kees Cook <keescook@chromium.org>

Thanks for getting this hammered out!

> ---
> v2: Convert to adjusted on_acessible_stack APIs. Fixed alloca check to
> just panic. Dropped the extra include per Kees. I also didn't add the
> Reviewed-by since the APIs did change and I wanted another pass.

Maybe the panic() should get a comment above it to describe why it's
there (i.e. summarize the thread where that change was discussed?) Or
maybe mention it in the commit log (instead of being only below the
--- line?)

-Kees

> ---
>  arch/arm64/Kconfig                    |  1 +
>  arch/arm64/include/asm/processor.h    | 15 +++++++++++++++
>  arch/arm64/kernel/entry.S             |  7 +++++++
>  arch/arm64/kernel/process.c           | 17 +++++++++++++++++
>  arch/arm64/kvm/hyp/Makefile           |  3 ++-
>  drivers/firmware/efi/libstub/Makefile |  3 ++-
>  6 files changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 42c090cf0292..216d36a49ab5 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -96,6 +96,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 a73ae1e49200..0061450a793b 100644
> --- a/arch/arm64/include/asm/processor.h
> +++ b/arch/arm64/include/asm/processor.h
> @@ -266,5 +266,20 @@ extern void __init minsigstksz_setup(void);
>  #define SVE_SET_VL(arg)        sve_set_current_vl(arg)
>  #define SVE_GET_VL()   sve_get_current_vl()
>
> +/*
> + * For CONFIG_GCC_PLUGIN_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()                                                 \
> +({                                                                             \
> +       struct stack_info _info;                                                \
> +       BUG_ON(!on_accessible_stack(current, current_stack_pointer, &_info));   \
> +       _info.high;                                                             \
> +})
> +#define on_thread_stack()      (on_task_stack(current, current_stack_pointer, NULL))
> +
>  #endif /* __ASSEMBLY__ */
>  #endif /* __ASM_PROCESSOR_H */
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 28ad8799406f..67d12016063d 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -431,6 +431,11 @@ tsk        .req    x28             // current thread_info
>
>         .text
>
> +       .macro  stackleak_erase
> +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
> +       bl      stackleak_erase
> +#endif
> +       .endm
>  /*
>   * Exception vectors.
>   */
> @@ -910,6 +915,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
> @@ -936,6 +942,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 e10bc363f533..2724e4d31b16 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -493,3 +493,20 @@ void arch_setup_new_exec(void)
>  {
>         current->mm->context.flags = is_compat_task() ? MMCF_AARCH32 : 0;
>  }
> +
> +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
> +void __used stackleak_check_alloca(unsigned long size)
> +{
> +       unsigned long stack_left;
> +       unsigned long current_sp = current_stack_pointer;
> +       struct stack_info info;
> +
> +       BUG_ON(!on_accessible_stack(current, current_sp, &info));
> +
> +       stack_left = current_sp - info.low;
> +
> +       if (size >= stack_left)
> +               panic("alloca() over the kernel stack boundary\n");
> +}
> +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
> --
> 2.17.1
>



-- 
Kees Cook
Pixel Security

Re: [PATCHv2 2/2] arm64: Clear the stack

[Mark Rutland]

On Thu, Jul 19, 2018 at 04:28:06PM -0700, Laura Abbott wrote:
> 
> Implementation of stackleak based heavily on the x86 version
> 
> Signed-off-by: Laura Abbott <labbott@redhat.com>

Reviewed-by: Mark Rutlamd <mark.rutland@arm.com>

Thanks for working on this!

Mark.

> ---
> v2: Convert to adjusted on_acessible_stack APIs. Fixed alloca check to
> just panic. Dropped the extra include per Kees. I also didn't add the
> Reviewed-by since the APIs did change and I wanted another pass.
> ---
>  arch/arm64/Kconfig                    |  1 +
>  arch/arm64/include/asm/processor.h    | 15 +++++++++++++++
>  arch/arm64/kernel/entry.S             |  7 +++++++
>  arch/arm64/kernel/process.c           | 17 +++++++++++++++++
>  arch/arm64/kvm/hyp/Makefile           |  3 ++-
>  drivers/firmware/efi/libstub/Makefile |  3 ++-
>  6 files changed, 44 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 42c090cf0292..216d36a49ab5 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -96,6 +96,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 a73ae1e49200..0061450a793b 100644
> --- a/arch/arm64/include/asm/processor.h
> +++ b/arch/arm64/include/asm/processor.h
> @@ -266,5 +266,20 @@ extern void __init minsigstksz_setup(void);
>  #define SVE_SET_VL(arg)	sve_set_current_vl(arg)
>  #define SVE_GET_VL()	sve_get_current_vl()
>  
> +/*
> + * For CONFIG_GCC_PLUGIN_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()							\
> +({										\
> +	struct stack_info _info;						\
> +	BUG_ON(!on_accessible_stack(current, current_stack_pointer, &_info));	\
> +	_info.high;								\
> +})
> +#define on_thread_stack()	(on_task_stack(current, current_stack_pointer, NULL))
> +
>  #endif /* __ASSEMBLY__ */
>  #endif /* __ASM_PROCESSOR_H */
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 28ad8799406f..67d12016063d 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -431,6 +431,11 @@ tsk	.req	x28		// current thread_info
>  
>  	.text
>  
> +	.macro	stackleak_erase
> +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
> +	bl	stackleak_erase
> +#endif
> +	.endm
>  /*
>   * Exception vectors.
>   */
> @@ -910,6 +915,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
> @@ -936,6 +942,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 e10bc363f533..2724e4d31b16 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -493,3 +493,20 @@ void arch_setup_new_exec(void)
>  {
>  	current->mm->context.flags = is_compat_task() ? MMCF_AARCH32 : 0;
>  }
> +
> +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
> +void __used stackleak_check_alloca(unsigned long size)
> +{
> +	unsigned long stack_left;
> +	unsigned long current_sp = current_stack_pointer;
> +	struct stack_info info;
> +
> +	BUG_ON(!on_accessible_stack(current, current_sp, &info));
> +
> +	stack_left = current_sp - info.low;
> +
> +	if (size >= stack_left)
> +		panic("alloca() over the kernel stack boundary\n");
> +}
> +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
> -- 
> 2.17.1
> 

[PATCHv3 0/2] Stackleak for arm64

[Laura Abbott]

Hi,

This is the version of stackleak for arm64, hopefully ready for queueing

Laura Abbott (2):
  arm64: Add stack information to on_accessible_stack
  arm64: Add support for STACKLEAK gcc plugin

 arch/arm64/Kconfig                    |  1 +
 arch/arm64/include/asm/processor.h    | 15 ++++++
 arch/arm64/include/asm/sdei.h         |  9 ++--
 arch/arm64/include/asm/stacktrace.h   | 73 ++++++++++++++++++++++-----
 arch/arm64/kernel/entry.S             |  7 +++
 arch/arm64/kernel/process.c           | 22 ++++++++
 arch/arm64/kernel/ptrace.c            |  2 +-
 arch/arm64/kernel/sdei.c              | 51 ++++++++++++++++---
 arch/arm64/kernel/stacktrace.c        |  2 +-
 arch/arm64/kvm/hyp/Makefile           |  3 +-
 drivers/firmware/efi/libstub/Makefile |  3 +-
 11 files changed, 161 insertions(+), 27 deletions(-)

-- 
2.17.1

[PATCHv3 1/2] arm64: Add stack information to on_accessible_stack

[Laura Abbott]

In preparation for enabling the stackleak plugin on arm64,
we need a way to get the bounds of the current stack. Extend
on_accessible_stack to get this information.

Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Laura Abbott <labbott@redhat.com>
---
v3: Switched some logic to reduce if nesting
---
 arch/arm64/include/asm/sdei.h       |  9 ++--
 arch/arm64/include/asm/stacktrace.h | 73 ++++++++++++++++++++++++-----
 arch/arm64/kernel/ptrace.c          |  2 +-
 arch/arm64/kernel/sdei.c            | 51 ++++++++++++++++----
 arch/arm64/kernel/stacktrace.c      |  2 +-
 5 files changed, 112 insertions(+), 25 deletions(-)

diff --git a/arch/arm64/include/asm/sdei.h b/arch/arm64/include/asm/sdei.h
index e073e6886685..ffe47d766c25 100644
--- a/arch/arm64/include/asm/sdei.h
+++ b/arch/arm64/include/asm/sdei.h
@@ -40,15 +40,18 @@ asmlinkage unsigned long __sdei_handler(struct pt_regs *regs,
 unsigned long sdei_arch_get_entry_point(int conduit);
 #define sdei_arch_get_entry_point(x)	sdei_arch_get_entry_point(x)
 
-bool _on_sdei_stack(unsigned long sp);
-static inline bool on_sdei_stack(unsigned long sp)
+struct stack_info;
+
+bool _on_sdei_stack(unsigned long sp, struct stack_info *info);
+static inline bool on_sdei_stack(unsigned long sp,
+				struct stack_info *info)
 {
 	if (!IS_ENABLED(CONFIG_VMAP_STACK))
 		return false;
 	if (!IS_ENABLED(CONFIG_ARM_SDE_INTERFACE))
 		return false;
 	if (in_nmi())
-		return _on_sdei_stack(sp);
+		return _on_sdei_stack(sp, info);
 
 	return false;
 }
diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
index 902f9edacbea..e86737b7c924 100644
--- a/arch/arm64/include/asm/stacktrace.h
+++ b/arch/arm64/include/asm/stacktrace.h
@@ -32,6 +32,21 @@ struct stackframe {
 #endif
 };
 
+enum stack_type {
+	STACK_TYPE_UNKNOWN,
+	STACK_TYPE_TASK,
+	STACK_TYPE_IRQ,
+	STACK_TYPE_OVERFLOW,
+	STACK_TYPE_SDEI_NORMAL,
+	STACK_TYPE_SDEI_CRITICAL,
+};
+
+struct stack_info {
+	unsigned long low;
+	unsigned long high;
+	enum stack_type type;
+};
+
 extern int unwind_frame(struct task_struct *tsk, struct stackframe *frame);
 extern void walk_stackframe(struct task_struct *tsk, struct stackframe *frame,
 			    int (*fn)(struct stackframe *, void *), void *data);
@@ -39,7 +54,8 @@ extern void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk);
 
 DECLARE_PER_CPU(unsigned long *, irq_stack_ptr);
 
-static inline bool on_irq_stack(unsigned long sp)
+static inline bool on_irq_stack(unsigned long sp,
+				struct stack_info *info)
 {
 	unsigned long low = (unsigned long)raw_cpu_read(irq_stack_ptr);
 	unsigned long high = low + IRQ_STACK_SIZE;
@@ -47,46 +63,79 @@ static inline bool on_irq_stack(unsigned long sp)
 	if (!low)
 		return false;
 
-	return (low <= sp && sp < high);
+	if (sp < low || sp >= high)
+		return false;
+
+	if (info) {
+		info->low = low;
+		info->high = high;
+		info->type = STACK_TYPE_IRQ;
+	}
+
+	return true;
 }
 
-static inline bool on_task_stack(struct task_struct *tsk, unsigned long sp)
+static inline bool on_task_stack(struct task_struct *tsk, unsigned long sp,
+				struct stack_info *info)
 {
 	unsigned long low = (unsigned long)task_stack_page(tsk);
 	unsigned long high = low + THREAD_SIZE;
 
-	return (low <= sp && sp < high);
+	if (sp < low || sp >= high)
+		return false;
+
+	if (info) {
+		info->low = low;
+		info->high = high;
+		info->type = STACK_TYPE_TASK;
+	}
+
+	return true;
 }
 
 #ifdef CONFIG_VMAP_STACK
 DECLARE_PER_CPU(unsigned long [OVERFLOW_STACK_SIZE/sizeof(long)], overflow_stack);
 
-static inline bool on_overflow_stack(unsigned long sp)
+static inline bool on_overflow_stack(unsigned long sp,
+				struct stack_info *info)
 {
 	unsigned long low = (unsigned long)raw_cpu_ptr(overflow_stack);
 	unsigned long high = low + OVERFLOW_STACK_SIZE;
 
-	return (low <= sp && sp < high);
+	if (sp < low || sp >= high)
+		return false;
+
+	if (info) {
+		info->low = low;
+		info->high = high;
+		info->type = STACK_TYPE_OVERFLOW;
+	}
+
+	return true;
 }
 #else
-static inline bool on_overflow_stack(unsigned long sp) { return false; }
+static inline bool on_overflow_stack(unsigned long sp,
+			struct stack_info *info) { return false; }
 #endif
 
+
 /*
  * We can only safely access per-cpu stacks from current in a non-preemptible
  * context.
  */
-static inline bool on_accessible_stack(struct task_struct *tsk, unsigned long sp)
+static inline bool on_accessible_stack(struct task_struct *tsk,
+					unsigned long sp,
+					struct stack_info *info)
 {
-	if (on_task_stack(tsk, sp))
+	if (on_task_stack(tsk, sp, info))
 		return true;
 	if (tsk != current || preemptible())
 		return false;
-	if (on_irq_stack(sp))
+	if (on_irq_stack(sp, info))
 		return true;
-	if (on_overflow_stack(sp))
+	if (on_overflow_stack(sp, info))
 		return true;
-	if (on_sdei_stack(sp))
+	if (on_sdei_stack(sp, info))
 		return true;
 
 	return false;
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 5c338ce5a7fa..cf94e1498ba6 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -132,7 +132,7 @@ static bool regs_within_kernel_stack(struct pt_regs *regs, unsigned long addr)
 {
 	return ((addr & ~(THREAD_SIZE - 1))  ==
 		(kernel_stack_pointer(regs) & ~(THREAD_SIZE - 1))) ||
-		on_irq_stack(addr);
+		on_irq_stack(addr, NULL);
 }
 
 /**
diff --git a/arch/arm64/kernel/sdei.c b/arch/arm64/kernel/sdei.c
index 6b8d90d5ceae..7154fee1cb2b 100644
--- a/arch/arm64/kernel/sdei.c
+++ b/arch/arm64/kernel/sdei.c
@@ -88,23 +88,58 @@ static int init_sdei_stacks(void)
 	return err;
 }
 
-bool _on_sdei_stack(unsigned long sp)
+bool on_sdei_normal_stack(unsigned long sp,
+			struct stack_info *info)
+{
+	unsigned long low = (unsigned long)raw_cpu_read(sdei_stack_normal_ptr);
+	unsigned long high = low + SDEI_STACK_SIZE;
+
+	if (sp < low || sp >= high)
+		return false;
+
+	if (info) {
+		info->low = low;
+		info->high = high;
+		info->type = STACK_TYPE_SDEI_NORMAL;
+	}
+
+	return true;
+}
+
+bool on_sdei_critical_stack(unsigned long sp,
+			struct stack_info *info)
+{
+	unsigned long low = (unsigned long)raw_cpu_read(sdei_stack_critical_ptr);
+	unsigned long high = low + SDEI_STACK_SIZE;
+
+	if (sp < low || sp >= high)
+		return false;
+
+	if (info) {
+		info->low = low;
+		info->high = high;
+		info->type = STACK_TYPE_SDEI_CRITICAL;
+	}
+
+	return true;
+}
+
+bool _on_sdei_stack(unsigned long sp,
+		unsigned long *stack_low,
+		unsigned long *stack_high)
 {
 	unsigned long low, high;
 
 	if (!IS_ENABLED(CONFIG_VMAP_STACK))
 		return false;
 
-	low = (unsigned long)raw_cpu_read(sdei_stack_critical_ptr);
-	high = low + SDEI_STACK_SIZE;
-
-	if (low <= sp && sp < high)
+	if (on_sdei_critical_stack(sp, info))
 		return true;
 
-	low = (unsigned long)raw_cpu_read(sdei_stack_normal_ptr);
-	high = low + SDEI_STACK_SIZE;
+	if (on_sdei_normal_stack(sp, info))
+		return true;
 
-	return (low <= sp && sp < high);
+	return false;
 }
 
 unsigned long sdei_arch_get_entry_point(int conduit)
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index d5718a060672..4989f7ea1e59 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -50,7 +50,7 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
 	if (!tsk)
 		tsk = current;
 
-	if (!on_accessible_stack(tsk, fp))
+	if (!on_accessible_stack(tsk, fp, NULL))
 		return -EINVAL;
 
 	frame->fp = READ_ONCE_NOCHECK(*(unsigned long *)(fp));
-- 
2.17.1

[PATCHv3 2/2] arm64: Add support for STACKLEAK gcc plugin

[Laura Abbott]

This adds support for the STACKLEAK gcc plugin to arm64 by implementing
stackleak_check_alloca(), based heavily on the x86 version, and adding the
two helpers used by the stackleak common code: current_top_of_stack() and
on_thread_stack(). The stack erasure calls are made at syscall returns.
Additionally, this disables the plugin in hypervisor and EFI stub code,
which are out of scope for the protection.

Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Laura Abbott <labbott@redhat.com>
---
v3: Actual commit text courtesy of Kees. A comment explaining why we
panic
---
 arch/arm64/Kconfig                    |  1 +
 arch/arm64/include/asm/processor.h    | 15 +++++++++++++++
 arch/arm64/kernel/entry.S             |  7 +++++++
 arch/arm64/kernel/process.c           | 22 ++++++++++++++++++++++
 arch/arm64/kvm/hyp/Makefile           |  3 ++-
 drivers/firmware/efi/libstub/Makefile |  3 ++-
 6 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 42c090cf0292..216d36a49ab5 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -96,6 +96,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 a73ae1e49200..0061450a793b 100644
--- a/arch/arm64/include/asm/processor.h
+++ b/arch/arm64/include/asm/processor.h
@@ -266,5 +266,20 @@ extern void __init minsigstksz_setup(void);
 #define SVE_SET_VL(arg)	sve_set_current_vl(arg)
 #define SVE_GET_VL()	sve_get_current_vl()
 
+/*
+ * For CONFIG_GCC_PLUGIN_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()							\
+({										\
+	struct stack_info _info;						\
+	BUG_ON(!on_accessible_stack(current, current_stack_pointer, &_info));	\
+	_info.high;								\
+})
+#define on_thread_stack()	(on_task_stack(current, current_stack_pointer, NULL))
+
 #endif /* __ASSEMBLY__ */
 #endif /* __ASM_PROCESSOR_H */
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 28ad8799406f..67d12016063d 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -431,6 +431,11 @@ tsk	.req	x28		// current thread_info
 
 	.text
 
+	.macro	stackleak_erase
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+	bl	stackleak_erase
+#endif
+	.endm
 /*
  * Exception vectors.
  */
@@ -910,6 +915,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
@@ -936,6 +942,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 e10bc363f533..f0ad00fb6a71 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -493,3 +493,25 @@ void arch_setup_new_exec(void)
 {
 	current->mm->context.flags = is_compat_task() ? MMCF_AARCH32 : 0;
 }
+
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+void __used stackleak_check_alloca(unsigned long size)
+{
+	unsigned long stack_left;
+	unsigned long current_sp = current_stack_pointer;
+	struct stack_info info;
+
+	BUG_ON(!on_accessible_stack(current, current_sp, &info));
+
+	stack_left = current_sp - info.low;
+
+	/*
+	 * There's a good chance we're almost out of stack space if this
+	 * is true. Using panic() over BUG() is more likely to give
+	 * reliable debugging output.
+	 */
+	if (size >= stack_left)
+		panic("alloca() over the kernel stack boundary\n");
+}
+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
-- 
2.17.1

Re: [PATCHv3 2/2] arm64: Add support for STACKLEAK gcc plugin

[Alexander Popov]

On 21.07.2018 00:41, Laura Abbott wrote:
> This adds support for the STACKLEAK gcc plugin to arm64 by implementing
> stackleak_check_alloca(), based heavily on the x86 version, and adding the
> two helpers used by the stackleak common code: current_top_of_stack() and
> on_thread_stack(). The stack erasure calls are made at syscall returns.
> Additionally, this disables the plugin in hypervisor and EFI stub code,
> which are out of scope for the protection.
> 
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> v3: Actual commit text courtesy of Kees. A comment explaining why we
> panic
> ---
>  arch/arm64/Kconfig                    |  1 +
>  arch/arm64/include/asm/processor.h    | 15 +++++++++++++++
>  arch/arm64/kernel/entry.S             |  7 +++++++
>  arch/arm64/kernel/process.c           | 22 ++++++++++++++++++++++
>  arch/arm64/kvm/hyp/Makefile           |  3 ++-
>  drivers/firmware/efi/libstub/Makefile |  3 ++-
>  6 files changed, 49 insertions(+), 2 deletions(-)

Laura, thanks for your work!

I've reviewed and tested this patch on my LeMaker HiKey board (HiSilicon Kirin
620 SoC). The lkdtm tests for STACKLEAK work fine.

Acked-by: Alexander Popov <alex.popov@linux.com>

For testing I applied your patches above Kees' for-next/kspp:
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=for-next/kspp

I've had one trouble with building CONFIG_STACKLEAK_RUNTIME_DISABLE on arm64.
Kees, could you please fold this into the 7th patch of the series?

---- >8 ----

diff --git a/kernel/stackleak.c b/kernel/stackleak.c
index f731c9a..03031f7a 100644
--- a/kernel/stackleak.c
+++ b/kernel/stackleak.c
@@ -16,6 +16,7 @@

 #ifdef CONFIG_STACKLEAK_RUNTIME_DISABLE
 #include <linux/jump_label.h>
+#include <linux/sysctl.h>

 static DEFINE_STATIC_KEY_FALSE(stack_erasing_bypass);

Re: [PATCHv3 2/2] arm64: Add support for STACKLEAK gcc plugin

[Kees Cook]

On Tue, Jul 24, 2018 at 5:44 AM, Alexander Popov <alex.popov@linux.com> wrote:
> On 21.07.2018 00:41, Laura Abbott wrote:
>> This adds support for the STACKLEAK gcc plugin to arm64 by implementing
>> stackleak_check_alloca(), based heavily on the x86 version, and adding the
>> two helpers used by the stackleak common code: current_top_of_stack() and
>> on_thread_stack(). The stack erasure calls are made at syscall returns.
>> Additionally, this disables the plugin in hypervisor and EFI stub code,
>> which are out of scope for the protection.
>>
>> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
>> Reviewed-by: Kees Cook <keescook@chromium.org>
>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>> ---
>> v3: Actual commit text courtesy of Kees. A comment explaining why we
>> panic
>> ---
>>  arch/arm64/Kconfig                    |  1 +
>>  arch/arm64/include/asm/processor.h    | 15 +++++++++++++++
>>  arch/arm64/kernel/entry.S             |  7 +++++++
>>  arch/arm64/kernel/process.c           | 22 ++++++++++++++++++++++
>>  arch/arm64/kvm/hyp/Makefile           |  3 ++-
>>  drivers/firmware/efi/libstub/Makefile |  3 ++-
>>  6 files changed, 49 insertions(+), 2 deletions(-)
>
> Laura, thanks for your work!
>
> I've reviewed and tested this patch on my LeMaker HiKey board (HiSilicon Kirin
> 620 SoC). The lkdtm tests for STACKLEAK work fine.
>
> Acked-by: Alexander Popov <alex.popov@linux.com>
>
> For testing I applied your patches above Kees' for-next/kspp:
> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=for-next/kspp
>
> I've had one trouble with building CONFIG_STACKLEAK_RUNTIME_DISABLE on arm64.
> Kees, could you please fold this into the 7th patch of the series?

Sure thing!

-Kees

>
> ---- >8 ----
>
> diff --git a/kernel/stackleak.c b/kernel/stackleak.c
> index f731c9a..03031f7a 100644
> --- a/kernel/stackleak.c
> +++ b/kernel/stackleak.c
> @@ -16,6 +16,7 @@
>
>  #ifdef CONFIG_STACKLEAK_RUNTIME_DISABLE
>  #include <linux/jump_label.h>
> +#include <linux/sysctl.h>
>
>  static DEFINE_STATIC_KEY_FALSE(stack_erasing_bypass);
>



-- 
Kees Cook
Pixel Security

Re: [PATCHv3 0/2] Stackleak for arm64

[Will Deacon]

Hi Laura,

On Fri, Jul 20, 2018 at 02:41:52PM -0700, Laura Abbott wrote:
> This is the version of stackleak for arm64, hopefully ready for queueing

Thanks. I'll push these into linux-next tomorrow, once I've had a chance
to test my conflict resolution in entry.S.

Will

Re: [PATCHv3 0/2] Stackleak for arm64

[Will Deacon]

Hi Laura,

On Tue, Jul 24, 2018 at 05:38:07PM +0100, Will Deacon wrote:
> On Fri, Jul 20, 2018 at 02:41:52PM -0700, Laura Abbott wrote:
> > This is the version of stackleak for arm64, hopefully ready for queueing
> 
> Thanks. I'll push these into linux-next tomorrow, once I've had a chance
> to test my conflict resolution in entry.S.

I've run into a couple of issues with this series:

1. I had to install libmpc-dev to get GCC_PLUGINS to appear, otherwise the
   hostcc check would silently fail. I guess that's a general observation,
   but it might be nice to print a message about the missing dependencies.

2. It breaks arm64 allmodconfig build. Log below.

Please can you take a look at the build failure? Otherwise, the patches
look good to me.

Cheers,

Will

--->8

arch/arm64/kernel/sdei.c: In function ‘on_sdei_normal_stack’:
arch/arm64/kernel/sdei.c:101:7: error: dereferencing pointer to incomplete type ‘struct stack_info’
   info->low = low;
       ^~
arch/arm64/kernel/sdei.c:103:16: error: ‘STACK_TYPE_SDEI_NORMAL’ undeclared (first use in this function); did you mean ‘SCHED_NORMAL’?
   info->type = STACK_TYPE_SDEI_NORMAL;
                ^~~~~~~~~~~~~~~~~~~~~~
                SCHED_NORMAL
arch/arm64/kernel/sdei.c:103:16: note: each undeclared identifier is reported only once for each function it appears in
arch/arm64/kernel/sdei.c: In function ‘on_sdei_critical_stack’:
arch/arm64/kernel/sdei.c:121:16: error: ‘STACK_TYPE_SDEI_CRITICAL’ undeclared (first use in this function)
   info->type = STACK_TYPE_SDEI_CRITICAL;
                ^~~~~~~~~~~~~~~~~~~~~~~~
arch/arm64/kernel/sdei.c: At top level:
arch/arm64/kernel/sdei.c:127:6: error: conflicting types for ‘_on_sdei_stack’
 bool _on_sdei_stack(unsigned long sp,
      ^~~~~~~~~~~~~~
In file included from ./include/linux/arm_sdei.h:14:0,
                 from arch/arm64/kernel/sdei.c:5:
./arch/arm64/include/asm/sdei.h:45:6: note: previous declaration of ‘_on_sdei_stack’ was here
 bool _on_sdei_stack(unsigned long sp, struct stack_info *info);
      ^~~~~~~~~~~~~~
arch/arm64/kernel/sdei.c: In function ‘_on_sdei_stack’:
arch/arm64/kernel/sdei.c:136:33: error: ‘info’ undeclared (first use in this function); did you mean ‘int’?
  if (on_sdei_critical_stack(sp, info))
                                 ^~~~
                                 int
arch/arm64/kernel/sdei.c:131:21: warning: unused variable ‘high’ [-Wunused-variable]
  unsigned long low, high;
                     ^~~~
arch/arm64/kernel/sdei.c:131:16: warning: unused variable ‘low’ [-Wunused-variable]
  unsigned long low, high;
                ^~~
make[1]: *** [arch/arm64/kernel/sdei.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [arch/arm64/kernel] Error 2

Re: [PATCHv3 0/2] Stackleak for arm64

[Laura Abbott]

On 07/25/2018 04:49 AM, Will Deacon wrote:
> Hi Laura,
> 
> On Tue, Jul 24, 2018 at 05:38:07PM +0100, Will Deacon wrote:
>> On Fri, Jul 20, 2018 at 02:41:52PM -0700, Laura Abbott wrote:
>>> This is the version of stackleak for arm64, hopefully ready for queueing
>>
>> Thanks. I'll push these into linux-next tomorrow, once I've had a chance
>> to test my conflict resolution in entry.S.
> 
> I've run into a couple of issues with this series:
> 
> 1. I had to install libmpc-dev to get GCC_PLUGINS to appear, otherwise the
>     hostcc check would silently fail. I guess that's a general observation,
>     but it might be nice to print a message about the missing dependencies.
> 
> 2. It breaks arm64 allmodconfig build. Log below.
> 
> Please can you take a look at the build failure? Otherwise, the patches
> look good to me.
> 
> Cheers,
> 
> Will
> 
> --->8
> 
> arch/arm64/kernel/sdei.c: In function ‘on_sdei_normal_stack’:
> arch/arm64/kernel/sdei.c:101:7: error: dereferencing pointer to incomplete type ‘struct stack_info’
>     info->low = low;
>         ^~
> arch/arm64/kernel/sdei.c:103:16: error: ‘STACK_TYPE_SDEI_NORMAL’ undeclared (first use in this function); did you mean ‘SCHED_NORMAL’?
>     info->type = STACK_TYPE_SDEI_NORMAL;
>                  ^~~~~~~~~~~~~~~~~~~~~~
>                  SCHED_NORMAL
> arch/arm64/kernel/sdei.c:103:16: note: each undeclared identifier is reported only once for each function it appears in
> arch/arm64/kernel/sdei.c: In function ‘on_sdei_critical_stack’:
> arch/arm64/kernel/sdei.c:121:16: error: ‘STACK_TYPE_SDEI_CRITICAL’ undeclared (first use in this function)
>     info->type = STACK_TYPE_SDEI_CRITICAL;
>                  ^~~~~~~~~~~~~~~~~~~~~~~~
> arch/arm64/kernel/sdei.c: At top level:
> arch/arm64/kernel/sdei.c:127:6: error: conflicting types for ‘_on_sdei_stack’
>   bool _on_sdei_stack(unsigned long sp,
>        ^~~~~~~~~~~~~~
> In file included from ./include/linux/arm_sdei.h:14:0,
>                   from arch/arm64/kernel/sdei.c:5:
> ./arch/arm64/include/asm/sdei.h:45:6: note: previous declaration of ‘_on_sdei_stack’ was here
>   bool _on_sdei_stack(unsigned long sp, struct stack_info *info);
>        ^~~~~~~~~~~~~~
> arch/arm64/kernel/sdei.c: In function ‘_on_sdei_stack’:
> arch/arm64/kernel/sdei.c:136:33: error: ‘info’ undeclared (first use in this function); did you mean ‘int’?
>    if (on_sdei_critical_stack(sp, info))
>                                   ^~~~
>                                   int
> arch/arm64/kernel/sdei.c:131:21: warning: unused variable ‘high’ [-Wunused-variable]
>    unsigned long low, high;
>                       ^~~~
> arch/arm64/kernel/sdei.c:131:16: warning: unused variable ‘low’ [-Wunused-variable]
>    unsigned long low, high;
>                  ^~~
> make[1]: *** [arch/arm64/kernel/sdei.o] Error 1
> make[1]: *** Waiting for unfinished jobs....
> make: *** [arch/arm64/kernel] Error 2
> 

Ugh this was a failure that I missed folding in, sorry about that

--- 8< ----

diff --git a/arch/arm64/kernel/sdei.c b/arch/arm64/kernel/sdei.c
index 7154fee1cb2b..a94a868f0532 100644
--- a/arch/arm64/kernel/sdei.c
+++ b/arch/arm64/kernel/sdei.c
@@ -13,6 +13,7 @@
  #include <asm/mmu.h>
  #include <asm/ptrace.h>
  #include <asm/sections.h>
+#include <asm/stacktrace.h>
  #include <asm/sysreg.h>
  #include <asm/vmap_stack.h>
  
@@ -125,11 +126,8 @@ bool on_sdei_critical_stack(unsigned long sp,
  }
  
  bool _on_sdei_stack(unsigned long sp,
-		unsigned long *stack_low,
-		unsigned long *stack_high)
+		struct stack_info *info)
  {
-	unsigned long low, high;
-
  	if (!IS_ENABLED(CONFIG_VMAP_STACK))
  		return false;
  

Re: [PATCHv3 0/2] Stackleak for arm64

[Will Deacon]

Hi Laura,

On Wed, Jul 25, 2018 at 03:05:58PM -0700, Laura Abbott wrote:
> On 07/25/2018 04:49 AM, Will Deacon wrote:
> >arch/arm64/kernel/sdei.c: At top level:
> >arch/arm64/kernel/sdei.c:127:6: error: conflicting types for ‘_on_sdei_stack’
> >  bool _on_sdei_stack(unsigned long sp,
> >       ^~~~~~~~~~~~~~
> >In file included from ./include/linux/arm_sdei.h:14:0,
> >                  from arch/arm64/kernel/sdei.c:5:
> >./arch/arm64/include/asm/sdei.h:45:6: note: previous declaration of ‘_on_sdei_stack’ was here
> >  bool _on_sdei_stack(unsigned long sp, struct stack_info *info);
> >       ^~~~~~~~~~~~~~
> >arch/arm64/kernel/sdei.c: In function ‘_on_sdei_stack’:
> >arch/arm64/kernel/sdei.c:136:33: error: ‘info’ undeclared (first use in this function); did you mean ‘int’?
> >   if (on_sdei_critical_stack(sp, info))
> >                                  ^~~~
> >                                  int
> >arch/arm64/kernel/sdei.c:131:21: warning: unused variable ‘high’ [-Wunused-variable]
> >   unsigned long low, high;
> >                      ^~~~
> >arch/arm64/kernel/sdei.c:131:16: warning: unused variable ‘low’ [-Wunused-variable]
> >   unsigned long low, high;
> >                 ^~~
> >make[1]: *** [arch/arm64/kernel/sdei.o] Error 1
> >make[1]: *** Waiting for unfinished jobs....
> >make: *** [arch/arm64/kernel] Error 2
> >
> 
> Ugh this was a failure that I missed folding in, sorry about that

That's ok, thanks for the quick fixup. I'll fold it in and push this out
later on.

Cheers,

Will