diff mbox series

ARM: Add support for STACKLEAK gcc plugin

Message ID 20240620131649.886995-1-ruanjinjie@huawei.com
State Superseded
Headers show
Series ARM: Add support for STACKLEAK gcc plugin | expand

Commit Message

Jinjie Ruan June 20, 2024, 1:16 p.m. UTC
Add the STACKLEAK gcc plugin to arm32 by adding the helper used by
stackleak common code: on_thread_stack(). It initialize the stack with the
poison value before returning from system calls which improves the kernel
security. Additionally, this disables the plugin in EFI stub code and
decompress code, which are out of scope for the protection.

Before the test on Qemu versatilepb board:
	# echo STACKLEAK_ERASING  > /sys/kernel/debug/provoke-crash/DIRECT
	lkdtm: Performing direct entry STACKLEAK_ERASING
	lkdtm: XFAIL: stackleak is not supported on this arch (HAVE_ARCH_STACKLEAK=n)

After:
	# echo STACKLEAK_ERASING  > /sys/kernel/debug/provoke-crash/DIRECT
	lkdtm: Performing direct entry STACKLEAK_ERASING
	lkdtm: stackleak stack usage:
	  high offset: 80 bytes
	  current:     280 bytes
	  lowest:      696 bytes
	  tracked:     696 bytes
	  untracked:   192 bytes
	  poisoned:    7220 bytes
	  low offset:  4 bytes
	lkdtm: OK: the rest of the thread stack is properly erased

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 arch/arm/Kconfig                      | 1 +
 arch/arm/boot/compressed/Makefile     | 1 +
 arch/arm/include/asm/stacktrace.h     | 5 +++++
 arch/arm/kernel/entry-common.S        | 3 +++
 drivers/firmware/efi/libstub/Makefile | 3 ++-
 5 files changed, 12 insertions(+), 1 deletion(-)

Comments

Kees Cook June 20, 2024, 6:38 p.m. UTC | #1
On Thu, Jun 20, 2024 at 09:16:49PM +0800, Jinjie Ruan wrote:
> Add the STACKLEAK gcc plugin to arm32 by adding the helper used by
> stackleak common code: on_thread_stack(). It initialize the stack with the
> poison value before returning from system calls which improves the kernel
> security. Additionally, this disables the plugin in EFI stub code and
> decompress code, which are out of scope for the protection.

Oh very cool! Thanks for sending this!

