Re: [PATCH] riscv: Add STACKPROTECTOR supported

[Guo Ren]

Hi Kees,

On Sun, Jul 5, 2020 at 2:53 PM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
> On Sun, Jul 05, 2020 at 06:24:15AM +0000, guoren@xxxxxxxxxx wrote:
> > From: Guo Ren <guoren@xxxxxxxxxxxxxxxxx>
> >
> > The -fstack-protector & -fstack-protector-strong features are from
> > gcc. The patch only add basic kernel support to stack-protector
> > feature and some arch could have its own solution such as
> > ARM64_PTR_AUTH.
> >
> > After enabling STACKPROTECTOR and STACKPROTECTOR_STRONG, the .text
> > size is expanded from  0x7de066 to 0x81fb32 (only 5%) to add canary
> > checking code.
> >
> > Signed-off-by: Guo Ren <guoren@xxxxxxxxxxxxxxxxx>
> > Cc: Paul Walmsley <paul.walmsley@xxxxxxxxxx>
> > Cc: Palmer Dabbelt <palmerdabbelt@xxxxxxxxxx>
> > Cc: Albert Ou <aou@xxxxxxxxxxxxxxxxx>
> > Cc: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
> > Cc: BjÃrn TÃpel <bjorn.topel@xxxxxxxxx>
> > Cc: Greentime Hu <green.hu@xxxxxxxxx>
> > Cc: Atish Patra <atish.patra@xxxxxxx>
> > ---
> >  arch/riscv/Kconfig                      |  1 +
> >  arch/riscv/include/asm/stackprotector.h | 29 +++++++++++++++++++++++++++++
> >  arch/riscv/kernel/process.c             |  6 ++++++
> >  3 files changed, 36 insertions(+)
> >  create mode 100644 arch/riscv/include/asm/stackprotector.h
> >
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index f927a91..4b0e308 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -63,6 +63,7 @@ config RISCV
> >       select HAVE_PERF_EVENTS
> >       select HAVE_PERF_REGS
> >       select HAVE_PERF_USER_STACK_DUMP
> > +     select HAVE_STACKPROTECTOR
> >       select HAVE_SYSCALL_TRACEPOINTS
> >       select IRQ_DOMAIN
> >       select MODULES_USE_ELF_RELA if MODULES
> > diff --git a/arch/riscv/include/asm/stackprotector.h b/arch/riscv/include/asm/stackprotector.h
> > new file mode 100644
> > index 00000000..5962f88
> > --- /dev/null
> > +++ b/arch/riscv/include/asm/stackprotector.h
> > @@ -0,0 +1,29 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#ifndef _ASM_RISCV_STACKPROTECTOR_H
> > +#define _ASM_RISCV_STACKPROTECTOR_H
> > +
> > +#include <linux/random.h>
> > +#include <linux/version.h>
> > +
> > +extern unsigned long __stack_chk_guard;
> > +
> > +/*
> > + * Initialize the stackprotector canary value.
> > + *
> > + * NOTE: this must only be called from functions that never return,
> > + * and it must always be inlined.
> > + */
> > +static __always_inline void boot_init_stack_canary(void)
> > +{
> > +     unsigned long canary;
> > +
> > +     /* Try to get a semi random initial value. */
> > +     get_random_bytes(&canary, sizeof(canary));
> > +     canary ^= LINUX_VERSION_CODE;
> > +     canary &= CANARY_MASK;
>
> Does riscv have any kind of instruction counters or other trivial timers
> that could be mixed in here? (e.g. x86's TSC)
Do you mean:
  get_random_bytes(&canary, sizeof(canary));
+ canary += get_cycles64() + (get_cycles64() << 32UL);
  canary ^= LINUX_VERSION_CODE;
  canary &= CANARY_MASK;

Ok ?

>
> > +
> > +     current->stack_canary = canary;
> > +     __stack_chk_guard = current->stack_canary;
>
> What's needed for riscv to support a per-task canary? (e.g. x86's TLS or
> arm64's register-specific methods)
Some archs change __stack_chk_guard in _switch_to of entry.S, but it
depends on !CONFIG_SMP.

#if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_SMP)
        get  value  from next_task->stack_canary
        store  value to __stack_chk_guard
#endif

It's a so limitation solution for per-task canary, so I didn't copy it
into riscv?

For the register-specific, I prefer arm64's then x86, let's continue
talk in this mail thread:

https://lore.kernel.org/linux-riscv/1593958397-62466-1-git-send-email-guoren@xxxxxxxxxx/T/#u

>
> > +}
> > +#endif /* _ASM_RISCV_STACKPROTECTOR_H */
> > diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c
> > index 824d117..6548929 100644
> > --- a/arch/riscv/kernel/process.c
> > +++ b/arch/riscv/kernel/process.c
> > @@ -24,6 +24,12 @@
> >
> >  register unsigned long gp_in_global __asm__("gp");
> >
> > +#ifdef CONFIG_STACKPROTECTOR
> > +#include <linux/stackprotector.h>
> > +unsigned long __stack_chk_guard __read_mostly;
> > +EXPORT_SYMBOL(__stack_chk_guard);
> > +#endif
> > +
> >  extern asmlinkage void ret_from_fork(void);
> >  extern asmlinkage void ret_from_kernel_thread(void);
> >
> > --
> > 2.7.4
> >
>
> But yes, as a starting point, better to have a single per-boot global
> canary than none at all. :)
>
> Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>
Thank you :)

--
Best Regards
 Guo Ren

ML: https://lore.kernel.org/linux-csky/