[PATCH 0/2] Fixups for fcf-protection

[PATCH 0/2] Fixups for fcf-protection

[Jason Andryuk]

Two patches to fix building with a cf-protection toolchain.  The first
handles the case where the compiler fails to run with "error:
‘-mindirect-branch’ and ‘-fcf-protection’ are not compatible".

The second fixes a runtime error that prevented Xen booting in legacy
mode.

I still haven't figured out exactly what is wrong with rombios and/or
ipxe.

Jason Andryuk (2):
  xen/x86: Disable fcf-protection when necessary to build
  x86/boot: Drop .note.gnu.properties in build32.lds

 xen/arch/x86/arch.mk          | 11 ++++++++++-
 xen/arch/x86/boot/build32.lds |  5 +++++
 2 files changed, 15 insertions(+), 1 deletion(-)

-- 
2.25.1

[PATCH 1/2] xen/x86: Disable fcf-protection when necessary to build

[Jason Andryuk]

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

Detect this incompatible combination, and add -fcf-protection=none to
allow the build to succeed.  To actually generated the error, the
compiled program must include a function.

CC: Stefan Bader <stefan.bader@canonical.com>
Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
 xen/arch/x86/arch.mk | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/arch.mk b/xen/arch/x86/arch.mk
index 2a51553edb..3aa6ce521a 100644
--- a/xen/arch/x86/arch.mk
+++ b/xen/arch/x86/arch.mk
@@ -63,7 +63,16 @@ CFLAGS += -mno-red-zone -fpic -fno-asynchronous-unwind-tables
 CFLAGS += -mno-sse $(call cc-option,$(CC),-mskip-rax-setup)
 
 # Compile with thunk-extern, indirect-branch-register if avaiable.
-CFLAGS-$(CONFIG_INDIRECT_THUNK) += -mindirect-branch=thunk-extern
+# Some versions of gcc error: "‘-mindirect-branch’ and ‘-fcf-protection’ are
+# not compatible".  For those, we need to disable cf-protection with
+# -fcf-protection=none
+cc-mindirect-branch = $(shell if test -n "`echo 'void foo(void) {};' | \
+      LANG=C $(CC) -mindirect-branch=thunk-extern -S -o /dev/null -x c - 2>&1 | \
+      grep -- '-mindirect-branch.*-fcf-protection.*are not compatible' -`"; \
+    then echo "-mindirect-branch=thunk-extern -fcf-protection=none"; \
+    else echo "-mindirect-branch=thunk-extern"; fi ;)
+
+CFLAGS-$(CONFIG_INDIRECT_THUNK) += $(call cc-mindirect-branch)
 CFLAGS-$(CONFIG_INDIRECT_THUNK) += -mindirect-branch-register
 CFLAGS-$(CONFIG_INDIRECT_THUNK) += -fno-jump-tables
 
-- 
2.25.1

[PATCH 2/2] x86/boot: Drop .note.gnu.properties in build32.lds

[Jason Andryuk]

reloc.S and cmdline.S as arrays of executable bytes for inclusion in
head.S generated from compiled object files.  Object files generated by
an -fcf-protection toolchain include a .note.gnu.property section.  The
way reloc.S and cmdline.S are generated, the bytes of .note.gnu.property
become the start of the .S files.  When head.S calls reloc or
cmdline_parse_early, those note bytes are executed instead of the
intended .text section.  This results in an early crash in reloc.

Discard the .note.gnu.property section when linking to avoid the extra
bytes.

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

CC: Stefan Bader <stefan.bader@canonical.com>
Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
 xen/arch/x86/boot/build32.lds | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/xen/arch/x86/boot/build32.lds b/xen/arch/x86/boot/build32.lds
index da35aee910..0f69765579 100644
--- a/xen/arch/x86/boot/build32.lds
+++ b/xen/arch/x86/boot/build32.lds
@@ -48,5 +48,10 @@ SECTIONS
          * Please check build32.mk for more details.
          */
         /* *(.got.plt) */
+        /*
+         * The note will end up before the .text section and be called
+         * incorrectly as instructions.
+         */
+        *(.note.gnu.property)
   }
 }
-- 
2.25.1

Re: [PATCH 0/2] Fixups for fcf-protection

[Stefan Bader]

On 12.05.20 05:39, Jason Andryuk wrote:
> Two patches to fix building with a cf-protection toolchain.  The first
> handles the case where the compiler fails to run with "error:
> ‘-mindirect-branch’ and ‘-fcf-protection’ are not compatible".
> 
> The second fixes a runtime error that prevented Xen booting in legacy
> mode.

