x86/build32: Discard all orphaned sections
diff mbox series

Message ID 20200512191108.6461-1-andrew.cooper3@citrix.com
State New
Headers show
Series
  • x86/build32: Discard all orphaned sections
Related show

Commit Message

Andrew Cooper May 12, 2020, 7:11 p.m. UTC
Linkers may put orphaned sections ahead of .text, which breaks the calling
requirements.  A concrete example is Ubuntu's GCC-9 default of enabling
-fcf-protection which causes us to try and execute .note.gnu.properties during
Xen's boot.

Put .got.plt in its own section as it specifically needs preserving from the
linkers point of view, and discard everything else.  This will hopefully be
more robust to other unexpected toolchain properties.

Fixes boot from an Ubuntu build of Xen.

Reported-by: Jason Andryuk <jandryuk@gmail.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Jason Andryuk <jandryuk@gmail.com>
CC: Stefan Bader <stefan.bader@canonical.com>
---
 xen/arch/x86/boot/build32.lds | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

Jason Andryuk May 13, 2020, 2:27 a.m. UTC | #1
On Tue, May 12, 2020 at 3:11 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> Linkers may put orphaned sections ahead of .text, which breaks the calling
> requirements.  A concrete example is Ubuntu's GCC-9 default of enabling
> -fcf-protection which causes us to try and execute .note.gnu.properties during
> Xen's boot.
>
> Put .got.plt in its own section as it specifically needs preserving from the
> linkers point of view, and discard everything else.  This will hopefully be
> more robust to other unexpected toolchain properties.
>
> Fixes boot from an Ubuntu build of Xen.
>
> Reported-by: Jason Andryuk <jandryuk@gmail.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Tested-by: Jason Andryuk <jandryuk@gmail.com>

Thanks
Jan Beulich May 13, 2020, 9:13 a.m. UTC | #2
On 12.05.2020 21:11, Andrew Cooper wrote:
> @@ -47,6 +47,14 @@ SECTIONS
>           *
>           * Please check build32.mk for more details.
>           */
> -        /* *(.got.plt) */
> +        *(.got.plt)
> +  }
> +
> +  /DISCARD/ : {
> +        /*
> +         * Discard everything else, to prevent linkers from putting
> +         * orphaned sections ahead of .text, which needs to be first.
> +         */
> +        *(*)
>    }
>  }

To be honest I'm not sure if this isn't going too far. Much
depends on what would happen if a new real section appeared
that needs retaining. Granted the linker may then once again
put it at the beginning of the image, and we'll be screwed
again, just like we'll be screwed if a section gets discarded
by mistake. But it would be really nice if we had a way to
flag the need to play with the linker script. Hence perhaps
on new enough tool chains we indeed may want to make use of
--orphan-handling= ? And then discard just .note and .note.*
here?

Jan
Andrew Cooper May 13, 2020, 3 p.m. UTC | #3
On 13/05/2020 10:13, 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 21:11, Andrew Cooper wrote:
>> @@ -47,6 +47,14 @@ SECTIONS
>>           *
>>           * Please check build32.mk for more details.
>>           */
>> -        /* *(.got.plt) */
>> +        *(.got.plt)
>> +  }
>> +
>> +  /DISCARD/ : {
>> +        /*
>> +         * Discard everything else, to prevent linkers from putting
>> +         * orphaned sections ahead of .text, which needs to be first.
>> +         */
>> +        *(*)
>>    }
>>  }
> To be honest I'm not sure if this isn't going too far. Much
> depends on what would happen if a new real section appeared
> that needs retaining.

Anything which is important enough will result in a linker error.

> Granted the linker may then once again
> put it at the beginning of the image, and we'll be screwed
> again, just like we'll be screwed if a section gets discarded
> by mistake.

This way is more likely to result in a build failure than an inability
to boot the resulting build of Xen.

> But it would be really nice if we had a way to
> flag the need to play with the linker script. Hence perhaps
> on new enough tool chains we indeed may want to make use of
> --orphan-handling= ? And then discard just .note and .note.*
> here?

The only valid option would be =error, but experimenting with that yields

ld: error: unplaced orphan section `.comment' from `cmdline.o'
ld: error: unplaced orphan section `.note.GNU-stack' from `cmdline.o'
ld: error: unplaced orphan section `.note.gnu.property' from `cmdline.o'
ld: error: unplaced orphan section `.rel.got' from `cmdline.o'
ld: error: unplaced orphan section `.got' from `cmdline.o'
ld: error: unplaced orphan section `.got.plt' from `cmdline.o'
ld: error: unplaced orphan section `.iplt' from `cmdline.o'
ld: error: unplaced orphan section `.rel.iplt' from `cmdline.o'
ld: error: unplaced orphan section `.igot.plt' from `cmdline.o'

