Re: [RFC PATCH] generic_entry: Add stackleak support

[Guo Ren]

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