[GOLD,AArch64] default stack not executable

Message ID 1506025575-1559-1-git-send-email-jim.wilson@linaro.org
State New
Headers show
Series
  • [GOLD,AArch64] default stack not executable
Related show

Commit Message

Jim Wilson Sept. 21, 2017, 8:26 p.m.
For aarch64, ld.bfd makes the stack not executable when a GNU-stack note is
missing.  Note that elf_backend_default_execstack is 0 in bfd/elfnn-aarch64.c.

However, ld.gold makes the stack executable when a GNU-stack note is missing.
Note that is_default_stack_executable is true in gold/aarch64.cc.

This appears to be a bug in gold.  It also looks like 64-bit ppc gets this
wrong also, though I have not tested that.

The following patch fixes this for aarch64 by changing gold aarch64 to make
is_default_stack_executable false.  This was tested with a make check, and
there were no regressions.  It was also verified against a testcase using a
.s file with a missing GNU-stack note.

	gold/
	* aarch64.cc (Target_aarch64::aarch64_info): Set
	is_default_stack_executable to false.
---
 gold/aarch64.cc | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

-- 
2.7.4

Comments

Jim Wilson Sept. 21, 2017, 8:35 p.m. | #1
Testcase attached.

On Thu, Sep 21, 2017 at 1:26 PM, Jim Wilson <jim.wilson@linaro.org> wrote:
> For aarch64, ld.bfd makes the stack not executable when a GNU-stack note is

> missing.  Note that elf_backend_default_execstack is 0 in bfd/elfnn-aarch64.c.

>

> However, ld.gold makes the stack executable when a GNU-stack note is missing.

> Note that is_default_stack_executable is true in gold/aarch64.cc.

>

> This appears to be a bug in gold.  It also looks like 64-bit ppc gets this

> wrong also, though I have not tested that.

>

> The following patch fixes this for aarch64 by changing gold aarch64 to make

> is_default_stack_executable false.  This was tested with a make check, and

> there were no regressions.  It was also verified against a testcase using a

> .s file with a missing GNU-stack note.

>

>         gold/

>         * aarch64.cc (Target_aarch64::aarch64_info): Set

>         is_default_stack_executable to false.

> ---

>  gold/aarch64.cc | 8 ++++----

>  1 file changed, 4 insertions(+), 4 deletions(-)

>

> diff --git a/gold/aarch64.cc b/gold/aarch64.cc

> index a72e2c3..4c6e920 100644

> --- a/gold/aarch64.cc

> +++ b/gold/aarch64.cc

> @@ -3523,7 +3523,7 @@ const Target::Target_info Target_aarch64<64, false>::aarch64_info =

>    false,               // has_make_symbol

>    false,               // has_resolve

>    false,               // has_code_fill

> -  true,                        // is_default_stack_executable

> +  false,               // is_default_stack_executable

>    true,                        // can_icf_inline_merge_sections

>    '\0',                        // wrap_char

>    "/lib/ld.so.1",      // program interpreter

> @@ -3551,7 +3551,7 @@ const Target::Target_info Target_aarch64<32, false>::aarch64_info =

>    false,               // has_make_symbol

>    false,               // has_resolve

>    false,               // has_code_fill

> -  true,                        // is_default_stack_executable

> +  false,               // is_default_stack_executable

>    false,               // can_icf_inline_merge_sections

>    '\0',                        // wrap_char

>    "/lib/ld.so.1",      // program interpreter

> @@ -3579,7 +3579,7 @@ const Target::Target_info Target_aarch64<64, true>::aarch64_info =

>    false,               // has_make_symbol

>    false,               // has_resolve

>    false,               // has_code_fill

> -  true,                        // is_default_stack_executable

> +  false,               // is_default_stack_executable

>    true,                        // can_icf_inline_merge_sections

>    '\0',                        // wrap_char

>    "/lib/ld.so.1",      // program interpreter

> @@ -3607,7 +3607,7 @@ const Target::Target_info Target_aarch64<32, true>::aarch64_info =

