xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jason Andryuk <jandryuk@gmail.com>, Wei Liu <wl@xen.org>
Cc: minios-devel@lists.xenproject.org, samuel.thibault@ens-lyon.org,
	Stefan Bader <stefan.bader@canonical.com>,
	JBeulich@suse.com, xen-devel@lists.xenproject.org
Subject: Re: [PATCH] mini-os: Avoid segfaults in tc{g,s}etattr
Date: Tue, 28 Apr 2020 12:55:03 +0100	[thread overview]
Message-ID: <c242b963-ae80-1ca0-9b4d-fe2c8f66b6a2@citrix.com> (raw)
In-Reply-To: <5ea8173d.1c69fb81.915ba.8400@mx.google.com>

On 28/04/2020 12:44, Jason Andryuk wrote:
> From: Andrew Cooper <andrew.cooper3@citrix.com>
>
> 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

Yes.  This is false.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93654

but sadly tumbleweeds.

I'll start a thread on the email list.

>
> 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@canonical.com>
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>

Sadly, this isn't really appropriate.  We specifically do want to use
both -fcf-protection and -mindirect-branch=thunk-extern together, when
GCC isn't broken.

Overriding -fcf-protection is ok but only when we're certain we've got a
buggy GCC, so that when this bug is fixed, we can return to sensible
behaviour.

~Andrew

> ---
>  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



  reply	other threads:[~2020-04-28 11:55 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-27  3:40 [PATCH] mini-os: Avoid segfaults in tc{g,s}etattr Jason Andryuk
2020-04-27  7:54 ` Samuel Thibault
2020-04-27 13:30   ` Jason Andryuk
2020-04-28 11:16     ` Wei Liu
2020-04-28 11:24       ` Andrew Cooper
2020-04-28 11:44       ` Jason Andryuk
2020-04-28 11:55         ` Andrew Cooper [this message]
2020-04-29 12:34           ` Andrew Cooper
2020-05-12 19:31             ` rombios triple fault with -fcf-protection Jason Andryuk
2020-04-28 11:18   ` [PATCH] mini-os: Avoid segfaults in tc{g,s}etattr Wei Liu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c242b963-ae80-1ca0-9b4d-fe2c8f66b6a2@citrix.com \
    --to=andrew.cooper3@citrix.com \
    --cc=JBeulich@suse.com \
    --cc=jandryuk@gmail.com \
    --cc=minios-devel@lists.xenproject.org \
    --cc=samuel.thibault@ens-lyon.org \
    --cc=stefan.bader@canonical.com \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).