| Message ID | 20200512033948.3507-3-jandryuk@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixups for fcf-protection | expand |
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
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
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
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
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
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
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) } }
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(+)