Re: [PATCH v1 2/2] stackprotector: actually use get_random_canary()

From: Guo Ren
Date: Sun Oct 23 2022 - 20:47:37 EST

On Mon, Oct 24, 2022 at 4:32 AM Jason A. Donenfeld <Jason@xxxxxxxxx> wrote:
>
> The RNG always mixes in the Linux version extremely early in boot. It
> also always includes a cycle counter, not only during early boot, but
> each and every time it is invoked prior to being fully initialized.
> Together, this means that the use of additional xors inside of the
> various stackprotector.h files is superfluous and over-complicated.
> Instead, we can get exactly the same thing, but better, by just calling
> `get_random_canary()`.
>
> Signed-off-by: Jason A. Donenfeld <Jason@xxxxxxxxx>
> ---
> arch/arm/include/asm/stackprotector.h | 9 +--------
> arch/arm64/include/asm/stackprotector.h | 9 +--------
> arch/csky/include/asm/stackprotector.h | 10 +---------
> arch/mips/include/asm/stackprotector.h | 9 +--------
> arch/powerpc/include/asm/stackprotector.h | 10 +---------
> arch/riscv/include/asm/stackprotector.h | 10 +---------
> arch/sh/include/asm/stackprotector.h | 10 +---------
> arch/x86/include/asm/stackprotector.h | 14 +-------------
> arch/xtensa/include/asm/stackprotector.h | 7 +------
> 9 files changed, 9 insertions(+), 79 deletions(-)
>
> diff --git a/arch/arm/include/asm/stackprotector.h b/arch/arm/include/asm/stackprotector.h
> index 088d03161be5..0bd4979759f1 100644
> --- a/arch/arm/include/asm/stackprotector.h
> +++ b/arch/arm/include/asm/stackprotector.h
> @@ -15,9 +15,6 @@
> #ifndef _ASM_STACKPROTECTOR_H
> #define _ASM_STACKPROTECTOR_H 1
>
> -#include <linux/random.h>
> -#include <linux/version.h>
> -
> #include <asm/thread_info.h>
>
> extern unsigned long __stack_chk_guard;
> @@ -30,11 +27,7 @@ extern unsigned long __stack_chk_guard;
> */
> 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;
> + unsigned long canary = get_random_canary();
>
> current->stack_canary = canary;
> #ifndef CONFIG_STACKPROTECTOR_PER_TASK
> diff --git a/arch/arm64/include/asm/stackprotector.h b/arch/arm64/include/asm/stackprotector.h
> index 33f1bb453150..ae3ad80f51fe 100644
> --- a/arch/arm64/include/asm/stackprotector.h
> +++ b/arch/arm64/include/asm/stackprotector.h
> @@ -13,8 +13,6 @@
> #ifndef __ASM_STACKPROTECTOR_H
> #define __ASM_STACKPROTECTOR_H
>
> -#include <linux/random.h>
> -#include <linux/version.h>
> #include <asm/pointer_auth.h>
>
> extern unsigned long __stack_chk_guard;
> @@ -28,12 +26,7 @@ extern unsigned long __stack_chk_guard;
> static __always_inline void boot_init_stack_canary(void)
> {
> #if defined(CONFIG_STACKPROTECTOR)
> - unsigned long canary;
> -
> - /* Try to get a semi random initial value. */
> - get_random_bytes(&canary, sizeof(canary));
> - canary ^= LINUX_VERSION_CODE;
> - canary &= CANARY_MASK;
> + unsigned long canary = get_random_canary();
>
> current->stack_canary = canary;
> if (!IS_ENABLED(CONFIG_STACKPROTECTOR_PER_TASK))
> diff --git a/arch/csky/include/asm/stackprotector.h b/arch/csky/include/asm/stackprotector.h
> index d7cd4e51edd9..d23747447166 100644
> --- a/arch/csky/include/asm/stackprotector.h
> +++ b/arch/csky/include/asm/stackprotector.h
> @@ -2,9 +2,6 @@
> #ifndef _ASM_STACKPROTECTOR_H
> #define _ASM_STACKPROTECTOR_H 1
>
> -#include <linux/random.h>
> -#include <linux/version.h>
> -
> extern unsigned long __stack_chk_guard;
>
> /*
> @@ -15,12 +12,7 @@ extern unsigned long __stack_chk_guard;
> */
> 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;
> + unsigned long canary = get_random_canary();
Acked-by: Guo Ren <guoren@xxxxxxxxxx> #csky part

>
> current->stack_canary = canary;
> __stack_chk_guard = current->stack_canary;
> diff --git a/arch/mips/include/asm/stackprotector.h b/arch/mips/include/asm/stackprotector.h
> index 68d4be9e1254..518c192ad982 100644
> --- a/arch/mips/include/asm/stackprotector.h
> +++ b/arch/mips/include/asm/stackprotector.h
> @@ -15,9 +15,6 @@
> #ifndef _ASM_STACKPROTECTOR_H
> #define _ASM_STACKPROTECTOR_H 1
>
> -#include <linux/random.h>
> -#include <linux/version.h>
> -
> extern unsigned long __stack_chk_guard;
>
> /*
> @@ -28,11 +25,7 @@ extern unsigned long __stack_chk_guard;
> */
> 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;
> + unsigned long canary = get_random_canary();
>
> current->stack_canary = canary;
> __stack_chk_guard = current->stack_canary;
> diff --git a/arch/powerpc/include/asm/stackprotector.h b/arch/powerpc/include/asm/stackprotector.h
> index 1c8460e23583..283c34647856 100644
> --- a/arch/powerpc/include/asm/stackprotector.h
> +++ b/arch/powerpc/include/asm/stackprotector.h
> @@ -7,8 +7,6 @@
> #ifndef _ASM_STACKPROTECTOR_H
> #define _ASM_STACKPROTECTOR_H
>
> -#include <linux/random.h>
> -#include <linux/version.h>
> #include <asm/reg.h>
> #include <asm/current.h>
> #include <asm/paca.h>
> @@ -21,13 +19,7 @@
> */
> static __always_inline void boot_init_stack_canary(void)
> {
> - unsigned long canary;
> -
> - /* Try to get a semi random initial value. */
> - canary = get_random_canary();
> - canary ^= mftb();
> - canary ^= LINUX_VERSION_CODE;
> - canary &= CANARY_MASK;
> + unsigned long canary = get_random_canary();
>
> current->stack_canary = canary;
> #ifdef CONFIG_PPC64
> diff --git a/arch/riscv/include/asm/stackprotector.h b/arch/riscv/include/asm/stackprotector.h
> index 09093af46565..43895b90fe3f 100644
> --- a/arch/riscv/include/asm/stackprotector.h
> +++ b/arch/riscv/include/asm/stackprotector.h
> @@ -3,9 +3,6 @@
> #ifndef _ASM_RISCV_STACKPROTECTOR_H
> #define _ASM_RISCV_STACKPROTECTOR_H
>
> -#include <linux/random.h>
> -#include <linux/version.h>
> -
> extern unsigned long __stack_chk_guard;
>
> /*
> @@ -16,12 +13,7 @@ extern unsigned long __stack_chk_guard;
> */
> 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;
> + unsigned long canary = get_random_canary();
>
> current->stack_canary = canary;
> if (!IS_ENABLED(CONFIG_STACKPROTECTOR_PER_TASK))
> diff --git a/arch/sh/include/asm/stackprotector.h b/arch/sh/include/asm/stackprotector.h
> index 35616841d0a1..665dafac376f 100644
> --- a/arch/sh/include/asm/stackprotector.h
> +++ b/arch/sh/include/asm/stackprotector.h
> @@ -2,9 +2,6 @@
> #ifndef __ASM_SH_STACKPROTECTOR_H
> #define __ASM_SH_STACKPROTECTOR_H
>
> -#include <linux/random.h>
> -#include <linux/version.h>
> -
> extern unsigned long __stack_chk_guard;
>
> /*
> @@ -15,12 +12,7 @@ extern unsigned long __stack_chk_guard;
> */
> 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;
> + unsigned long canary = get_random_canary();
>
> current->stack_canary = canary;
> __stack_chk_guard = current->stack_canary;
> diff --git a/arch/x86/include/asm/stackprotector.h b/arch/x86/include/asm/stackprotector.h
> index 24a8d6c4fb18..00473a650f51 100644
> --- a/arch/x86/include/asm/stackprotector.h
> +++ b/arch/x86/include/asm/stackprotector.h
> @@ -34,7 +34,6 @@
> #include <asm/percpu.h>
> #include <asm/desc.h>
>
> -#include <linux/random.h>
> #include <linux/sched.h>
>
> /*
> @@ -50,22 +49,11 @@
> */
> static __always_inline void boot_init_stack_canary(void)
> {
> - u64 canary;
> - u64 tsc;
> + unsigned long canary = get_random_canary();
>
> #ifdef CONFIG_X86_64
> BUILD_BUG_ON(offsetof(struct fixed_percpu_data, stack_canary) != 40);
> #endif
> - /*
> - * We both use the random pool and the current TSC as a source
> - * of randomness. The TSC only matters for very early init,
> - * there it already has some randomness on most systems. Later
> - * on during the bootup the random pool has true entropy too.
> - */
> - get_random_bytes(&canary, sizeof(canary));
> - tsc = rdtsc();
> - canary += tsc + (tsc << 32UL);
> - canary &= CANARY_MASK;
>
> current->stack_canary = canary;
> #ifdef CONFIG_X86_64
> diff --git a/arch/xtensa/include/asm/stackprotector.h b/arch/xtensa/include/asm/stackprotector.h
> index e368f94fd2af..e1e318a0c98a 100644
> --- a/arch/xtensa/include/asm/stackprotector.h
> +++ b/arch/xtensa/include/asm/stackprotector.h
> @@ -14,7 +14,6 @@
> #ifndef _ASM_STACKPROTECTOR_H
> #define _ASM_STACKPROTECTOR_H 1
>
> -#include <linux/random.h>
> #include <linux/version.h>
>
> extern unsigned long __stack_chk_guard;
> @@ -27,11 +26,7 @@ extern unsigned long __stack_chk_guard;
> */
> 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;
> + unsigned long canary = get_random_canary();
>
> current->stack_canary = canary;
> __stack_chk_guard = current->stack_canary;
> --
> 2.38.1
>


--
Best Regards
Guo Ren