>    false,               // has_make_symbol

>    false,               // has_resolve

>    false,               // has_code_fill

> -  true,                        // is_default_stack_executable

> +  false,               // is_default_stack_executable

>    false,               // can_icf_inline_merge_sections

>    '\0',                        // wrap_char

>    "/lib/ld.so.1",      // program interpreter

> --

> 2.7.4

>
weathertop:2106$ cat tmp2.s
	.comm	i,4,4
weathertop:2107$ cd bin
weathertop:2108$ rm ld
weathertop:2109$ ln -s ld.bfd ld
weathertop:2110$ cd ..
weathertop:2111$ gcc -Bbin/ tmp.c tmp2.s
weathertop:2112$ readelf -l a.out

Elf file type is EXEC (Executable file)
Entry point 0x400410
There are 8 program headers, starting at offset 64

Program Headers:
  Type           Offset             VirtAddr           PhysAddr
                 FileSiz            MemSiz              Flags  Align
  PHDR           0x0000000000000040 0x0000000000400040 0x0000000000400040
                 0x00000000000001c0 0x00000000000001c0  R E    8
  INTERP         0x0000000000000200 0x0000000000400200 0x0000000000400200
                 0x000000000000001b 0x000000000000001b  R      1
      [Requesting program interpreter: /lib/ld-linux-aarch64.so.1]
  LOAD           0x0000000000000000 0x0000000000400000 0x0000000000400000
                 0x00000000000005fc 0x00000000000005fc  R E    10000
  LOAD           0x0000000000000df0 0x0000000000410df0 0x0000000000410df0
                 0x0000000000000238 0x0000000000000240  RW     10000
  DYNAMIC        0x0000000000000e08 0x0000000000410e08 0x0000000000410e08
                 0x00000000000001d0 0x00000000000001d0  RW     8
  NOTE           0x000000000000021c 0x000000000040021c 0x000000000040021c
                 0x0000000000000044 0x0000000000000044  R      4
  GNU_STACK      0x0000000000000000 0x0000000000000000 0x0000000000000000
                 0x0000000000000000 0x0000000000000000  RW     10
  GNU_RELRO      0x0000000000000df0 0x0000000000410df0 0x0000000000410df0
                 0x0000000000000210 0x0000000000000210  R      1

 Section to Segment mapping:
  Segment Sections...
   00     
   01     .interp 
   02     .interp .note.ABI-tag .note.gnu.build-id .gnu.hash .dynsym .dynstr .gnu.version .gnu.version_r .rela.dyn .rela.plt .init .plt .text .fini .rodata .eh_frame 
   03     .init_array .fini_array .jcr .dynamic .got .got.plt .data .bss 
   04     .dynamic 
   05     .note.ABI-tag .note.gnu.build-id 
   06     
   07     .init_array .fini_array .jcr .dynamic .got 
weathertop:2113$ cd bin
weathertop:2114$ rm ld
weathertop:2115$ ln -s ld.gold  ld
weathertop:2116$ cd ..
weathertop:2117$ gcc -Bbin/ tmp.c tmp2.s
weathertop:2118$ readelf -l a.out

Elf file type is EXEC (Executable file)
Entry point 0x4004e0
There are 9 program headers, starting at offset 64