which I think is going to get us massively bogged down in toolchain
specifics.  I'm not entirely convinced this would be a good move.

~Andrew
Jan Beulich May 13, 2020, 3:15 p.m. UTC | #4
On 13.05.2020 17:00, Andrew Cooper wrote:
> On 13/05/2020 10:13, 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 21:11, Andrew Cooper wrote:
>>> @@ -47,6 +47,14 @@ SECTIONS
>>>           *
>>>           * Please check build32.mk for more details.
>>>           */
>>> -        /* *(.got.plt) */
>>> +        *(.got.plt)
>>> +  }
>>> +
>>> +  /DISCARD/ : {
>>> +        /*
>>> +         * Discard everything else, to prevent linkers from putting
>>> +         * orphaned sections ahead of .text, which needs to be first.
>>> +         */
>>> +        *(*)
>>>    }
>>>  }
>> To be honest I'm not sure if this isn't going too far. Much
>> depends on what would happen if a new real section appeared
>> that needs retaining.
> 
> Anything which is important enough will result in a linker error.
> 
>> Granted the linker may then once again
>> put it at the beginning of the image, and we'll be screwed
>> again, just like we'll be screwed if a section gets discarded
>> by mistake.
> 
> This way is more likely to result in a build failure than an inability
> to boot the resulting build of Xen.
> 
>> But it would be really nice if we had a way to
>> flag the need to play with the linker script. Hence perhaps
>> on new enough tool chains we indeed may want to make use of
>> --orphan-handling= ? And then discard just .note and .note.*
>> here?
> 
> The only valid option would be =error, but experimenting with that yields
> 
> ld: error: unplaced orphan section `.comment' from `cmdline.o'
> ld: error: unplaced orphan section `.note.GNU-stack' from `cmdline.o'
> ld: error: unplaced orphan section `.note.gnu.property' from `cmdline.o'
> ld: error: unplaced orphan section `.rel.got' from `cmdline.o'
> ld: error: unplaced orphan section `.got' from `cmdline.o'
> ld: error: unplaced orphan section `.got.plt' from `cmdline.o'
> ld: error: unplaced orphan section `.iplt' from `cmdline.o'
> ld: error: unplaced orphan section `.rel.iplt' from `cmdline.o'
> ld: error: unplaced orphan section `.igot.plt' from `cmdline.o'
> 
> which I think is going to get us massively bogged down in toolchain
> specifics.  I'm not entirely convinced this would be a good move.

That's ugly indeed; especially the .rel.* sections are worrying to
appear there. Hence patch as is
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan
Andrew Cooper May 13, 2020, 7:14 p.m. UTC | #5
On 13/05/2020 16:15, Jan Beulich wrote:
>>> But it would be really nice if we had a way to
>>> flag the need to play with the linker script. Hence perhaps
>>> on new enough tool chains we indeed may want to make use of
>>> --orphan-handling= ? And then discard just .note and .note.*
>>> here?
>> The only valid option would be =error, but experimenting with that yields
>>
>> ld: error: unplaced orphan section `.comment' from `cmdline.o'
>> ld: error: unplaced orphan section `.note.GNU-stack' from `cmdline.o'
>> ld: error: unplaced orphan section `.note.gnu.property' from `cmdline.o'
>> ld: error: unplaced orphan section `.rel.got' from `cmdline.o'
>> ld: error: unplaced orphan section `.got' from `cmdline.o'
>> ld: error: unplaced orphan section `.got.plt' from `cmdline.o'
>> ld: error: unplaced orphan section `.iplt' from `cmdline.o'
>> ld: error: unplaced orphan section `.rel.iplt' from `cmdline.o'
>> ld: error: unplaced orphan section `.igot.plt' from `cmdline.o'
>>
>> which I think is going to get us massively bogged down in toolchain
>> specifics.  I'm not entirely convinced this would be a good move.
> That's ugly indeed; especially the .rel.* sections are worrying to
> appear there.

What is even more curious, most of them don't exist in cmdine.o

Section Headers:
  [Nr] Name              Type            Addr     Off    Size   ES Flg
Lk Inf Al
  [ 0]                   NULL            00000000 000000 000000 00     
0   0  0
  [ 1] .group            GROUP           00000000 000034 000008 04    
14  38  4
  [ 2] .group            GROUP           00000000 00003c 000008 04    
14  40  4
  [ 3] .text             PROGBITS        00000000 000044 00094a 00  AX 
