Re: [PATCH] mini-os: Avoid segfaults in tc{g,s}etattr

[Jason Andryuk <jandryuk@xxxxxxxxx>]

From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Andrew Cooper wrote:
>On 28/04/2020 12:16, Wei Liu wrote:
>>>>> ---
>>>>> I can't get ioemu-stubdom to start without this.  With this, the guest
>>>>> just reboots immediately, but it does that with a non-stubdom
>>>>> device_model_version="qemu-xen-traditional" .  The same guest disk image
>>>>> (cirros 0.5.1) boots with a linux stubdom or non-stubdom Ubuntu
>>>>> qemu-system-x86_64.
>>> Ubuntu gcc-9 adds -fcf-protection by default.  Somehow that flag
>>> caused rombios (I think) to restart.  Setting -fcf-protection=none
>>> like below (probably just the EMBEDDED_EXTRA_CFLAGS part) lets rombios
>>> start properly.
>
>All it does is insert ENDBR{32,64} instructions, which are nops on older
>processors.
>
>I suspect that it is not the -fcf-protection= directly, but some change
>in alignment of a critical function.
>
>>>   The hypervisor needs it as well via
>>> EXTRA_CFLAGS_XEN_CORE=-fcf-protection=none and maybe also added to
>>> xen/arch/x86/boot/build32.mk .
>> Are you able to turn this into a proper patch? I suspect you will need
>> to test the availability of this new (?) flag.
>>
>> Also Cc Jan and Andrew because it affects hypervisor build too.
>
>I need to chase this up.  It is a GCC bug breaking the hypervisor build,
>and I'm moderately disinclined to hack around it, seeing as
>-fcf-protection is something we want in due course.
>
>The bug is that GCC falsely declares that -fcf-protection is
>incompatible with -mindirect-thunk=extern, despite me spending a week
>during the Spectre embargo period specifically arranging for the two to
>be compatible, because we knew we'd want to build retpoline-safe
>binaries which could also use CET on newer hardware.

The gcc manual states:
  "Note that -mindirect-branch=thunk-extern is incompatible with
   -fcf-protection=branch since the external thunk cannot be modified
   to disable control-flow check."

https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html

Below is what I was preparing to submit as a patch.  So, yes it hacks around
it, but it isn't messy.

---
Disable fcf-protection to build working binaries

Ubuntu gcc-9 enables -fcf-protection by default, which conflicts with
-mindirect-branch=extern and prevents building the hypervisor with
CONFIG_INDIRECT_THUNK:
xmalloc.h:81:1: error: ‘-mindirect-branch’ and ‘-fcf-protection’ are not
compatible

Stefan Bader also noticed that build32.mk requires -fcf-protection=none
or else the hypervisor will not boot.
https://bugs.launchpad.net/ubuntu/+source/gcc-9/+bug/1863260  Similarly,
rombios reboots almost immediately without -fcf-protection=none.  Both
of those can be handled by setting it in EMBEDDED_EXTRA_CFLAGS.

CC: Stefan Bader <stefan.bader@xxxxxxxxxxxxx>
Signed-off-by: Jason Andryuk <jandryuk@xxxxxxxxx>
---
 Config.mk             | 1 +
 xen/arch/x86/Rules.mk | 1 +
 2 files changed, 2 insertions(+)

diff --git a/Config.mk b/Config.mk
index 0f303c79b2..efb3d42bc4 100644
--- a/Config.mk
+++ b/Config.mk
@@ -205,6 +205,7 @@ APPEND_CFLAGS += $(foreach i, $(APPEND_INCLUDES), -I$(i))
 
 EMBEDDED_EXTRA_CFLAGS := -nopie -fno-stack-protector -fno-stack-protector-all
 EMBEDDED_EXTRA_CFLAGS += -fno-exceptions
+EMBEDDED_EXTRA_CFLAGS += -fcf-protection=none
 
 XEN_EXTFILES_URL ?= http://xenbits.xen.org/xen-extfiles
 # All the files at that location were downloaded from elsewhere on
diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk
index 4b7ab78467..c3cbae69d2 100644
--- a/xen/arch/x86/Rules.mk
+++ b/xen/arch/x86/Rules.mk
@@ -69,6 +69,7 @@ CFLAGS += -mno-sse $(call cc-option,$(CC),-mskip-rax-setup)
 ifeq ($(CONFIG_INDIRECT_THUNK),y)
 CFLAGS += -mindirect-branch=thunk-extern -mindirect-branch-register
 CFLAGS += -fno-jump-tables
+$(call cc-option-add,CFLAGS,CC,-fcf-protection=none)
 endif
 
 # If supported by the compiler, reduce stack alignment to 8 bytes. But allow
-- 
2.20.1