That might be better than just disabling fcf-protection as well (which was done
in Ubuntu lacking a better solution).

Not sure it was already hit but that additional .note section breaks the build
of the emulator as generated headers become gigantic:

https://git.launchpad.net/ubuntu/+source/xen/tree/debian/patches/1001-strip-note-gnu-property.patch?h=ubuntu/focal

-Stefan
> 
> I still haven't figured out exactly what is wrong with rombios and/or
> ipxe.
> 
> Jason Andryuk (2):
>   xen/x86: Disable fcf-protection when necessary to build
>   x86/boot: Drop .note.gnu.properties in build32.lds
> 
>  xen/arch/x86/arch.mk          | 11 ++++++++++-
>  xen/arch/x86/boot/build32.lds |  5 +++++
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 

Re: [PATCH 2/2] x86/boot: Drop .note.gnu.properties in build32.lds

[Jan Beulich]

On 12.05.2020 05:39, Jason Andryuk wrote:
> reloc.S and cmdline.S as arrays of executable bytes for inclusion in
> head.S generated from compiled object files.  Object files generated by
> an -fcf-protection toolchain include a .note.gnu.property section.  The
> way reloc.S and cmdline.S are generated, the bytes of .note.gnu.property
> become the start of the .S files.  When head.S calls reloc or
> cmdline_parse_early, those note bytes are executed instead of the
> intended .text section.  This results in an early crash in reloc.

I may be misremembering, but I vaguely recall some similar change
suggestion. What I'm missing here is some form of statement as to
whether this is legitimate tool chain behavior, or a bug, and
hence whether this is a fix or a workaround.

> Discard the .note.gnu.property section when linking to avoid the extra
> bytes.

