[RFC PATCH] riscv: enable per-task stack canaries

[Guo Ren]

On Mon, Jul 6, 2020 at 4:40 AM Kees Cook <keescook at chromium.org> wrote:
>
> On Sun, Jul 05, 2020 at 02:13:17PM +0000, guoren at kernel.org wrote:
> > From: Guo Ren <guoren at linux.alibaba.com>
> >
> > After compare arm64 and x86 implementations, seems arm64's is more
> > flexible and readable. The key point is how gcc get the offset of
> > stack_canary from gs/el0_sp.
> >
> > x86: Use a fix offset from gs, not flexible.
> >
> > struct fixed_percpu_data {
> >         /*
> >          * GCC hardcodes the stack canary as %gs:40.  Since the
> >          * irq_stack is the object at %gs:0, we reserve the bottom
> >          * 48 bytes of the irq stack for the canary.
> >          */
> >         char            gs_base[40]; // :(
> >         unsigned long   stack_canary;
> > };
>
> Yes, x86's compiler's implementation of "thread local" stack canary
> isn't great for the kernel.
>
> > arm64: Use -mstack-protector-guard-offset & guard-reg
> >
> > ifeq ($(CONFIG_STACKPROTECTOR_PER_TASK),y)
> > prepare: stack_protector_prepare
> > stack_protector_prepare: prepare0
> >        $(eval KBUILD_CFLAGS += -mstack-protector-guard=sysreg            \
> >                                -mstack-protector-guard-reg=sp_el0        \
> >                                -mstack-protector-guard-offset=$(shell    \
> >                        awk '{if ($$2 == "TSK_STACK_CANARY") print $$3;}' \
> >                                        include/generated/asm-offsets.h))
> > endif
> >
> > I prefer arm64, but x86 percpu_data design needs to be considered ?
>
> I don't know riscv internals, so I leave that to y'all! :)
>
> > After the discussion, let's continue the work for riscv gcc
> > stack-protector.
>
> I think you'll need some buy-in from GCC before this kernel patch can
> land.
exactly!

>
> > Here is arm64 gcc's work [1].
> >
> > [1] https://github.com/gcc-mirror/gcc/commit/cd0b2d361df82c848dc7e1c3078651bb0624c3c6
>
> Can this kind of thing be made general-purposes, instead of having to
> reimplement it each time there's a new arch wanting to do it?
Great idea. Now only x86 arm64 support, It's the right time point.

>
> >
> > Signed-off-by: Guo Ren <guoren at linux.alibaba.com>
> > Cc: Kees Cook <keescook at chromium.org>
> > ---
> >  arch/riscv/Kconfig                      |  7 +++++++
> >  arch/riscv/Makefile                     | 10 ++++++++++
> >  arch/riscv/include/asm/stackprotector.h |  3 ++-
> >  arch/riscv/kernel/asm-offsets.c         |  3 +++
> >  arch/riscv/kernel/process.c             |  2 +-
> >  5 files changed, 23 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index 4b0e308..4b4e833 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -394,6 +394,13 @@ config CMDLINE_FORCE
> >
> >  endchoice
> >
> > +config CC_HAVE_STACKPROTECTOR_SYSREG
Should change to CC_HAVE_STACKPROTECTOR_GPR

> > +     def_bool $(cc-option,-mstack-protector-guard=gpr -mstack-protector-guard-reg=tp -mstack-protector-guard-offset=0)
>
> And, as I'm sure you realize, it's not supported by the riscv backend
> yet:
>
> riscv64-unknown-linux-gnu-gcc: error: unrecognized command line option '-mstack-protector-guard=gpr'; did you mean '-fstack-protector-strong'?
> riscv64-unknown-linux-gnu-gcc: error: unrecognized command line option '-mstack-protector-guard-reg=tp'; did you mean '-fstack-protector-strong'?
> riscv64-unknown-linux-gnu-gcc: error: unrecognized command line option '-mstack-protector-guard-offset=0'; did you mean '-fstack-protector-strong'?

Yeah! :) I just want to show you, how about the format: use tp in gpr
to do that. The format is similar to arm64.

tp is the task_struct point in riscv.

-- 
Best Regards
 Guo Ren

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