* [RFC PATCH] generic_entry: Add stackleak support
@ 2022-09-07 1:48 guoren
2022-09-17 14:13 ` Mark Rutland
0 siblings, 1 reply; 3+ messages in thread
From: guoren @ 2022-09-07 1:48 UTC (permalink / raw)
To: tglx, peterz, luto, Conor.Dooley, xianting.tian, daolu, arnd
Cc: linux-kernel, linux-efi, linux-security-module, Guo Ren, Guo Ren
From: Guo Ren <guoren@linux.alibaba.com>
Make generic_entry supports basic STACKLEAK, and no arch custom
code is needed.
Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
Signed-off-by: Guo Ren <guoren@kernel.org>
---
drivers/firmware/efi/libstub/Makefile | 4 +++-
include/linux/stackleak.h | 3 +++
kernel/entry/common.c | 5 +++++
security/Kconfig.hardening | 2 +-
4 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
index d0537573501e..bb6ad37a9690 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -19,7 +19,7 @@ cflags-$(CONFIG_X86) += -m$(BITS) -D__KERNEL__ \
# arm64 uses the full KBUILD_CFLAGS so it's necessary to explicitly
# disable the stackleak plugin
cflags-$(CONFIG_ARM64) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
- -fpie $(DISABLE_STACKLEAK_PLUGIN) \
+ -fpie \
$(call cc-option,-mbranch-protection=none)
cflags-$(CONFIG_ARM) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
-fno-builtin -fpic \
@@ -27,6 +27,8 @@ cflags-$(CONFIG_ARM) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
cflags-$(CONFIG_RISCV) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
-fpic
+cflags-$(CONFIG_GCC_PLUGIN_STACKLEAK) += $(DISABLE_STACKLEAK_PLUGIN)
+
cflags-$(CONFIG_EFI_GENERIC_STUB) += -I$(srctree)/scripts/dtc/libfdt
KBUILD_CFLAGS := $(cflags-y) -Os -DDISABLE_BRANCH_PROFILING \
diff --git a/include/linux/stackleak.h b/include/linux/stackleak.h
index c36e7a3b45e7..9890802a5868 100644
--- a/include/linux/stackleak.h
+++ b/include/linux/stackleak.h
@@ -76,8 +76,11 @@ static inline void stackleak_task_init(struct task_struct *t)
# endif
}
+void noinstr stackleak_erase(void);
+
#else /* !CONFIG_GCC_PLUGIN_STACKLEAK */
static inline void stackleak_task_init(struct task_struct *t) { }
+static inline void stackleak_erase(void) {}
#endif
#endif
diff --git a/kernel/entry/common.c b/kernel/entry/common.c
index 063068a9ea9b..6acb1d6a1396 100644
--- a/kernel/entry/common.c
+++ b/kernel/entry/common.c
@@ -8,6 +8,7 @@
#include <linux/livepatch.h>
#include <linux/audit.h>
#include <linux/tick.h>
+#include <linux/stackleak.h>
#include "common.h"
@@ -194,6 +195,10 @@ static void exit_to_user_mode_prepare(struct pt_regs *regs)
lockdep_assert_irqs_disabled();
+#ifndef CONFIG_HAVE_ARCH_STACKLEAK
+ stackleak_erase();
+#endif
+
/* Flush pending rcuog wakeup before the last need_resched() check */
tick_nohz_user_enter_prepare();
diff --git a/security/Kconfig.hardening b/security/Kconfig.hardening
index bd2aabb2c60f..3329482beb8d 100644
--- a/security/Kconfig.hardening
+++ b/security/Kconfig.hardening
@@ -152,7 +152,7 @@ config GCC_PLUGIN_STRUCTLEAK_VERBOSE
config GCC_PLUGIN_STACKLEAK
bool "Poison kernel stack before returning from syscalls"
depends on GCC_PLUGINS
- depends on HAVE_ARCH_STACKLEAK
+ depends on HAVE_ARCH_STACKLEAK || GENERIC_ENTRY
help
This option makes the kernel erase the kernel stack before
returning from system calls. This has the effect of leaving
--
2.36.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [RFC PATCH] generic_entry: Add stackleak support
2022-09-07 1:48 [RFC PATCH] generic_entry: Add stackleak support guoren
@ 2022-09-17 14:13 ` Mark Rutland
2022-09-17 18:57 ` Guo Ren
0 siblings, 1 reply; 3+ messages in thread
From: Mark Rutland @ 2022-09-17 14:13 UTC (permalink / raw)
To: guoren
Cc: tglx, peterz, luto, Conor.Dooley, xianting.tian, daolu, arnd,
linux-kernel, linux-efi, linux-security-module, Guo Ren
On Tue, Sep 06, 2022 at 09:48:09PM -0400, guoren@kernel.org wrote:
> From: Guo Ren <guoren@linux.alibaba.com>
>
> Make generic_entry supports basic STACKLEAK, and no arch custom
> code is needed.
IIUC, this change is going to cause redundant work to be done on x86 (since it
erases the stack in its entry assembly). It also means any arch relying upon
this will not clear some stack contents that could be cleared from assembly
later in the return to userspace path, after the C entry code stack frames are
gone.
I assume you're adding this so that riscv can use stackleak? WHy can't it call
stackleak_erase*() later in the return-to-userspce path?
> Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> Signed-off-by: Guo Ren <guoren@kernel.org>
> ---
> drivers/firmware/efi/libstub/Makefile | 4 +++-
> include/linux/stackleak.h | 3 +++
> kernel/entry/common.c | 5 +++++
> security/Kconfig.hardening | 2 +-
> 4 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> index d0537573501e..bb6ad37a9690 100644
> --- a/drivers/firmware/efi/libstub/Makefile
> +++ b/drivers/firmware/efi/libstub/Makefile
> @@ -19,7 +19,7 @@ cflags-$(CONFIG_X86) += -m$(BITS) -D__KERNEL__ \
> # arm64 uses the full KBUILD_CFLAGS so it's necessary to explicitly
> # disable the stackleak plugin
> cflags-$(CONFIG_ARM64) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> - -fpie $(DISABLE_STACKLEAK_PLUGIN) \
> + -fpie \
> $(call cc-option,-mbranch-protection=none)
> cflags-$(CONFIG_ARM) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> -fno-builtin -fpic \
> @@ -27,6 +27,8 @@ cflags-$(CONFIG_ARM) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> cflags-$(CONFIG_RISCV) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> -fpic
>
> +cflags-$(CONFIG_GCC_PLUGIN_STACKLEAK) += $(DISABLE_STACKLEAK_PLUGIN)
> +
> cflags-$(CONFIG_EFI_GENERIC_STUB) += -I$(srctree)/scripts/dtc/libfdt
>
> KBUILD_CFLAGS := $(cflags-y) -Os -DDISABLE_BRANCH_PROFILING \
Huh; is there a latent bug here where x86's EFI stub is instrumented with
stackleak?
Thanks,
Mark.
> diff --git a/include/linux/stackleak.h b/include/linux/stackleak.h
> index c36e7a3b45e7..9890802a5868 100644
> --- a/include/linux/stackleak.h
> +++ b/include/linux/stackleak.h
> @@ -76,8 +76,11 @@ static inline void stackleak_task_init(struct task_struct *t)
> # endif
> }
>
> +void noinstr stackleak_erase(void);
> +
> #else /* !CONFIG_GCC_PLUGIN_STACKLEAK */
> static inline void stackleak_task_init(struct task_struct *t) { }
> +static inline void stackleak_erase(void) {}
> #endif
>
> #endif
> diff --git a/kernel/entry/common.c b/kernel/entry/common.c
> index 063068a9ea9b..6acb1d6a1396 100644
> --- a/kernel/entry/common.c
> +++ b/kernel/entry/common.c
> @@ -8,6 +8,7 @@
> #include <linux/livepatch.h>
> #include <linux/audit.h>
> #include <linux/tick.h>
> +#include <linux/stackleak.h>
>
> #include "common.h"
>
> @@ -194,6 +195,10 @@ static void exit_to_user_mode_prepare(struct pt_regs *regs)
>
> lockdep_assert_irqs_disabled();
>
> +#ifndef CONFIG_HAVE_ARCH_STACKLEAK
> + stackleak_erase();
> +#endif
> +
> /* Flush pending rcuog wakeup before the last need_resched() check */
> tick_nohz_user_enter_prepare();
>
> diff --git a/security/Kconfig.hardening b/security/Kconfig.hardening
> index bd2aabb2c60f..3329482beb8d 100644
> --- a/security/Kconfig.hardening
> +++ b/security/Kconfig.hardening
> @@ -152,7 +152,7 @@ config GCC_PLUGIN_STRUCTLEAK_VERBOSE
> config GCC_PLUGIN_STACKLEAK
> bool "Poison kernel stack before returning from syscalls"
> depends on GCC_PLUGINS
> - depends on HAVE_ARCH_STACKLEAK
> + depends on HAVE_ARCH_STACKLEAK || GENERIC_ENTRY
> help
> This option makes the kernel erase the kernel stack before
> returning from system calls. This has the effect of leaving
> --
> 2.36.1
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [RFC PATCH] generic_entry: Add stackleak support
2022-09-17 14:13 ` Mark Rutland
@ 2022-09-17 18:57 ` Guo Ren
0 siblings, 0 replies; 3+ messages in thread
From: Guo Ren @ 2022-09-17 18:57 UTC (permalink / raw)
To: Mark Rutland
Cc: tglx, peterz, luto, Conor.Dooley, xianting.tian, daolu, arnd,
linux-kernel, linux-efi, linux-security-module, Guo Ren
On Sat, Sep 17, 2022 at 10:13 PM Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Tue, Sep 06, 2022 at 09:48:09PM -0400, guoren@kernel.org wrote:
> > From: Guo Ren <guoren@linux.alibaba.com>
> >
> > Make generic_entry supports basic STACKLEAK, and no arch custom
> > code is needed.
>
> IIUC, this change is going to cause redundant work to be done on x86 (since it
> erases the stack in its entry assembly). It also means any arch relying upon
> this will not clear some stack contents that could be cleared from assembly
> later in the return to userspace path, after the C entry code stack frames are
> gone.
Yeah, it's a point, Thx.
>
> I assume you're adding this so that riscv can use stackleak? WHy can't it call
> stackleak_erase*() later in the return-to-userspce path?
Okay, I would move stackleak_erase back to riscv code and call it in
ret_from_exception of entry.S.
diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
index 426529b84db0..fe5f67c3ea2c 100644
--- a/arch/riscv/kernel/entry.S
+++ b/arch/riscv/kernel/entry.S
@@ -130,7 +130,6 @@ END(handle_exception)
ENTRY(ret_from_exception)
REG_L s0, PT_STATUS(sp)
- csrc CSR_STATUS, SR_IE
#ifdef CONFIG_RISCV_M_MODE
/* the MPP value is too large to be used as an immediate arg for addi */
li t0, SR_MPP
@@ -139,6 +138,8 @@ ENTRY(ret_from_exception)
andi s0, s0, SR_SPP
#endif
bnez s0, 1f
+ call stackleak_erase
+ csrc CSR_STATUS, SR_IE
/* Save unwound kernel stack pointer in thread_info */
addi s0, sp, PT_SIZE_ON_STACK
@@ -150,6 +151,7 @@ ENTRY(ret_from_exception)
*/
csrw CSR_SCRATCH, tp
1:
+ csrc CSR_STATUS, SR_IE
>
> > Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> > Signed-off-by: Guo Ren <guoren@kernel.org>
> > ---
> > drivers/firmware/efi/libstub/Makefile | 4 +++-
> > include/linux/stackleak.h | 3 +++
> > kernel/entry/common.c | 5 +++++
> > security/Kconfig.hardening | 2 +-
> > 4 files changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> > index d0537573501e..bb6ad37a9690 100644
> > --- a/drivers/firmware/efi/libstub/Makefile
> > +++ b/drivers/firmware/efi/libstub/Makefile
> > @@ -19,7 +19,7 @@ cflags-$(CONFIG_X86) += -m$(BITS) -D__KERNEL__ \
> > # arm64 uses the full KBUILD_CFLAGS so it's necessary to explicitly
> > # disable the stackleak plugin
> > cflags-$(CONFIG_ARM64) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> > - -fpie $(DISABLE_STACKLEAK_PLUGIN) \
> > + -fpie \
> > $(call cc-option,-mbranch-protection=none)
> > cflags-$(CONFIG_ARM) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> > -fno-builtin -fpic \
> > @@ -27,6 +27,8 @@ cflags-$(CONFIG_ARM) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> > cflags-$(CONFIG_RISCV) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> > -fpic
> >
> > +cflags-$(CONFIG_GCC_PLUGIN_STACKLEAK) += $(DISABLE_STACKLEAK_PLUGIN)
> > +
> > cflags-$(CONFIG_EFI_GENERIC_STUB) += -I$(srctree)/scripts/dtc/libfdt
> >
> > KBUILD_CFLAGS := $(cflags-y) -Os -DDISABLE_BRANCH_PROFILING \
>
> Huh; is there a latent bug here where x86's EFI stub is instrumented with
> stackleak?
Oops, I forgot x86. Thank you for reminding.
>
> Thanks,
> Mark.
>
> > diff --git a/include/linux/stackleak.h b/include/linux/stackleak.h
> > index c36e7a3b45e7..9890802a5868 100644
> > --- a/include/linux/stackleak.h
> > +++ b/include/linux/stackleak.h
> > @@ -76,8 +76,11 @@ static inline void stackleak_task_init(struct task_struct *t)
> > # endif
> > }
> >
> > +void noinstr stackleak_erase(void);
> > +
> > #else /* !CONFIG_GCC_PLUGIN_STACKLEAK */
> > static inline void stackleak_task_init(struct task_struct *t) { }
> > +static inline void stackleak_erase(void) {}
> > #endif
> >
> > #endif
> > diff --git a/kernel/entry/common.c b/kernel/entry/common.c
> > index 063068a9ea9b..6acb1d6a1396 100644
> > --- a/kernel/entry/common.c
> > +++ b/kernel/entry/common.c
> > @@ -8,6 +8,7 @@
> > #include <linux/livepatch.h>
> > #include <linux/audit.h>
> > #include <linux/tick.h>
> > +#include <linux/stackleak.h>
> >
> > #include "common.h"
> >
> > @@ -194,6 +195,10 @@ static void exit_to_user_mode_prepare(struct pt_regs *regs)
> >
> > lockdep_assert_irqs_disabled();
> >
> > +#ifndef CONFIG_HAVE_ARCH_STACKLEAK
> > + stackleak_erase();
> > +#endif
> > +
> > /* Flush pending rcuog wakeup before the last need_resched() check */
> > tick_nohz_user_enter_prepare();
> >
> > diff --git a/security/Kconfig.hardening b/security/Kconfig.hardening
> > index bd2aabb2c60f..3329482beb8d 100644
> > --- a/security/Kconfig.hardening
> > +++ b/security/Kconfig.hardening
> > @@ -152,7 +152,7 @@ config GCC_PLUGIN_STRUCTLEAK_VERBOSE
> > config GCC_PLUGIN_STACKLEAK
> > bool "Poison kernel stack before returning from syscalls"
> > depends on GCC_PLUGINS
> > - depends on HAVE_ARCH_STACKLEAK
> > + depends on HAVE_ARCH_STACKLEAK || GENERIC_ENTRY
> > help
> > This option makes the kernel erase the kernel stack before
> > returning from system calls. This has the effect of leaving
> > --
> > 2.36.1
> >
--
Best Regards
Guo Ren
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-09-17 18:57 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-09-07 1:48 [RFC PATCH] generic_entry: Add stackleak support guoren
2022-09-17 14:13 ` Mark Rutland
2022-09-17 18:57 ` Guo Ren
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox