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]
Message-ID: <CAFULd4Yc6JSH3BBCQ=Hr8Noxrc5HTw4FNxQk2jd5BRrRiU3Cow@mail.gmail.com>
Date: Thu, 7 Nov 2024 14:29:00 +0100
From: Uros Bizjak <ubizjak@...il.com>
To: Brian Gerst <brgerst@...il.com>
Cc: linux-kernel@...r.kernel.org, x86@...nel.org, 
	Ingo Molnar <mingo@...nel.org>, "H . Peter Anvin" <hpa@...or.com>, Thomas Gleixner <tglx@...utronix.de>, 
	Borislav Petkov <bp@...en8.de>, Ard Biesheuvel <ardb@...nel.org>
Subject: Re: [PATCH v5 08/16] x86/stackprotector/64: Convert to normal percpu variable

On Tue, Nov 5, 2024 at 4:58 PM Brian Gerst <brgerst@...il.com> wrote:
>
> Older versions of GCC fixed the location of the stack protector canary
> at %gs:40.  This constraint forced the percpu section to be linked at
> absolute address 0 so that the canary could be the first data object in
> the percpu section.  Supporting the zero-based percpu section requires
> additional code to handle relocations for RIP-relative references to
> percpu data, extra complexity to kallsyms, and workarounds for linker
> bugs due to the use of absolute symbols.
>
> GCC 8.1 supports redefining where the canary is located, allowng it to
> become a normal percpu variable instead of at a fixed location.  This
> removes the contraint that the percpu section must be zero-based.
>
> Signed-off-by: Brian Gerst <brgerst@...il.com>

Reviewed-by: Uros Bizjak <ubizjak@...il.com>

