Re: [PATCH v3 0/3] Add stack protector

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.12.2024 02:17, Andrew Cooper wrote:
> (With the knowledge that this is a disputed Kconfig pattern, and will
> need rebasing), the way I want this to work is simply:
> 
> diff --git a/xen/Makefile b/xen/Makefile
> index 0de0101fd0bf..5d0a88fb3c3f 100644
> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -434,6 +434,9 @@ endif
>  
>  ifeq ($(CONFIG_STACK_PROTECTOR),y)
>  CFLAGS += -fstack-protector
> +ifeq ($(CONFIG_X86),y)
> +CFLAGS += -mstack-protector-guard=global
> +endif
>  else
>  CFLAGS += -fno-stack-protector
>  endif
> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> index 9cdd04721afa..7951ca908b36 100644
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -28,6 +28,7 @@ config X86
>         select HAS_PCI_MSI
>         select HAS_PIRQ
>         select HAS_SCHED_GRANULARITY
> +       select HAS_STACK_PROTECTOR if
> $(cc-option,-mstack-protector-guard=global)
>         select HAS_UBSAN
>         select HAS_VMAP
>         select HAS_VPCI if HVM
> 
> 
> 
> Sadly, it doesn't build.  I get a handful of:
> 
> prelink.o: in function `cmdline_parse':
> /home/andrew/xen.git/xen/common/kernel.c:216:(.init.text+0x20f2): failed
> to convert GOTPCREL relocation against '__stack_chk_guard'; relink with
> --no-relax
> /home/andrew/xen.git/xen/common/kernel.c:230:(.init.text+0x246f): failed
> to convert GOTPCREL relocation against '__stack_chk_guard'; relink with
> --no-relax
> 
> which is more toolchain-whispering than I feel like doing tonight.

Imo the root of the problem is that the compiler doesn't itself mark
__stack_chk_guard hidden (it does so for __stack_chk_fail, albeit only for
32-bit code), and hence finds it necessary to use @gotpcrel to access the
variable. Even if the linker managed to relax all of these, it would then
still be less efficient compared to direct RIP-relative accesses.

I also can't see how we might be able to override the compiler's internal
declaration to mark it hidden (the same appears to be true for other items
the declares internally, like the retpoline thunks or even strcmp() et al).
Passing -fvisibility=hidden doesn't make a difference (just as another
data point).

Playing with -fstack-protector* flavors, I observe:
- -fstack-protector causing several failures, like you observed, oddly
  enough exclusively from __init functions,
- -fstack-protector-all and -fstack-protector-strong each causing a single
  (but respectively different) failure, for apparently random non-__init
  functions.
Taking this together it very much smells like a linker issue. I'll see
about checking there further.

Jan