Program Headers:
  Type           Offset             VirtAddr           PhysAddr
                 FileSiz            MemSiz              Flags  Align
  PHDR           0x0000000000000040 0x0000000000400040 0x0000000000400040
                 0x00000000000001f8 0x00000000000001f8  R      8
  INTERP         0x0000000000000238 0x0000000000400238 0x0000000000400238
                 0x000000000000001b 0x000000000000001b  R      1
      [Requesting program interpreter: /lib/ld-linux-aarch64.so.1]
  LOAD           0x0000000000000000 0x0000000000400000 0x0000000000400000
                 0x00000000000006d4 0x00000000000006d4  R E    10000
  LOAD           0x000000000000fdf0 0x000000000041fdf0 0x000000000041fdf0
                 0x0000000000000238 0x0000000000000240  RW     10000
  DYNAMIC        0x000000000000fe08 0x000000000041fe08 0x000000000041fe08
                 0x00000000000001d0 0x00000000000001d0  RW     8
  NOTE           0x0000000000000254 0x0000000000400254 0x0000000000400254
                 0x0000000000000044 0x0000000000000044  R      4
  GNU_EH_FRAME   0x00000000000006cc 0x00000000004006cc 0x00000000004006cc
                 0x0000000000000008 0x0000000000000008  R      4
  GNU_STACK      0x0000000000000000 0x0000000000000000 0x0000000000000000
                 0x0000000000000000 0x0000000000000000  RWE    10
  GNU_RELRO      0x000000000000fdf0 0x000000000041fdf0 0x000000000041fdf0
                 0x0000000000000210 0x0000000000000210  RW     8

 Section to Segment mapping:
  Segment Sections...
   00     
   01     .interp 
   02     .interp .note.ABI-tag .note.gnu.build-id .dynsym .dynstr .gnu.hash .gnu.version .gnu.version_r .rela.dyn .rela.plt .init .plt .text .fini .rodata .eh_frame .eh_frame_hdr 
   03     .jcr .fini_array .init_array .dynamic .got .got.plt .data .bss 
   04     .dynamic 
   05     .note.ABI-tag .note.gnu.build-id 
   06     .eh_frame_hdr 
   07     
   08     .jcr .fini_array .init_array .dynamic .got 
weathertop:2119$
Cary Coutant Sept. 22, 2017, 12:26 a.m. | #2
> For aarch64, ld.bfd makes the stack not executable when a GNU-stack note is

> missing.  Note that elf_backend_default_execstack is 0 in bfd/elfnn-aarch64.c.

>

> However, ld.gold makes the stack executable when a GNU-stack note is missing.

> Note that is_default_stack_executable is true in gold/aarch64.cc.

>

> This appears to be a bug in gold.  It also looks like 64-bit ppc gets this

> wrong also, though I have not tested that.

>

> The following patch fixes this for aarch64 by changing gold aarch64 to make

> is_default_stack_executable false.  This was tested with a make check, and

> there were no regressions.  It was also verified against a testcase using a

> .s file with a missing GNU-stack note.

>

>         gold/

>         * aarch64.cc (Target_aarch64::aarch64_info): Set

>         is_default_stack_executable to false.


This is currently set to true in *all* targets that gold supports,
because objects without a stack note must be assumed to be old objects
that may require an executable stack. All recent objects (where
"recent" is quite generous now) should have stack notes in them. The
only case that I know of where a fairly recent object would be missing
the stack note is an assembler-generated object that was assembled
without the --noexecstack option.

I don't see why aarch64 should be treated differently from other
targets here, unless you can claim that there is no such thing as an
old object that requires an executable stack but does not contain a
stack note to that effect. If that's the case for aarch64 (and Han
agrees), I'll support this patch.

Since you raise the possibility that ppc should also default to false,
I'll ask Alan the same question: are there old objects that must be
assumed to require an executable stack?

Regardless, I would support changing the --warn-execstack option to be
on by default, so that users are properly warned about the presence of
objects that lack the stack note. I think it's in everyone's interest
to have this warning turned on.

I'd also argue that it's time for the assembler to turn on
--noexecstack by default.

-cary
Jim Wilson Sept. 22, 2017, 12:56 a.m. | #3
On Thu, Sep 21, 2017 at 5:26 PM, Cary Coutant <ccoutant@gmail.com> wrote:
> I don't see why aarch64 should be treated differently from other

> targets here, unless you can claim that there is no such thing as an

> old object that requires an executable stack but does not contain a

> stack note to that effect. If that's the case for aarch64 (and Han

> agrees), I'll support this patch.


GNU-stack notes were added in 2004.  The aarch64 port was added in
2012.  So yes, there are no old object files that predate the
GNU-stack notes, which is why aarch64 ld.bfd ignores a missing
GNU-stack note.