> Before the test on Qemu versatilepb board:
> 	# echo STACKLEAK_ERASING  > /sys/kernel/debug/provoke-crash/DIRECT
> 	lkdtm: Performing direct entry STACKLEAK_ERASING
> 	lkdtm: XFAIL: stackleak is not supported on this arch (HAVE_ARCH_STACKLEAK=n)
> 
> After:
> 	# echo STACKLEAK_ERASING  > /sys/kernel/debug/provoke-crash/DIRECT
> 	lkdtm: Performing direct entry STACKLEAK_ERASING
> 	lkdtm: stackleak stack usage:
> 	  high offset: 80 bytes
> 	  current:     280 bytes
> 	  lowest:      696 bytes
> 	  tracked:     696 bytes
> 	  untracked:   192 bytes
> 	  poisoned:    7220 bytes
> 	  low offset:  4 bytes
> 	lkdtm: OK: the rest of the thread stack is properly erased
> 
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
>  arch/arm/Kconfig                      | 1 +
>  arch/arm/boot/compressed/Makefile     | 1 +
>  arch/arm/include/asm/stacktrace.h     | 5 +++++
>  arch/arm/kernel/entry-common.S        | 3 +++
>  drivers/firmware/efi/libstub/Makefile | 3 ++-
>  5 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 036381c5d42f..b211b7f5a138 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -86,6 +86,7 @@ config ARM
>  	select HAVE_ARCH_PFN_VALID
>  	select HAVE_ARCH_SECCOMP
>  	select HAVE_ARCH_SECCOMP_FILTER if AEABI && !OABI_COMPAT
> +	select HAVE_ARCH_STACKLEAK
>  	select HAVE_ARCH_THREAD_STRUCT_WHITELIST
>  	select HAVE_ARCH_TRACEHOOK
>  	select HAVE_ARCH_TRANSPARENT_HUGEPAGE if ARM_LPAE
> diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
> index 6bca03c0c7f0..945b5975fce2 100644
> --- a/arch/arm/boot/compressed/Makefile
> +++ b/arch/arm/boot/compressed/Makefile
> @@ -9,6 +9,7 @@ OBJS		=
>  
>  HEAD	= head.o
>  OBJS	+= misc.o decompress.o
> +CFLAGS_decompress.o += $(DISABLE_STACKLEAK_PLUGIN)
>  ifeq ($(CONFIG_DEBUG_UNCOMPRESS),y)
>  OBJS	+= debug.o
>  AFLAGS_head.o += -DDEBUG
> diff --git a/arch/arm/include/asm/stacktrace.h b/arch/arm/include/asm/stacktrace.h
> index 360f0d2406bf..a9b4b72ed241 100644
> --- a/arch/arm/include/asm/stacktrace.h
> +++ b/arch/arm/include/asm/stacktrace.h
> @@ -26,6 +26,11 @@ struct stackframe {
>  #endif
>  };
>  
> +static inline bool on_thread_stack(void)
> +{
> +	return !(((unsigned long)(current->stack) ^ current_stack_pointer) & ~(THREAD_SIZE - 1));
> +}
> +
>  static __always_inline
>  void arm_get_current_stackframe(struct pt_regs *regs, struct stackframe *frame)
>  {
> diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
> index 5c31e9de7a60..f379c852dcb7 100644
> --- a/arch/arm/kernel/entry-common.S
> +++ b/arch/arm/kernel/entry-common.S
> @@ -119,6 +119,9 @@ no_work_pending:
>  
>  	ct_user_enter save = 0
>  
> +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
> +	bl	stackleak_erase_on_task_stack
> +#endif
>  	restore_user_regs fast = 0, offset = 0
>  ENDPROC(ret_to_user_from_irq)
>  ENDPROC(ret_to_user)
> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> index 06f0428a723c..20d8a491f25f 100644
> --- a/drivers/firmware/efi/libstub/Makefile
> +++ b/drivers/firmware/efi/libstub/Makefile
> @@ -27,7 +27,8 @@ cflags-$(CONFIG_ARM64)		+= -fpie $(DISABLE_STACKLEAK_PLUGIN) \
>  cflags-$(CONFIG_ARM)		+= -DEFI_HAVE_STRLEN -DEFI_HAVE_STRNLEN \
>  				   -DEFI_HAVE_MEMCHR -DEFI_HAVE_STRRCHR \
>  				   -DEFI_HAVE_STRCMP -fno-builtin -fpic \
> -				   $(call cc-option,-mno-single-pic-base)
> +				   $(call cc-option,-mno-single-pic-base) \
> +				   $(DISABLE_STACKLEAK_PLUGIN)
>  cflags-$(CONFIG_RISCV)		+= -fpic -DNO_ALTERNATIVE -mno-relax
>  cflags-$(CONFIG_LOONGARCH)	+= -fpie

This looks very straight forward! If an ARM person can Ack this, I could
carry it via the hardening tree. Otherwise, it should probably go via
rmk's patch tracker?

-Kees
Jinjie Ruan June 21, 2024, 2:24 a.m. UTC | #2
On 2024/6/21 2:38, Kees Cook wrote:
> On Thu, Jun 20, 2024 at 09:16:49PM +0800, Jinjie Ruan wrote:
>> Add the STACKLEAK gcc plugin to arm32 by adding the helper used by
>> stackleak common code: on_thread_stack(). It initialize the stack with the
>> poison value before returning from system calls which improves the kernel
>> security. Additionally, this disables the plugin in EFI stub code and
>> decompress code, which are out of scope for the protection.
> 
> Oh very cool! Thanks for sending this!
> 
>> Before the test on Qemu versatilepb board:
>> 	# echo STACKLEAK_ERASING  > /sys/kernel/debug/provoke-crash/DIRECT
>> 	lkdtm: Performing direct entry STACKLEAK_ERASING
>> 	lkdtm: XFAIL: stackleak is not supported on this arch (HAVE_ARCH_STACKLEAK=n)
>>
>> After:
>> 	# echo STACKLEAK_ERASING  > /sys/kernel/debug/provoke-crash/DIRECT
>> 	lkdtm: Performing direct entry STACKLEAK_ERASING
>> 	lkdtm: stackleak stack usage:
>> 	  high offset: 80 bytes
>> 	  current:     280 bytes
>> 	  lowest:      696 bytes
>> 	  tracked:     696 bytes
>> 	  untracked:   192 bytes
>> 	  poisoned:    7220 bytes
>> 	  low offset:  4 bytes
>> 	lkdtm: OK: the rest of the thread stack is properly erased
>>
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>> ---
>>  arch/arm/Kconfig                      | 1 +
>>  arch/arm/boot/compressed/Makefile     | 1 +
>>  arch/arm/include/asm/stacktrace.h     | 5 +++++
>>  arch/arm/kernel/entry-common.S        | 3 +++
>>  drivers/firmware/efi/libstub/Makefile | 3 ++-
>>  5 files changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>> index 036381c5d42f..b211b7f5a138 100644
>> --- a/arch/arm/Kconfig
>> +++ b/arch/arm/Kconfig
>> @@ -86,6 +86,7 @@ config ARM
>>  	select HAVE_ARCH_PFN_VALID
>>  	select HAVE_ARCH_SECCOMP
>>  	select HAVE_ARCH_SECCOMP_FILTER if AEABI && !OABI_COMPAT
>> +	select HAVE_ARCH_STACKLEAK
>>  	select HAVE_ARCH_THREAD_STRUCT_WHITELIST
>>  	select HAVE_ARCH_TRACEHOOK
>>  	select HAVE_ARCH_TRANSPARENT_HUGEPAGE if ARM_LPAE
>> diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
>> index 6bca03c0c7f0..945b5975fce2 100644
>> --- a/arch/arm/boot/compressed/Makefile
>> +++ b/arch/arm/boot/compressed/Makefile
>> @@ -9,6 +9,7 @@ OBJS		=
>>  
>>  HEAD	= head.o
>>  OBJS	+= misc.o decompress.o
>> +CFLAGS_decompress.o += $(DISABLE_STACKLEAK_PLUGIN)
>>  ifeq ($(CONFIG_DEBUG_UNCOMPRESS),y)
>>  OBJS	+= debug.o
>>  AFLAGS_head.o += -DDEBUG
>> diff --git a/arch/arm/include/asm/stacktrace.h b/arch/arm/include/asm/stacktrace.h
>> index 360f0d2406bf..a9b4b72ed241 100644
>> --- a/arch/arm/include/asm/stacktrace.h
>> +++ b/arch/arm/include/asm/stacktrace.h
>> @@ -26,6 +26,11 @@ struct stackframe {
>>  #endif
>>  };
>>  
>> +static inline bool on_thread_stack(void)
>> +{
>> +	return !(((unsigned long)(current->stack) ^ current_stack_pointer) & ~(THREAD_SIZE - 1));
>> +}
>> +
>>  static __always_inline
>>  void arm_get_current_stackframe(struct pt_regs *regs, struct stackframe *frame)
>>  {
>> diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
>> index 5c31e9de7a60..f379c852dcb7 100644
>> --- a/arch/arm/kernel/entry-common.S
>> +++ b/arch/arm/kernel/entry-common.S
>> @@ -119,6 +119,9 @@ no_work_pending:
>>  
>>  	ct_user_enter save = 0
>>  
>> +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
>> +	bl	stackleak_erase_on_task_stack
>> +#endif
>>  	restore_user_regs fast = 0, offset = 0
>>  ENDPROC(ret_to_user_from_irq)
>>  ENDPROC(ret_to_user)
>> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
>> index 06f0428a723c..20d8a491f25f 100644
>> --- a/drivers/firmware/efi/libstub/Makefile
>> +++ b/drivers/firmware/efi/libstub/Makefile
>> @@ -27,7 +27,8 @@ cflags-$(CONFIG_ARM64)		+= -fpie $(DISABLE_STACKLEAK_PLUGIN) \
>>  cflags-$(CONFIG_ARM)		+= -DEFI_HAVE_STRLEN -DEFI_HAVE_STRNLEN \
>>  				   -DEFI_HAVE_MEMCHR -DEFI_HAVE_STRRCHR \
>>  				   -DEFI_HAVE_STRCMP -fno-builtin -fpic \
>> -				   $(call cc-option,-mno-single-pic-base)
>> +				   $(call cc-option,-mno-single-pic-base) \
>> +				   $(DISABLE_STACKLEAK_PLUGIN)
>>  cflags-$(CONFIG_RISCV)		+= -fpic -DNO_ALTERNATIVE -mno-relax
>>  cflags-$(CONFIG_LOONGARCH)	+= -fpie
> 
> This looks very straight forward! If an ARM person can Ack this, I could
> carry it via the hardening tree. Otherwise, it should probably go via
> rmk's patch tracker?

Thank you for your attention and reply.

> 
> -Kees
>
Ard Biesheuvel June 21, 2024, 11 a.m. UTC | #3
On Thu, 20 Jun 2024 at 15:14, Jinjie Ruan <ruanjinjie@huawei.com> wrote:
>
> Add the STACKLEAK gcc plugin to arm32 by adding the helper used by
> stackleak common code: on_thread_stack(). It initialize the stack with the
> poison value before returning from system calls which improves the kernel
> security. Additionally, this disables the plugin in EFI stub code and
> decompress code, which are out of scope for the protection.
>
> Before the test on Qemu versatilepb board:
>         # echo STACKLEAK_ERASING  > /sys/kernel/debug/provoke-crash/DIRECT
>         lkdtm: Performing direct entry STACKLEAK_ERASING
>         lkdtm: XFAIL: stackleak is not supported on this arch (HAVE_ARCH_STACKLEAK=n)
>
> After:
>         # echo STACKLEAK_ERASING  > /sys/kernel/debug/provoke-crash/DIRECT
>         lkdtm: Performing direct entry STACKLEAK_ERASING
>         lkdtm: stackleak stack usage:
>           high offset: 80 bytes
>           current:     280 bytes
>           lowest:      696 bytes
>           tracked:     696 bytes
>           untracked:   192 bytes
>           poisoned:    7220 bytes
>           low offset:  4 bytes
>         lkdtm: OK: the rest of the thread stack is properly erased
>
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
>  arch/arm/Kconfig                      | 1 +
>  arch/arm/boot/compressed/Makefile     | 1 +
>  arch/arm/include/asm/stacktrace.h     | 5 +++++
>  arch/arm/kernel/entry-common.S        | 3 +++
>  drivers/firmware/efi/libstub/Makefile | 3 ++-
>  5 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 036381c5d42f..b211b7f5a138 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -86,6 +86,7 @@ config ARM
>         select HAVE_ARCH_PFN_VALID
>         select HAVE_ARCH_SECCOMP
>         select HAVE_ARCH_SECCOMP_FILTER if AEABI && !OABI_COMPAT
> +       select HAVE_ARCH_STACKLEAK
>         select HAVE_ARCH_THREAD_STRUCT_WHITELIST
>         select HAVE_ARCH_TRACEHOOK
>         select HAVE_ARCH_TRANSPARENT_HUGEPAGE if ARM_LPAE
> diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
> index 6bca03c0c7f0..945b5975fce2 100644
> --- a/arch/arm/boot/compressed/Makefile
> +++ b/arch/arm/boot/compressed/Makefile
> @@ -9,6 +9,7 @@ OBJS            =
>
>  HEAD   = head.o
>  OBJS   += misc.o decompress.o
> +CFLAGS_decompress.o += $(DISABLE_STACKLEAK_PLUGIN)
>  ifeq ($(CONFIG_DEBUG_UNCOMPRESS),y)
>  OBJS   += debug.o
>  AFLAGS_head.o += -DDEBUG
> diff --git a/arch/arm/include/asm/stacktrace.h b/arch/arm/include/asm/stacktrace.h
> index 360f0d2406bf..a9b4b72ed241 100644
> --- a/arch/arm/include/asm/stacktrace.h
> +++ b/arch/arm/include/asm/stacktrace.h
> @@ -26,6 +26,11 @@ struct stackframe {
>  #endif
>  };
>
> +static inline bool on_thread_stack(void)
> +{
> +       return !(((unsigned long)(current->stack) ^ current_stack_pointer) & ~(THREAD_SIZE - 1));

Can we make this a bit more legible, without the redundant parens?
Perhaps something like the below?


unsigned long delta = current_stack_pointer ^ (unsigned long)current->stack;

return delta < THREAD_SIZE;


Other than that, this looks fine to me

Acked-by: Ard Biesheuvel <ardb@kernel.org>


> +}
> +
>  static __always_inline
>  void arm_get_current_stackframe(struct pt_regs *regs, struct stackframe *frame)
>  {
> diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
> index 5c31e9de7a60..f379c852dcb7 100644
> --- a/arch/arm/kernel/entry-common.S
> +++ b/arch/arm/kernel/entry-common.S
> @@ -119,6 +119,9 @@ no_work_pending:
>
>         ct_user_enter save = 0
>
> +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
> +       bl      stackleak_erase_on_task_stack
> +#endif
>         restore_user_regs fast = 0, offset = 0
>  ENDPROC(ret_to_user_from_irq)
>  ENDPROC(ret_to_user)
> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> index 06f0428a723c..20d8a491f25f 100644
> --- a/drivers/firmware/efi/libstub/Makefile
> +++ b/drivers/firmware/efi/libstub/Makefile
> @@ -27,7 +27,8 @@ cflags-$(CONFIG_ARM64)                += -fpie $(DISABLE_STACKLEAK_PLUGIN) \
>  cflags-$(CONFIG_ARM)           += -DEFI_HAVE_STRLEN -DEFI_HAVE_STRNLEN \
>                                    -DEFI_HAVE_MEMCHR -DEFI_HAVE_STRRCHR \
>                                    -DEFI_HAVE_STRCMP -fno-builtin -fpic \
> -                                  $(call cc-option,-mno-single-pic-base)
> +                                  $(call cc-option,-mno-single-pic-base) \
> +                                  $(DISABLE_STACKLEAK_PLUGIN)
>  cflags-$(CONFIG_RISCV)         += -fpic -DNO_ALTERNATIVE -mno-relax
>  cflags-$(CONFIG_LOONGARCH)     += -fpie
>
> --
> 2.34.1
>
>
diff mbox series

Patch

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 036381c5d42f..b211b7f5a138 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -86,6 +86,7 @@  config ARM
 	select HAVE_ARCH_PFN_VALID
 	select HAVE_ARCH_SECCOMP
 	select HAVE_ARCH_SECCOMP_FILTER if AEABI && !OABI_COMPAT
+	select HAVE_ARCH_STACKLEAK
 	select HAVE_ARCH_THREAD_STRUCT_WHITELIST
 	select HAVE_ARCH_TRACEHOOK
 	select HAVE_ARCH_TRANSPARENT_HUGEPAGE if ARM_LPAE
diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
index 6bca03c0c7f0..945b5975fce2 100644
--- a/arch/arm/boot/compressed/Makefile
+++ b/arch/arm/boot/compressed/Makefile
@@ -9,6 +9,7 @@  OBJS		=
 
 HEAD	= head.o
 OBJS	+= misc.o decompress.o
+CFLAGS_decompress.o += $(DISABLE_STACKLEAK_PLUGIN)
 ifeq ($(CONFIG_DEBUG_UNCOMPRESS),y)
 OBJS	+= debug.o
 AFLAGS_head.o += -DDEBUG
diff --git a/arch/arm/include/asm/stacktrace.h b/arch/arm/include/asm/stacktrace.h
index 360f0d2406bf..a9b4b72ed241 100644
--- a/arch/arm/include/asm/stacktrace.h
+++ b/arch/arm/include/asm/stacktrace.h
@@ -26,6 +26,11 @@  struct stackframe {
 #endif
 };
 
+static inline bool on_thread_stack(void)
+{
+	return !(((unsigned long)(current->stack) ^ current_stack_pointer) & ~(THREAD_SIZE - 1));
+}
+
 static __always_inline
 void arm_get_current_stackframe(struct pt_regs *regs, struct stackframe *frame)
 {
diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
index 5c31e9de7a60..f379c852dcb7 100644
--- a/arch/arm/kernel/entry-common.S
+++ b/arch/arm/kernel/entry-common.S
@@ -119,6 +119,9 @@  no_work_pending:
 
 	ct_user_enter save = 0
 
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+	bl	stackleak_erase_on_task_stack
+#endif
 	restore_user_regs fast = 0, offset = 0
 ENDPROC(ret_to_user_from_irq)
 ENDPROC(ret_to_user)
diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
index 06f0428a723c..20d8a491f25f 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -27,7 +27,8 @@  cflags-$(CONFIG_ARM64)		+= -fpie $(DISABLE_STACKLEAK_PLUGIN) \
 cflags-$(CONFIG_ARM)		+= -DEFI_HAVE_STRLEN -DEFI_HAVE_STRNLEN \
 				   -DEFI_HAVE_MEMCHR -DEFI_HAVE_STRRCHR \
 				   -DEFI_HAVE_STRCMP -fno-builtin -fpic \
-				   $(call cc-option,-mno-single-pic-base)
+				   $(call cc-option,-mno-single-pic-base) \
+				   $(DISABLE_STACKLEAK_PLUGIN)
 cflags-$(CONFIG_RISCV)		+= -fpic -DNO_ALTERNATIVE -mno-relax
 cflags-$(CONFIG_LOONGARCH)	+= -fpie