> ---
>  arch/x86/Makefile                     | 20 +++++++++------
>  arch/x86/entry/entry.S                |  2 --
>  arch/x86/entry/entry_64.S             |  2 +-
>  arch/x86/include/asm/processor.h      | 16 ++----------
>  arch/x86/include/asm/stackprotector.h | 36 ++++-----------------------
>  arch/x86/kernel/asm-offsets_64.c      |  6 -----
>  arch/x86/kernel/cpu/common.c          |  5 +---
>  arch/x86/kernel/head_64.S             |  3 +--
>  arch/x86/xen/xen-head.S               |  3 +--
>  9 files changed, 23 insertions(+), 70 deletions(-)
>
> diff --git a/arch/x86/Makefile b/arch/x86/Makefile
> index 5b773b34768d..88a1705366f9 100644
> --- a/arch/x86/Makefile
> +++ b/arch/x86/Makefile
> @@ -140,14 +140,7 @@ ifeq ($(CONFIG_X86_32),y)
>          # temporary until string.h is fixed
>          KBUILD_CFLAGS += -ffreestanding
>
> -    ifeq ($(CONFIG_STACKPROTECTOR),y)
> -        ifeq ($(CONFIG_SMP),y)
> -            KBUILD_CFLAGS += -mstack-protector-guard-reg=fs \
> -                             -mstack-protector-guard-symbol=__ref_stack_chk_guard
> -        else
> -            KBUILD_CFLAGS += -mstack-protector-guard=global
> -        endif
> -    endif
> +        percpu_seg := fs
>  else
>          BITS := 64
>          UTS_MACHINE := x86_64
> @@ -197,6 +190,17 @@ else
>          KBUILD_CFLAGS += -mcmodel=kernel
>          KBUILD_RUSTFLAGS += -Cno-redzone=y
>          KBUILD_RUSTFLAGS += -Ccode-model=kernel
> +
> +        percpu_seg := gs
> +endif
> +
> +ifeq ($(CONFIG_STACKPROTECTOR),y)
> +    ifeq ($(CONFIG_SMP),y)
> +       KBUILD_CFLAGS += -mstack-protector-guard-reg=$(percpu_seg)
> +       KBUILD_CFLAGS += -mstack-protector-guard-symbol=__ref_stack_chk_guard
> +    else
> +       KBUILD_CFLAGS += -mstack-protector-guard=global
> +    endif
>  endif
>
>  #
> diff --git a/arch/x86/entry/entry.S b/arch/x86/entry/entry.S
> index b7ea3e8e9ecc..fe5344a249a1 100644
> --- a/arch/x86/entry/entry.S
> +++ b/arch/x86/entry/entry.S
> @@ -52,7 +52,6 @@ EXPORT_SYMBOL_GPL(mds_verw_sel);
>
>  THUNK warn_thunk_thunk, __warn_thunk
>
> -#ifndef CONFIG_X86_64
>  /*
>   * Clang's implementation of TLS stack cookies requires the variable in
>   * question to be a TLS variable. If the variable happens to be defined as an
> @@ -66,4 +65,3 @@ THUNK warn_thunk_thunk, __warn_thunk
>  #ifdef CONFIG_STACKPROTECTOR
>  EXPORT_SYMBOL(__ref_stack_chk_guard);
>  #endif
> -#endif
> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> index 1b5be07f8669..f78ef9667c39 100644
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -192,7 +192,7 @@ SYM_FUNC_START(__switch_to_asm)
>
>  #ifdef CONFIG_STACKPROTECTOR
>         movq    TASK_stack_canary(%rsi), %rbx
> -       movq    %rbx, PER_CPU_VAR(fixed_percpu_data + FIXED_stack_canary)
> +       movq    %rbx, PER_CPU_VAR(__stack_chk_guard)
>  #endif
>
>         /*
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index c0975815980c..a113c3f4f558 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -419,16 +419,8 @@ struct irq_stack {
>
>  #ifdef CONFIG_X86_64
>  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.
> -        *
> -        * Once we are willing to require -mstack-protector-guard-symbol=
> -        * support for x86_64 stackprotector, we can get rid of this.
> -        */
>         char            gs_base[40];
> -       unsigned long   stack_canary;
> +       unsigned long   reserved;
>  };
>
>  DECLARE_PER_CPU_FIRST(struct fixed_percpu_data, fixed_percpu_data) __visible;
> @@ -443,11 +435,7 @@ extern asmlinkage void entry_SYSCALL32_ignore(void);
>
>  /* Save actual FS/GS selectors and bases to current->thread */
>  void current_save_fsgs(void);
> -#else  /* X86_64 */
> -#ifdef CONFIG_STACKPROTECTOR
> -DECLARE_PER_CPU(unsigned long, __stack_chk_guard);
> -#endif
> -#endif /* !X86_64 */
> +#endif /* X86_64 */
>
>  struct perf_event;
>
> diff --git a/arch/x86/include/asm/stackprotector.h b/arch/x86/include/asm/stackprotector.h
> index 00473a650f51..d43fb589fcf6 100644
> --- a/arch/x86/include/asm/stackprotector.h
> +++ b/arch/x86/include/asm/stackprotector.h
> @@ -2,26 +2,10 @@
>  /*
>   * GCC stack protector support.
>   *
> - * Stack protector works by putting predefined pattern at the start of
> + * Stack protector works by putting a predefined pattern at the start of
>   * the stack frame and verifying that it hasn't been overwritten when
> - * returning from the function.  The pattern is called stack canary
> - * and unfortunately gcc historically required it to be at a fixed offset
> - * from the percpu segment base.  On x86_64, the offset is 40 bytes.
> - *
> - * The same segment is shared by percpu area and stack canary.  On
> - * x86_64, percpu symbols are zero based and %gs (64-bit) points to the
> - * base of percpu area.  The first occupant of the percpu area is always
> - * fixed_percpu_data which contains stack_canary at the appropriate
> - * offset.  On x86_32, the stack canary is just a regular percpu
> - * variable.
> - *
> - * Putting percpu data in %fs on 32-bit is a minor optimization compared to
> - * using %gs.  Since 32-bit userspace normally has %fs == 0, we are likely
> - * to load 0 into %fs on exit to usermode, whereas with percpu data in
> - * %gs, we are likely to load a non-null %gs on return to user mode.
> - *
> - * Once we are willing to require GCC 8.1 or better for 64-bit stackprotector
> - * support, we can remove some of this complexity.
> + * returning from the function.  The pattern is called the stack canary
> + * and is a unique value for each task.
>   */
>
>  #ifndef _ASM_STACKPROTECTOR_H
> @@ -36,6 +20,8 @@
>
>  #include <linux/sched.h>
>
> +DECLARE_PER_CPU(unsigned long, __stack_chk_guard);
> +
>  /*
>   * Initialize the stackprotector canary value.
>   *
> @@ -51,25 +37,13 @@ static __always_inline void boot_init_stack_canary(void)
>  {
>         unsigned long canary = get_random_canary();
>
> -#ifdef CONFIG_X86_64
> -       BUILD_BUG_ON(offsetof(struct fixed_percpu_data, stack_canary) != 40);
> -#endif
> -
>         current->stack_canary = canary;
> -#ifdef CONFIG_X86_64
> -       this_cpu_write(fixed_percpu_data.stack_canary, canary);
> -#else
>         this_cpu_write(__stack_chk_guard, canary);
> -#endif
>  }
>
>  static inline void cpu_init_stack_canary(int cpu, struct task_struct *idle)
>  {
> -#ifdef CONFIG_X86_64
> -       per_cpu(fixed_percpu_data.stack_canary, cpu) = idle->stack_canary;
> -#else
>         per_cpu(__stack_chk_guard, cpu) = idle->stack_canary;
> -#endif
>  }
>
>  #else  /* STACKPROTECTOR */
> diff --git a/arch/x86/kernel/asm-offsets_64.c b/arch/x86/kernel/asm-offsets_64.c
> index bb65371ea9df..590b6cd0eac0 100644
> --- a/arch/x86/kernel/asm-offsets_64.c
> +++ b/arch/x86/kernel/asm-offsets_64.c
> @@ -54,11 +54,5 @@ int main(void)
>         BLANK();
>  #undef ENTRY
>
> -       BLANK();
> -
> -#ifdef CONFIG_STACKPROTECTOR
> -       OFFSET(FIXED_stack_canary, fixed_percpu_data, stack_canary);
> -       BLANK();
> -#endif
>         return 0;
>  }
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 9d42bd15e06c..1f33d5feb050 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -2086,8 +2086,7 @@ void syscall_init(void)
>         if (!cpu_feature_enabled(X86_FEATURE_FRED))
>                 idt_syscall_init();
>  }
> -
> -#else  /* CONFIG_X86_64 */
> +#endif /* CONFIG_X86_64 */
>
>  #ifdef CONFIG_STACKPROTECTOR
>  DEFINE_PER_CPU(unsigned long, __stack_chk_guard);
> @@ -2096,8 +2095,6 @@ EXPORT_PER_CPU_SYMBOL(__stack_chk_guard);
>  #endif
>  #endif
>
> -#endif /* CONFIG_X86_64 */
> -
>  /*
>   * Clear all 6 debug registers:
>   */
> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> index 56163e2124cf..c3028b4df85f 100644
> --- a/arch/x86/kernel/head_64.S
> +++ b/arch/x86/kernel/head_64.S
> @@ -355,8 +355,7 @@ SYM_INNER_LABEL(common_startup_64, SYM_L_LOCAL)
>
>         /* Set up %gs.
>          *
> -        * The base of %gs always points to fixed_percpu_data. If the
> -        * stack protector canary is enabled, it is located at %gs:40.
> +        * The base of %gs always points to fixed_percpu_data.
>          * Note that, on SMP, the boot cpu uses init data section until
>          * the per cpu areas are set up.
>          */
> diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S
> index 758bcd47b72d..ae4672ea00bb 100644
> --- a/arch/x86/xen/xen-head.S
> +++ b/arch/x86/xen/xen-head.S
> @@ -53,8 +53,7 @@ SYM_CODE_START(startup_xen)
>
>         /* Set up %gs.
>          *
> -        * The base of %gs always points to fixed_percpu_data.  If the
> -        * stack protector canary is enabled, it is located at %gs:40.
> +        * The base of %gs always points to fixed_percpu_data.
>          * Note that, on SMP, the boot cpu uses init data section until
>          * the per cpu areas are set up.
>          */
> --
> 2.47.0
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