> Since you raise the possibility that ppc should also default to false,

> I'll ask Alan the same question: are there old objects that must be

> assumed to require an executable stack?


That should be 64-bit ppc only.  32-bit ppc has old object files that
we need to be compatible with.  Though I'm not sure what the
justification is for 64-bit ppc, since it did exist before GNU-stack
notes.  Maybe this is because of the new ABI that rolled out a few of
years ago?   Checking ChangeLogs, I see that execstack was turned off
for 64-bit ppc in 2007.

Jim
Alan Modra Sept. 22, 2017, 1:31 a.m. | #4
On Thu, Sep 21, 2017 at 05:56:51PM -0700, Jim Wilson wrote:
> On Thu, Sep 21, 2017 at 5:26 PM, Cary Coutant <ccoutant@gmail.com> wrote:

> > I don't see why aarch64 should be treated differently from other

> > targets here, unless you can claim that there is no such thing as an

> > old object that requires an executable stack but does not contain a

> > stack note to that effect. If that's the case for aarch64 (and Han

> > agrees), I'll support this patch.

> 

> GNU-stack notes were added in 2004.  The aarch64 port was added in

> 2012.  So yes, there are no old object files that predate the

> GNU-stack notes, which is why aarch64 ld.bfd ignores a missing

> GNU-stack note.

> 

> > Since you raise the possibility that ppc should also default to false,

> > I'll ask Alan the same question: are there old objects that must be

> > assumed to require an executable stack?

> 

> That should be 64-bit ppc only.  32-bit ppc has old object files that

> we need to be compatible with.  Though I'm not sure what the

> justification is for 64-bit ppc, since it did exist before GNU-stack

> notes.  Maybe this is because of the new ABI that rolled out a few of

> years ago?   Checking ChangeLogs, I see that execstack was turned off

> for 64-bit ppc in 2007.


ppc64 ELFv1 never needs to be exec stack.  Since function descriptors
are used there is no need for stack trampolines.  Current gcc doesn't
emit the stack notes for ELFv1.

ppc64 ELFv2 on the other hand *does* need stack trampolines, and gcc
has always emitted the stack notes for ELFv2.

So I think is_default_stack_executable should indeed be false for
ppc64.

-- 
Alan Modra
Australia Development Lab, IBM
Cary Coutant Sept. 22, 2017, 5:56 a.m. | #5
On Thu, Sep 21, 2017 at 6:31 PM, Alan Modra <amodra@gmail.com> wrote:
> On Thu, Sep 21, 2017 at 05:56:51PM -0700, Jim Wilson wrote:

>> On Thu, Sep 21, 2017 at 5:26 PM, Cary Coutant <ccoutant@gmail.com> wrote:

>> > I don't see why aarch64 should be treated differently from other

>> > targets here, unless you can claim that there is no such thing as an

>> > old object that requires an executable stack but does not contain a

>> > stack note to that effect. If that's the case for aarch64 (and Han

>> > agrees), I'll support this patch.

>>

>> GNU-stack notes were added in 2004.  The aarch64 port was added in

>> 2012.  So yes, there are no old object files that predate the

>> GNU-stack notes, which is why aarch64 ld.bfd ignores a missing

>> GNU-stack note.

>>

>> > Since you raise the possibility that ppc should also default to false,

>> > I'll ask Alan the same question: are there old objects that must be

>> > assumed to require an executable stack?

>>

>> That should be 64-bit ppc only.  32-bit ppc has old object files that

>> we need to be compatible with.  Though I'm not sure what the

>> justification is for 64-bit ppc, since it did exist before GNU-stack

>> notes.  Maybe this is because of the new ABI that rolled out a few of

>> years ago?   Checking ChangeLogs, I see that execstack was turned off

>> for 64-bit ppc in 2007.

>

> ppc64 ELFv1 never needs to be exec stack.  Since function descriptors

> are used there is no need for stack trampolines.  Current gcc doesn't

> emit the stack notes for ELFv1.

>

