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

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Tue, May 12, 2020 at 11:58 AM Andrew Cooper
<andrew.cooper3@xxxxxxxxxx> 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