linux-kernel - Re: [PATCH] riscv: Add STACKPROTECTOR supported

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 4 Jul 2020 23:53:45 -0700
From:   Kees Cook <keescook@...omium.org>
To:     guoren@...nel.org
Cc:     palmerdabbelt@...gle.com, paul.walmsley@...ive.com,
        anup@...infault.org, greentime.hu@...ive.com, zong.li@...ive.com,
        linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-csky@...r.kernel.org, Guo Ren <guoren@...ux.alibaba.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Björn Töpel <bjorn.topel@...il.com>,
        Greentime Hu <green.hu@...il.com>,
        Atish Patra <atish.patra@....com>
Subject: Re: [PATCH] riscv: Add STACKPROTECTOR supported

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

-- 
Kees Cook