| Message ID | 20240624023612.2134144-1-ruanjinjie@huawei.com |
|---|---|
| State | Accepted |
| Commit | 2335c9cb831faba1a4efcc612886073b6f175fe4 |
| Headers | show |
| Series | [v2] ARM: Add support for STACKLEAK gcc plugin | expand |
On 2024/6/24 15:30, Linus Walleij wrote: > On Mon, Jun 24, 2024 at 4:33 AM 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> >> Acked-by: Ard Biesheuvel <ardb@kernel.org> > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > I was digging around to see if this would interfere with BPF > trampolines, but the > BPF code seems so generic that I assume it already takes stackleak into account. > Thank you very much, as Kees said, can this patch go via rmk's patch tracker now? > Yours, > Linus Walleij
On Thu, Jun 27, 2024 at 03:53:14PM +0800, Jinjie Ruan wrote: > > > On 2024/6/24 15:30, Linus Walleij wrote: > > On Mon, Jun 24, 2024 at 4:33 AM 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> > >> Acked-by: Ard Biesheuvel <ardb@kernel.org> > > > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > > > I was digging around to see if this would interfere with BPF > > trampolines, but the > > BPF code seems so generic that I assume it already takes stackleak into account. > > > Thank you very much, as Kees said, can this patch go via > rmk's patch tracker now? Probably yes (we have some reviews now). Please go ahead and add it there.
Gentle ping. On 2024/6/24 10:36, 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. > > 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> > Acked-by: Ard Biesheuvel <ardb@kernel.org> > --- > v2: > - Make on_thread_stack() more legible. > - Add Acked-by. > --- > arch/arm/Kconfig | 1 + > arch/arm/boot/compressed/Makefile | 1 + > arch/arm/include/asm/stacktrace.h | 7 +++++++ > arch/arm/kernel/entry-common.S | 3 +++ > drivers/firmware/efi/libstub/Makefile | 3 ++- > 5 files changed, 14 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..f80a85b091d6 100644 > --- a/arch/arm/include/asm/stacktrace.h > +++ b/arch/arm/include/asm/stacktrace.h > @@ -26,6 +26,13 @@ struct stackframe { > #endif > }; > > +static inline bool on_thread_stack(void) > +{ > + unsigned long delta = current_stack_pointer ^ (unsigned long)current->stack; > + > + return delta < THREAD_SIZE; > +} > + > 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 >
On Mon, Jul 22, 2024 at 10:53:10AM +0800, Jinjie Ruan wrote:
> Gentle ping.
You submitted it on 27th June to the patch system. I applied it on 2nd
July, and you would have received an automatic notification of that.
I have been on vacation from 6th July through to the 28th inclusive. As
seems to be the norm, the merge window opened during my vacation time,
in this case, 14th to 28th.
Nevertheless, the pull request was sent to Linus on Saturday 27th.
On 2024/7/29 19:12, Russell King (Oracle) wrote: > On Mon, Jul 22, 2024 at 10:53:10AM +0800, Jinjie Ruan wrote: >> Gentle ping. > > You submitted it on 27th June to the patch system. I applied it on 2nd > July, and you would have received an automatic notification of that. > > I have been on vacation from 6th July through to the 28th inclusive. As > seems to be the norm, the merge window opened during my vacation time, > in this case, 14th to 28th. > > Nevertheless, the pull request was sent to Linus on Saturday 27th. Sorry to interrupt your vacation. Thank you. >
On Mon, Jul 22, 2024 at 10:53:10AM +0800, Jinjie Ruan wrote: > Gentle ping. > > On 2024/6/24 10:36, 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. > > > > 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> > > Acked-by: Ard Biesheuvel <ardb@kernel.org> Is this a feature that you have a use case for?
On 2024/8/2 19:47, Russell King (Oracle) wrote: > On Mon, Jul 22, 2024 at 10:53:10AM +0800, Jinjie Ruan wrote: >> Gentle ping. >> >> On 2024/6/24 10:36, 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. >>> >>> 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> >>> Acked-by: Ard Biesheuvel <ardb@kernel.org> > > Is this a feature that you have a use case for? Yes, We have ARM32 products internally, and there is a need for security hardening. >
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..f80a85b091d6 100644 --- a/arch/arm/include/asm/stacktrace.h +++ b/arch/arm/include/asm/stacktrace.h @@ -26,6 +26,13 @@ struct stackframe { #endif }; +static inline bool on_thread_stack(void) +{ + unsigned long delta = current_stack_pointer ^ (unsigned long)current->stack; + + return delta < THREAD_SIZE; +} + 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