If we go this route (and if, as per above, I'm misremembering,
meaning we didn't reject such a change earlier on), why would we
not strip .note and .note.* in one go?

> 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

How's this related to the change here?

Jan

Re: [PATCH 2/2] x86/boot: Drop .note.gnu.properties in build32.lds

[Andrew Cooper]

On 12/05/2020 16:32, Jan Beulich wrote:
> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
>
> On 12.05.2020 05:39, Jason Andryuk wrote:
>> reloc.S and cmdline.S as arrays of executable bytes for inclusion in
>> head.S generated from compiled object files.  Object files generated by
>> an -fcf-protection toolchain include a .note.gnu.property section.  The
>> way reloc.S and cmdline.S are generated, the bytes of .note.gnu.property
>> become the start of the .S files.  When head.S calls reloc or
>> cmdline_parse_early, those note bytes are executed instead of the
>> intended .text section.  This results in an early crash in reloc.
> I may be misremembering, but I vaguely recall some similar change
> suggestion. What I'm missing here is some form of statement as to
> whether this is legitimate tool chain behavior, or a bug, and
> hence whether this is a fix or a workaround.

The linker is free to position unreferenced sections anywhere it wishes.

It is deeply unhelpful behaviour, but neither Binutils nor Clang
developers think it is something wanting fixing.

One option might be to use --orphan-handling=error so unexpected
toolchain behaviour breaks the build, or in this case perhaps =discard
might be better.

>> Discard the .note.gnu.property section when linking to avoid the extra
>> bytes.
> If we go this route (and if, as per above, I'm misremembering,
> meaning we didn't reject such a change earlier on), why would we
> not strip .note and .note.* in one go?
>
>> 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
> How's this related to the change here?

I think there is a bit of confusion as to exactly what is going on.

Ubuntu defaults -fcf-protection to enabled, which has a side effect of
turning on CET, which inserts ENDBR{32,64} instructions and generates
.note.gnu.properties indicating that the binary is CET-IBT compatible.

ENDBR* instructions come from the Hint Nop space so are safe on older
processors, but do ultimately add to binary bloat.  It also occurs to me
that it likely breaks livepath.

The reason Xen fails to boot is purely to do with the position of
.note.gnu.properties, not the ENDBR* instructions.

~Andrew

Re: [PATCH 2/2] x86/boot: Drop .note.gnu.properties in build32.lds

[Jan Beulich]

On 12.05.2020 17:58, Andrew Cooper wrote:
> On 12/05/2020 16:32, Jan Beulich wrote:
>> On 12.05.2020 05:39, Jason Andryuk wrote:
>>> Discard the .note.gnu.property section when linking to avoid the extra
>>> bytes.
>> If we go this route (and if, as per above, I'm misremembering,
>> meaning we didn't reject such a change earlier on), why would we
>> not strip .note and .note.* in one go?
>>
>>> 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
>> How's this related to the change here?
> 
> I think there is a bit of confusion as to exactly what is going on.
> 
> Ubuntu defaults -fcf-protection to enabled, which has a side effect of
> turning on CET, which inserts ENDBR{32,64} instructions and generates
> .note.gnu.properties indicating that the binary is CET-IBT compatible.

I.e. in principle this 2nd patch wouldn't be necessary at all if
we forced -fcf-protection=none unilaterally, and provided build32.mk
properly inherits CFLAGS. Discarding note sections may still be
a desirable thing to do though ...

Jan

Re: [PATCH 2/2] x86/boot: Drop .note.gnu.properties in build32.lds

[Andrew Cooper]

On 12/05/2020 17:11, Jan Beulich wrote:
> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
>
> On 12.05.2020 17:58, Andrew Cooper wrote:
>> On 12/05/2020 16:32, Jan Beulich wrote:
>>> On 12.05.2020 05:39, Jason Andryuk wrote:
>>>> Discard the .note.gnu.property section when linking to avoid the extra
>>>> bytes.
>>> If we go this route (and if, as per above, I'm misremembering,
>>> meaning we didn't reject such a change earlier on), why would we
>>> not strip .note and .note.* in one go?
>>>
>>>> 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
>>> How's this related to the change here?
>> I think there is a bit of confusion as to exactly what is going on.
>>
>> Ubuntu defaults -fcf-protection to enabled, which has a side effect of
>> turning on CET, which inserts ENDBR{32,64} instructions and generates
>> .note.gnu.properties indicating that the binary is CET-IBT compatible.
> I.e. in principle this 2nd patch wouldn't be necessary at all if
> we forced -fcf-protection=none unilaterally, and provided build32.mk
> properly inherits CFLAGS. Discarding note sections may still be
> a desirable thing to do though ...

Even if we disable unilaterally for now, we'll still want to re-enable
at some point, so this will come and bite us again.

I'm currently testing Ubuntu's default behaviour in combination with
livepatch, because I bet there are further breakages to be found.

~Andrew

Re: [PATCH 2/2] x86/boot: Drop .note.gnu.properties in build32.lds

[Jason Andryuk]

On Tue, May 12, 2020 at 11:58 AM Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>
> On 12/05/2020 16:32, Jan Beulich wrote:
> > [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
> >
> > On 12.05.2020 05:39, Jason Andryuk wrote:
> >> reloc.S and cmdline.S as arrays of executable bytes for inclusion in
> >> head.S generated from compiled object files.  Object files generated by
> >> an -fcf-protection toolchain include a .note.gnu.property section.  The
> >> way reloc.S and cmdline.S are generated, the bytes of .note.gnu.property
> >> become the start of the .S files.  When head.S calls reloc or
> >> cmdline_parse_early, those note bytes are executed instead of the
> >> intended .text section.  This results in an early crash in reloc.
> > I may be misremembering, but I vaguely recall some similar change
> > suggestion. What I'm missing here is some form of statement as to
> > whether this is legitimate tool chain behavior, or a bug, and
> > hence whether this is a fix or a workaround.
>
> The linker is free to position unreferenced sections anywhere it wishes.
>
> It is deeply unhelpful behaviour, but neither Binutils nor Clang
> developers think it is something wanting fixing.
>
> One option might be to use --orphan-handling=error so unexpected
> toolchain behaviour breaks the build, or in this case perhaps =discard
> might be better.

The toolchain uses .note.gnu.property to flag object files as
supporting Intel CET (Control-flow Enforcement Technology) enabled by
-fcf-protection.  The linker/loader uses the note to know if CET
should be enabled or disabled.  CET can only be enabled if the
application and all libraries support it.

So it's legitimate to flag compiled objects with .note.gnu.property.
The .S files generated by build32.mk are .. interesting.  It seems
like they should only be the runtime code & data, so we don't want the
.note in there.  So I guess this is a workaround for how the .S files
are generated?  My first attempt added -R .note.gnu.property, fyi.

I'm not familiar with the linker options Andrew references, to know
how usable they are off the top of my head.

-fcf-protection=none could also be specified in CFLAGS in build32.mk
to avoid generating the note.

> >> Discard the .note.gnu.property section when linking to avoid the extra
> >> bytes.
> > If we go this route (and if, as per above, I'm misremembering,
> > meaning we didn't reject such a change earlier on), why would we
> > not strip .note and .note.* in one go?

Maybe?  I made the conservative change since they weren't previously discarded.

> >> 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
> > How's this related to the change here?
>
> I think there is a bit of confusion as to exactly what is going on.
>
> Ubuntu defaults -fcf-protection to enabled, which has a side effect of
> turning on CET, which inserts ENDBR{32,64} instructions and generates
> .note.gnu.properties indicating that the binary is CET-IBT compatible.
>
> ENDBR* instructions come from the Hint Nop space so are safe on older
> processors, but do ultimately add to binary bloat.  It also occurs to me
> that it likely breaks livepath.
>
> The reason Xen fails to boot is purely to do with the position of
> .note.gnu.properties, not the ENDBR* instructions.

Yes.

I referenced Stefan's bug since it specifically called out build32.mk
as problematic even after supplying -fcf-protection=none for a
hypervisor build.  I was trying to give credit and reference a helpful
bug entry.  I don't know how Xen handles such things, but I am fine
dropping it.

Regards,
Jason

Re: [PATCH 0/2] Fixups for fcf-protection

[Andrew Cooper]

On 12/05/2020 08:17, Stefan Bader wrote:
> On 12.05.20 05:39, Jason Andryuk wrote:
>> Two patches to fix building with a cf-protection toolchain.  The first
>> handles the case where the compiler fails to run with "error:
>> ‘-mindirect-branch’ and ‘-fcf-protection’ are not compatible".
>>
>> The second fixes a runtime error that prevented Xen booting in legacy
>> mode.
> That might be better than just disabling fcf-protection as well (which was done
> in Ubuntu lacking a better solution).

It is a GCC bug

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

Fixed for 10, and 9.4

>
> Not sure it was already hit but that additional .note section breaks the build
> of the emulator as generated headers become gigantic:
>
> https://git.launchpad.net/ubuntu/+source/xen/tree/debian/patches/1001-strip-note-gnu-property.patch?h=ubuntu/focal

4.6G of notes?!?  That is surely a toolchain bug.

~Andrew

Re: [PATCH 2/2] x86/boot: Drop .note.gnu.properties in build32.lds

[Andrew Cooper]

On 12/05/2020 17:17, Jason Andryuk wrote:
> On Tue, May 12, 2020 at 11:58 AM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 12/05/2020 16:32, Jan Beulich wrote:
>>> On 12.05.2020 05:39, Jason Andryuk wrote:
>>>> reloc.S and cmdline.S as arrays of executable bytes for inclusion in
>>>> head.S generated from compiled object files.  Object files generated by
>>>> an -fcf-protection toolchain include a .note.gnu.property section.  The
>>>> way reloc.S and cmdline.S are generated, the bytes of .note.gnu.property
>>>> become the start of the .S files.  When head.S calls reloc or
>>>> cmdline_parse_early, those note bytes are executed instead of the
>>>> intended .text section.  This results in an early crash in reloc.
>>> I may be misremembering, but I vaguely recall some similar change
>>> suggestion. What I'm missing here is some form of statement as to
>>> whether this is legitimate tool chain behavior, or a bug, and
>>> hence whether this is a fix or a workaround.
>> The linker is free to position unreferenced sections anywhere it wishes.
>>
>> It is deeply unhelpful behaviour, but neither Binutils nor Clang
>> developers think it is something wanting fixing.
>>
>> One option might be to use --orphan-handling=error so unexpected
>> toolchain behaviour breaks the build, or in this case perhaps =discard
>> might be better.
> The toolchain uses .note.gnu.property to flag object files as
> supporting Intel CET (Control-flow Enforcement Technology) enabled by
> -fcf-protection.  The linker/loader uses the note to know if CET
> should be enabled or disabled.  CET can only be enabled if the
> application and all libraries support it.

Right, except we're a kernel here (rather than userspace), so the
practicalities are different.

> So it's legitimate to flag compiled objects with .note.gnu.property.
> The .S files generated by build32.mk are .. interesting.  It seems
> like they should only be the runtime code & data, so we don't want the
> .note in there.

Yes...  Self-hosted relocatable 32bit code is tricky at the best of
times, and this is a very good example of how not to do it.

I've got a plan to get rid of it completely, but it needs a bit more of
the "switch to kbuild" series to go in first.

>   So I guess this is a workaround for how the .S files
> are generated?  My first attempt added -R .note.gnu.property, fyi.
>
> I'm not familiar with the linker options Andrew references, to know
> how usable they are off the top of my head.
>
> -fcf-protection=none could also be specified in CFLAGS in build32.mk
> to avoid generating the note.
>
>>>> Discard the .note.gnu.property section when linking to avoid the extra
>>>> bytes.
>>> If we go this route (and if, as per above, I'm misremembering,
>>> meaning we didn't reject such a change earlier on), why would we
>>> not strip .note and .note.* in one go?
> Maybe?  I made the conservative change since they weren't previously discarded.
>
>>>> 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
>>> How's this related to the change here?
>> I think there is a bit of confusion as to exactly what is going on.
>>
>> Ubuntu defaults -fcf-protection to enabled, which has a side effect of
>> turning on CET, which inserts ENDBR{32,64} instructions and generates
>> .note.gnu.properties indicating that the binary is CET-IBT compatible.
>>
>> ENDBR* instructions come from the Hint Nop space so are safe on older
>> processors, but do ultimately add to binary bloat.  It also occurs to me
>> that it likely breaks livepath.
>>
>> The reason Xen fails to boot is purely to do with the position of
>> .note.gnu.properties, not the ENDBR* instructions.
> Yes.
>
> I referenced Stefan's bug since it specifically called out build32.mk
> as problematic even after supplying -fcf-protection=none for a
> hypervisor build.  I was trying to give credit and reference a helpful
> bug entry.  I don't know how Xen handles such things, but I am fine
> dropping it.

Typically a Reported-by: $PERSON <$EMAIL> tag, but frankly it would have
been nice if anyone had posted any of these problems to xen-devel 6
months ago when it was first discovered to be a problem.

So far, we're at one definite (and fixed) toolchain bug, one
obvious-but-not-yet-debugged toolchain bug, a robustness fix in Xen for
the 32bit mess, and overriding of a system default, and thats before
getting to the iPXE issues.

~Andrew

Re: [PATCH 0/2] Fixups for fcf-protection

[Stefan Bader]

On 12.05.20 20:47, Andrew Cooper wrote:
> On 12/05/2020 08:17, Stefan Bader wrote:
>> On 12.05.20 05:39, Jason Andryuk wrote:
>>> Two patches to fix building with a cf-protection toolchain.  The first
>>> handles the case where the compiler fails to run with "error:
>>> ‘-mindirect-branch’ and ‘-fcf-protection’ are not compatible".
>>>
>>> The second fixes a runtime error that prevented Xen booting in legacy
>>> mode.
>> That might be better than just disabling fcf-protection as well (which was done
>> in Ubuntu lacking a better solution).
> 
> It is a GCC bug
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93654
> 
> Fixed for 10, and 9.4

Ah, thanks. And yes, should have reported sooner but, as always, one runs into
those things while in a hurry, with not enough time. We were glad to have
something that did work somehow.

> 
>>
>> Not sure it was already hit but that additional .note section breaks the build
>> of the emulator as generated headers become gigantic:
>>
>> https://git.launchpad.net/ubuntu/+source/xen/tree/debian/patches/1001-strip-note-gnu-property.patch?h=ubuntu/focal
> 
> 4.6G of notes?!?  That is surely a toolchain bug.

No, sorry if that was unclear. The .notes themselves are just about some Kb or
so per object file. Problem is that each object file gets converted into a hex
array header file. And this does multiply the resulting header file sizes.
So the .h files generated are 4.6G in size. And there were a couple of those,
all included into one .c file. Which ended in the compiler running out of memory
on a 32GB system.

> 
> ~Andrew
> 

Re: [PATCH 0/2] Fixups for fcf-protection

[Jan Beulich]

On 13.05.2020 07:41, Stefan Bader wrote:
> On 12.05.20 20:47, Andrew Cooper wrote:
>> On 12/05/2020 08:17, Stefan Bader wrote:
>>> Not sure it was already hit but that additional .note section breaks the build
>>> of the emulator as generated headers become gigantic:
>>>
>>> https://git.launchpad.net/ubuntu/+source/xen/tree/debian/patches/1001-strip-note-gnu-property.patch?h=ubuntu/focal
>>
>> 4.6G of notes?!?  That is surely a toolchain bug.
> 
> No, sorry if that was unclear. The .notes themselves are just about some Kb or
> so per object file. Problem is that each object file gets converted into a hex
> array header file. And this does multiply the resulting header file sizes.
> So the .h files generated are 4.6G in size. And there were a couple of those,
> all included into one .c file. Which ended in the compiler running out of memory
> on a 32GB system.

But as per the link above it's still 3Mb notes per object,
which still seems insane.

Jan