> ppc64 ELFv2 on the other hand *does* need stack trampolines, and gcc

> has always emitted the stack notes for ELFv2.

>

> So I think is_default_stack_executable should indeed be false for

> ppc64.


OK, I'm satisfied for both aarch64 and ppc64. Jim, your patch is OK.
Alan, I'll leave the ppc64 patch to you.

Any thoughts on turning on --warn-execstack by default?

-cary
Jim Wilson Sept. 22, 2017, 3:16 p.m. | #6
On Thu, Sep 21, 2017 at 10:56 PM, Cary Coutant <ccoutant@gmail.com> wrote:
> Any thoughts on turning on --warn-execstack by default?


--warn-execstack does two things.
1) Warn if a missing GNU-stack note causes the stack to be marked as executable.
2) Warn if a present GNU-stack note causes the stack to be marked as executable.

I don't think the second one should be on by default, as this would be
inconvenient for anyone using nested functions on targets that require
executable stack trampolines.  Correctly written and compiled code
should not be producing warnings by default.

I think the first one could be on by default.  It has been 13 years
since we added GNU-stack notes.  If there are ones that are still
missing, we should warn people.  This would not affect aarch64 and
64-bit ppc, as don't mark the stack as executable when a note is
missing now.

Jim
Jim Wilson Sept. 22, 2017, 3:21 p.m. | #7
On Fri, Sep 22, 2017 at 8:16 AM, Jim Wilson <jim.wilson@linaro.org> wrote:
> On Thu, Sep 21, 2017 at 10:56 PM, Cary Coutant <ccoutant@gmail.com> wrote:

>> Any thoughts on turning on --warn-execstack by default?

>

> --warn-execstack does two things.

> 1) Warn if a missing GNU-stack note causes the stack to be marked as executable.

> 2) Warn if a present GNU-stack note causes the stack to be marked as executable.


Actually, there is a third one in a different function.  I didn't look
far enough.
3) Warn if -z noexecstack and a GNU-stack note is present and causes
the stack to be marked as executable.

I think that one should be on by default.  If a command line option
conflicts with the input files, then it seems like you should give a
warning always.

Jim

Patch

diff --git a/gold/aarch64.cc b/gold/aarch64.cc
index a72e2c3..4c6e920 100644
--- a/gold/aarch64.cc
+++ b/gold/aarch64.cc
@@ -3523,7 +3523,7 @@  const Target::Target_info Target_aarch64<64, false>::aarch64_info =
   false,		// has_make_symbol
   false,		// has_resolve
   false,		// has_code_fill
-  true,			// is_default_stack_executable
+  false,		// is_default_stack_executable
   true,			// can_icf_inline_merge_sections
   '\0',			// wrap_char
   "/lib/ld.so.1",	// program interpreter
@@ -3551,7 +3551,7 @@  const Target::Target_info Target_aarch64<32, false>::aarch64_info =
   false,		// has_make_symbol
   false,		// has_resolve
   false,		// has_code_fill
-  true,			// is_default_stack_executable
+  false,		// is_default_stack_executable
   false,		// can_icf_inline_merge_sections
   '\0',			// wrap_char
   "/lib/ld.so.1",	// program interpreter
@@ -3579,7 +3579,7 @@  const Target::Target_info Target_aarch64<64, true>::aarch64_info =
   false,		// has_make_symbol
   false,		// has_resolve
   false,		// has_code_fill
-  true,			// is_default_stack_executable
+  false,		// is_default_stack_executable
   true,			// can_icf_inline_merge_sections
   '\0',			// wrap_char
   "/lib/ld.so.1",	// program interpreter
@@ -3607,7 +3607,7 @@  const Target::Target_info Target_aarch64<32, true>::aarch64_info =
   false,		// has_make_symbol
   false,		// has_resolve
   false,		// has_code_fill
-  true,			// is_default_stack_executable
+  false,		// is_default_stack_executable
   false,		// can_icf_inline_merge_sections
   '\0',			// wrap_char
   "/lib/ld.so.1",	// program interpreter