0   0  1
  [ 4] .rel.text         REL             00000000 000e88 0001e8 08   I
14   3  4
  [ 5] .data             PROGBITS        00000000 00098e 000000 00  WA 
0   0  1
  [ 6] .bss              NOBITS          00000000 00098e 000000 00  WA 
0   0  1
  [ 7] .rodata           PROGBITS        00000000 000990 0000f3 00   A 
0   0  4
  [ 8] .rel.rodata       REL             00000000 001070 000120 08   I
14   7  4
  [ 9] .text.__x86.get_pc_thunk.ax PROGBITS        00000000 000a83
000004 00 AXG  0   0  1
  [10] .text.__x86.get_pc_thunk.bx PROGBITS        00000000 000a87
000004 00 AXG  0   0  1
  [11] .comment          PROGBITS        00000000 000a8b 00002d 01  MS 
0   0  1
  [12] .note.GNU-stack   PROGBITS        00000000 000ab8 000000 00     
0   0  1
  [13] .note.gnu.property NOTE            00000000 000ab8 00001c 00   A 
0   0  4
  [14] .symtab           SYMTAB          00000000 000ad4 000290 10    
15  36  4
  [15] .strtab           STRTAB          00000000 000d64 000124 00     
0   0  1
  [16] .shstrtab         STRTAB          00000000 001190 0000a7 00     
0   0  1

I suspect they are inserted by default as part of processing the
relocations, and end up empty.

With =warn rather than =error, we instead get

ld: warning: orphan section `.comment' from `cmdline.o' being placed in
section `.comment'
ld: warning: orphan section `.note.GNU-stack' from `cmdline.o' being
placed in section `.note.GNU-stack'
ld: warning: orphan section `.note.gnu.property' from `cmdline.o' being
placed in section `.note.gnu.property'
ld: warning: orphan section `.rel.got' from `cmdline.o' being placed in
section `.rel.dyn'
ld: warning: orphan section `.got' from `cmdline.o' being placed in
section `.got'
ld: warning: orphan section `.got.plt' from `cmdline.o' being placed in
section `.got.plt'
ld: warning: orphan section `.iplt' from `cmdline.o' being placed in
section `.iplt'
ld: warning: orphan section `.rel.iplt' from `cmdline.o' being placed in
section `.rel.dyn'
ld: warning: orphan section `.igot.plt' from `cmdline.o' being placed in
section `.igot.plt'

and

Section Headers:
  [Nr] Name              Type            Addr     Off    Size   ES Flg
Lk Inf Al
  [ 0]                   NULL            00000000 000000 000000 00     
0   0  0
  [ 1] .note.gnu.property NOTE            00000000 0000b4 00001c 00   A 
0   0  4
  [ 2] .text             PROGBITS        0000001c 0000d0 000a47 00 WAX 
0   0  4
  [ 3] .got.plt          PROGBITS        00000a64 000b18 00000c 04  WA 
0   0  4
  [ 4] .comment          PROGBITS        00000000 000b24 00002c 01  MS 
0   0  1
  [ 5] .symtab           SYMTAB          00000000 000b50 000230 10     
6  31  4
  [ 6] .strtab           STRTAB          00000000 000d80 000124 00     
0   0  1
  [ 7] .shstrtab         STRTAB          00000000 000ea4 000046 00     
0   0  1

in cmdline.lnk, so the .rel.* sections have been dropped overall.  I
think the =error logic is simply at an unhelpful position during processing.

> Hence patch as is Acked-by: Jan Beulich <jbeulich@suse.com>

Thanks,

As I say, I have a plan to replace all of this completely when a bit
more of kbuild is in place.

~Andrew

Patch
diff mbox series

diff --git a/xen/arch/x86/boot/build32.lds b/xen/arch/x86/boot/build32.lds
index da35aee910..97454b40ff 100644
--- a/xen/arch/x86/boot/build32.lds
+++ b/xen/arch/x86/boot/build32.lds
@@ -31,7 +31,7 @@  SECTIONS
         *(.bss.*)
   }
 
-  /DISCARD/ : {
+  .got.plt : {
         /*
          * PIC/PIE executable contains .got.plt section even if it is not linked
          * with dynamic libraries. In such case it is just placeholder for
@@ -47,6 +47,14 @@  SECTIONS
          *
          * Please check build32.mk for more details.
          */
-        /* *(.got.plt) */
+        *(.got.plt)
+  }
+
+  /DISCARD/ : {
+        /*
+         * Discard everything else, to prevent linkers from putting
+         * orphaned sections ahead of .text, which needs to be first.
+         */
+        *(*)
   }
 }