On Sat, Sep 17, 2022 at 10:13 PM Mark Rutland <mark.rutland@xxxxxxx> wrote:
>
> On Tue, Sep 06, 2022 at 09:48:09PM -0400, guoren@xxxxxxxxxx wrote:
> > From: Guo Ren <guoren@xxxxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxxx>
> > Signed-off-by: Guo Ren <guoren@xxxxxxxxxx>
> > ---
> > 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
