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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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