linux-csky.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] riscv: Add STACKPROTECTOR supported
@ 2020-07-05  6:24 guoren
  2020-07-05  6:53 ` Kees Cook
  0 siblings, 1 reply; 6+ messages in thread
From: guoren @ 2020-07-05  6:24 UTC (permalink / raw)
  To: palmerdabbelt, paul.walmsley, anup, greentime.hu, zong.li, guoren
  Cc: linux-riscv, linux-kernel, linux-csky, Guo Ren, Albert Ou,
	Masami Hiramatsu, Björn Töpel, Greentime Hu,
	Atish Patra

From: Guo Ren <guoren@linux.alibaba.com>

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@linux.alibaba.com>
Cc: Paul Walmsley <paul.walmsley@sifive.com>
Cc: Palmer Dabbelt <palmerdabbelt@google.com>
Cc: Albert Ou <aou@eecs.berkeley.edu>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Björn Töpel <bjorn.topel@gmail.com>
Cc: Greentime Hu <green.hu@gmail.com>
Cc: Atish Patra <atish.patra@wdc.com>
---
 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;
+
+	current->stack_canary = canary;
+	__stack_chk_guard = current->stack_canary;
+}
+#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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] riscv: Add STACKPROTECTOR supported
  2020-07-05  6:24 [PATCH] riscv: Add STACKPROTECTOR supported guoren
@ 2020-07-05  6:53 ` Kees Cook
  2020-07-05 14:16   ` Guo Ren
  0 siblings, 1 reply; 6+ messages in thread
From: Kees Cook @ 2020-07-05  6:53 UTC (permalink / raw)
  To: guoren
  Cc: palmerdabbelt, paul.walmsley, anup, greentime.hu, zong.li,
	linux-riscv, linux-kernel, linux-csky, Guo Ren, Albert Ou,
	Masami Hiramatsu, Björn Töpel, Greentime Hu,
	Atish Patra

On Sun, Jul 05, 2020 at 06:24:15AM +0000, guoren@kernel.org wrote:
> From: Guo Ren <guoren@linux.alibaba.com>
> 
> 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@linux.alibaba.com>
> Cc: Paul Walmsley <paul.walmsley@sifive.com>
> Cc: Palmer Dabbelt <palmerdabbelt@google.com>
> Cc: Albert Ou <aou@eecs.berkeley.edu>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Björn Töpel <bjorn.topel@gmail.com>
> Cc: Greentime Hu <green.hu@gmail.com>
> Cc: Atish Patra <atish.patra@wdc.com>
> ---
>  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)

> +
> +	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)

> +}
> +#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@chromium.org>

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] riscv: Add STACKPROTECTOR supported
  2020-07-05  6:53 ` Kees Cook
@ 2020-07-05 14:16   ` Guo Ren
  2020-07-05 20:31     ` Kees Cook
  0 siblings, 1 reply; 6+ messages in thread
From: Guo Ren @ 2020-07-05 14:16 UTC (permalink / raw)
  To: Kees Cook
  Cc: Palmer Dabbelt, Paul Walmsley, Anup Patel, Greentime Hu, Zong Li,
	linux-riscv, Linux Kernel Mailing List, linux-csky, Guo Ren,
	Albert Ou, Masami Hiramatsu, Björn Töpel, Greentime Hu,
	Atish Patra

Hi Kees,

On Sun, Jul 5, 2020 at 2:53 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Sun, Jul 05, 2020 at 06:24:15AM +0000, guoren@kernel.org wrote:
> > From: Guo Ren <guoren@linux.alibaba.com>
> >
> > 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@linux.alibaba.com>
> > Cc: Paul Walmsley <paul.walmsley@sifive.com>
> > Cc: Palmer Dabbelt <palmerdabbelt@google.com>
> > Cc: Albert Ou <aou@eecs.berkeley.edu>
> > Cc: Masami Hiramatsu <mhiramat@kernel.org>
> > Cc: Björn Töpel <bjorn.topel@gmail.com>
> > Cc: Greentime Hu <green.hu@gmail.com>
> > Cc: Atish Patra <atish.patra@wdc.com>
> > ---
> >  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@kernel.org/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@chromium.org>
Thank you :)

