On Mon, Jul 6, 2020 at 4:40 AM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
> On Sun, Jul 05, 2020 at 02:13:17PM +0000, guoren@xxxxxxxxxx wrote:
> > From: Guo Ren <guoren@xxxxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxxx>
> > Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> > ---
> > 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/