--
Best Regards
 Guo Ren

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] riscv: Add STACKPROTECTOR supported
  2020-07-05 14:16   ` Guo Ren
@ 2020-07-05 20:31     ` Kees Cook
  2020-07-06  0:55       ` Guo Ren
  0 siblings, 1 reply; 6+ messages in thread
From: Kees Cook @ 2020-07-05 20:31 UTC (permalink / raw)
  To: Guo Ren
  Cc: Palmer Dabbelt, Paul Walmsley, Anup Patel, Greentime Hu, Zong Li,
	linux-riscv, Linux Kernel Mailing List, linux-csky, Guo Ren,
	Albert Ou, Masami Hiramatsu, Björn Töpel, Greentime Hu,
	Atish Patra

On Sun, Jul 05, 2020 at 10:16:14PM +0800, Guo Ren wrote:
> On Sun, Jul 5, 2020 at 2:53 PM Kees Cook <keescook@chromium.org> wrote:
> > On Sun, Jul 05, 2020 at 06:24:15AM +0000, guoren@kernel.org wrote:
> > > +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 ?

Sure -- I assume get_cycles64() is architecturally "simple"? (i.e. it
doesn't require that the entire time-keeping subsystem has started?)

> >
> > > +
> > > +     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.

Oh, funny. I hadn't actually noticed that logic for the !CONFIG_SMP
cases. I see to problem with that, but the more important case, I think
is the per-task canaries.

> #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?

Right -- it's a limited solution. On the other had, is !CONFIG_SMP
expected to be a common config for riscv? If so, it's worth adding. If
not, I'd say skip it. (Though it looks very simple to do...)

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] riscv: Add STACKPROTECTOR supported
  2020-07-05 20:31     ` Kees Cook
@ 2020-07-06  0:55       ` Guo Ren
  2020-07-06  1:19         ` Kees Cook
  0 siblings, 1 reply; 6+ messages in thread
From: Guo Ren @ 2020-07-06  0:55 UTC (permalink / raw)
  To: Kees Cook
  Cc: Palmer Dabbelt, Paul Walmsley, Anup Patel, Greentime Hu, Zong Li,
	linux-riscv, Linux Kernel Mailing List, linux-csky, Guo Ren,
	Albert Ou, Masami Hiramatsu, Björn Töpel, Greentime Hu,
	Atish Patra

On Mon, Jul 6, 2020 at 4:31 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Sun, Jul 05, 2020 at 10:16:14PM +0800, Guo Ren wrote:
> > On Sun, Jul 5, 2020 at 2:53 PM Kees Cook <keescook@chromium.org> wrote:
> > > On Sun, Jul 05, 2020 at 06:24:15AM +0000, guoren@kernel.org wrote:
> > > > +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 ?
>
> Sure -- I assume get_cycles64() is architecturally "simple"? (i.e. it
> doesn't require that the entire time-keeping subsystem has started?)
Yes, it's just a csr read. But it's necessary? get_random_bytes should enough.

>
> > >
> > > > +
> > > > +     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.
>
> Oh, funny. I hadn't actually noticed that logic for the !CONFIG_SMP
> cases. I see to problem with that, but the more important case, I think
> is the per-task canaries.
Maybe some race condition problems. When canary changed the in the
switch to, but other CPUs still get that value concurrently.

>
> > #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?
>
> Right -- it's a limited solution. On the other had, is !CONFIG_SMP
> expected to be a common config for riscv? If so, it's worth adding. If
> not, I'd say skip it. (Though it looks very simple to do...)
CONFIG_SMP is mostly default for me and let's talk about arm64/x86
per-task solution. That is the right way.


-- 
Best Regards
 Guo Ren

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] riscv: Add STACKPROTECTOR supported
  2020-07-06  0:55       ` Guo Ren
@ 2020-07-06  1:19         ` Kees Cook
  0 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2020-07-06  1:19 UTC (permalink / raw)
  To: Guo Ren
  Cc: Palmer Dabbelt, Paul Walmsley, Anup Patel, Greentime Hu, Zong Li,
	linux-riscv, Linux Kernel Mailing List, linux-csky, Guo Ren,
	Albert Ou, Masami Hiramatsu, Björn Töpel, Greentime Hu,
	Atish Patra

On Mon, Jul 06, 2020 at 08:55:35AM +0800, Guo Ren wrote:
> On Mon, Jul 6, 2020 at 4:31 AM Kees Cook <keescook@chromium.org> wrote:
> > Sure -- I assume get_cycles64() is architecturally "simple"? (i.e. it
> > doesn't require that the entire time-keeping subsystem has started?)
> Yes, it's just a csr read. But it's necessary? get_random_bytes should enough.

Well, that depends on how early _all_ riscv platforms are able to
initialize their random pool correctly. I'd include a csr mix.

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-07-06  1:19 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-05  6:24 [PATCH] riscv: Add STACKPROTECTOR supported guoren
2020-07-05  6:53 ` Kees Cook
2020-07-05 14:16   ` Guo Ren
2020-07-05 20:31     ` Kees Cook
2020-07-06  0:55       ` Guo Ren
2020-07-06  1:19         ` Kees Cook

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).