[ELF] Add '-z nognustack' opt to suppress emitting PT_GNU_STACK

[mgorny]

Summary

Add a new '-z nognustack' option that suppresses emitting PT_GNU_STACK
segment. This segment is not supported at all on NetBSD (stack is
always non-executable), and the option is meant to be used to disable
emitting it.

[krytarowski]

docs/ld.lld.1

515 ↗

(On Diff #181121)

section?

[krytarowski]

Looks good, we don't want GNU STACK on NetBSD.

[mgorny]

docs/ld.lld.1

515 ↗

(On Diff #181121)

I think 'segment' is actually the correct term here.

[krytarowski]

ELF/Writer.cpp

1980 ↗

(On Diff #181121)

section -> segment?

docs/ld.lld.1

515 ↗

(On Diff #181121)

I see, probably right.

[krytarowski]

ELF/Driver.cpp

874 ↗

(On Diff #181121)

I would add gnustack vs nognustack - two options, but it's up to maintainers to decide. We can keep it enabled by default.... and disable it in NetBSD's clang driver.

[ruiu]

The absence of PT_GNU_STACK segment makes stack area executable on systems that recognizes PT_GNU_STACK segment. So, I think if -z execstack is specified, we should omit PT_GNU_STACK segment rather than adding it, which I think you guys want. If we do that, it seems -z nognustack is a redundant option. That option name is unfortunate (you don't really mean you want an executable stack area), but that's I think still better than adding an option that is very similar to an existing feature.

[krytarowski]

In D56554#1353368, @ruiu wrote:

The absence of PT_GNU_STACK segment makes stack area executable on systems that recognizes PT_GNU_STACK segment. So, I think if -z execstack is specified, we should omit PT_GNU_STACK segment rather than adding it, which I think you guys want. If we do that, it seems -z nognustack is a redundant option. That option name is unfortunate (you don't really mean you want an executable stack area), but that's I think still better than adding an option that is very similar to an existing feature.

If we are going to change the meaning of -z execstack, can we rename the option in lld? Probably to -z gnustack vs -z nognustack, probably there is no other use-case than RWX->RW protection change.

Systems like fuchsia don't need/want it either. FreeBSD&Linux recognize this ELF segment.

[ruiu]

In D56554#1353487, @krytarowski wrote:

In D56554#1353368, @ruiu wrote:

The absence of PT_GNU_STACK segment makes stack area executable on systems that recognizes PT_GNU_STACK segment. So, I think if -z execstack is specified, we should omit PT_GNU_STACK segment rather than adding it, which I think you guys want. If we do that, it seems -z nognustack is a redundant option. That option name is unfortunate (you don't really mean you want an executable stack area), but that's I think still better than adding an option that is very similar to an existing feature.

If we are going to change the meaning of -z execstack, can we rename the option in lld? Probably to -z gnustack vs -z nognustack, probably there is no other use-case than RWX->RW protection change.

Both -z execstack and -z noexecstack are supported by GNU linkers, so we can't simply rename them. We might be able to define aliases to the options. But I'm not sure if we want it if the only reason to do so is aesthetic reason.

[krytarowski]

In D56554#1353513, @ruiu wrote:

In D56554#1353487, @krytarowski wrote:

In D56554#1353368, @ruiu wrote:

The absence of PT_GNU_STACK segment makes stack area executable on systems that recognizes PT_GNU_STACK segment. So, I think if -z execstack is specified, we should omit PT_GNU_STACK segment rather than adding it, which I think you guys want. If we do that, it seems -z nognustack is a redundant option. That option name is unfortunate (you don't really mean you want an executable stack area), but that's I think still better than adding an option that is very similar to an existing feature.

If we are going to change the meaning of -z execstack, can we rename the option in lld? Probably to -z gnustack vs -z nognustack, probably there is no other use-case than RWX->RW protection change.

Both -z execstack and -z noexecstack are supported by GNU linkers, so we can't simply rename them. We might be able to define aliases to the options. But I'm not sure if we want it if the only reason to do so is aesthetic reason.

For compat with GNU linkers we could disable it by default, and emit GNU stack only on demand.

The current approach to enable GNU STACK is to inline a new segment definition in assembly. It's done this way in llvm projects too.

[krytarowski]

Actually looking at GNU ld(1) source code, the logic is a little bit more complex and it depends on more aspects like relocations.

[ruiu]

In D56554#1353527, @krytarowski wrote:

In D56554#1353513, @ruiu wrote:

In D56554#1353487, @krytarowski wrote:

In D56554#1353368, @ruiu wrote:

The absence of PT_GNU_STACK segment makes stack area executable on systems that recognizes PT_GNU_STACK segment. So, I think if -z execstack is specified, we should omit PT_GNU_STACK segment rather than adding it, which I think you guys want. If we do that, it seems -z nognustack is a redundant option. That option name is unfortunate (you don't really mean you want an executable stack area), but that's I think still better than adding an option that is very similar to an existing feature.

If we are going to change the meaning of -z execstack, can we rename the option in lld? Probably to -z gnustack vs -z nognustack, probably there is no other use-case than RWX->RW protection change.

Both -z execstack and -z noexecstack are supported by GNU linkers, so we can't simply rename them. We might be able to define aliases to the options. But I'm not sure if we want it if the only reason to do so is aesthetic reason.

For compat with GNU linkers we could disable it by default, and emit GNU stack only on demand.

The current approach to enable GNU STACK is to inline a new segment definition in assembly. It's done this way in llvm projects too.

Right. In terms of which is the default, lld is not compatible with GNU. lld by default tries to disable executable stack (which is definitely a good thing to do today). GNU linkers tries to do something smarter which doesn't make much sense and more error-prone today. I think changing the default makes sense.

Defining an alias to -z execstack as -z nognustack is a different story. I'm not totally against doing it, and I see that the option name is somewhat confusing, but I'm actually curious if the alias is what you really want. If you pass -z noexecstack to lld, your command line still works for both lld and GNU ld. But if you define -z nognustack and pass that option to lld, your command line is no longer compatible with GNU. You would no longer be able to copy all arguments to lld to run it again with GNU ld by just copy-n-pasting, for example. Are you happy with that?

[krytarowski]

Shouldn't the logic of emitting/not-emitting be based on findSection(".note.GNU-stack")? @mgorny can you please investigate it?

[krytarowski]

I understand the concerns, but lld is biased with being a Linux linker. We need to customize it (restore defaults?) for NetBSD.

[ruiu]

GNU linkers assume that input object files that work with non-executable stack has a .note.GNU-stack section, and they emit a PT_GNU_STACK segment to mark the stack area non-executable if all input files have that marker section.

If restoring default means we implement that behavior, then lld shouldn't do that. That mechanism is error-prone, and the consequence of an error is disabling a major security measure. To be on the safe side, we simply emit unless it is explicitly to not do that by -z execstack.

If you add -z nognustack as an alias to -z execstack (and changing the behavior of -z execstack to not add the marker segment at all), I think I'm fine with that.

[krytarowski]

What do you think about this new logic:

1. specified -z execstack -> do not emit GNU STACK segment
2. specified -z noexecstack -> emit GNU STACK segment
3. no option specified -> detect findSection(".note.GNU-stack") (I might miss offhand some details here to be sure if it is reliable)

3.1. detected -> emit GNU STACK segment
3.2. not detected -> do not emit GNU STACK segment

Additionally we will specify explicitly in the clang driver for Linux and FreeBSD -z noexecstack.

If this is not acceptable and we must use GNU extensions for everybody, we will need to specify this ugly -z execstack in the NetBSD driver (once we will get an agreement that we customize the linker through the driver).

[mgorny]

@ruiu, what if one of the systems changes defaults (e.g. due to Hardening) and starts defaulting to noexecstack? In that case we'd want `-z execstack' to actually emit PT_GNU_STACK, and I don't think we really are able to 100% detect the default in clang. I'd really see this as a trinary option: emit RW, emit RWX or not emit at all. I don't think it's a good idea to make linker rely on implicit assumptions or hidden guesses that could easily confuse user as to what's happening and why.

[mgorny]

Actually, I've just researched a bit and default stack permissions depend on arch in glibc, so assuming it's RWX by default is wrong.

[krytarowski]

@ruiu actually if we can get D56215 merged we will be able to tune it specifically for NetBSD (with if (Config->TargetTriple.isOSNetBSD()) {) and retain intact the current Linux-biased logic for everybody who desires to use it.

We will need to add similar NetBSD adjustments in other parts of lld.

[ruiu]

In D56554#1353572, @krytarowski wrote:

What do you think about this new logic:

1. specified -z execstack -> do not emit GNU STACK segment
2. specified -z noexecstack -> emit GNU STACK segment
3. no option specified -> detect findSection(".note.GNU-stack") (I might miss offhand some details here to be sure if it is reliable)

3.1. detected -> emit GNU STACK segment
3.2. not detected -> do not emit GNU STACK segment

Additionally we will specify explicitly in the clang driver for Linux and FreeBSD -z noexecstack.

I think I don't like the very idea of using "marker sections" in input object files to change the linker behavior. That's too subtle and seems error-prone to me.

After thinking for a while, I started thinking that the first version of this patch is probably exactly what we want. I had been thinking that -z {no,}execstack and -z {no,}gnustack are four different options, but what we actually want to get is tri-state:

• emit PT_GNU_STACK with RWX permission
• emit PT_GNU_STACK with RW permission
• do not emit PT_GNU_STACK

So we could map them to -z {execstack,noexecstack,nognustack}, respectively, with default set to -z noexecstack. You guys can pass -z nognustack to the linker to tell the linker to not emit it at all. That's exactly this patch does.

[krytarowski]

In D56554#1354885, @ruiu wrote:

In D56554#1353572, @krytarowski wrote:

What do you think about this new logic:

1. specified -z execstack -> do not emit GNU STACK segment
2. specified -z noexecstack -> emit GNU STACK segment
3. no option specified -> detect findSection(".note.GNU-stack") (I might miss offhand some details here to be sure if it is reliable)

3.1. detected -> emit GNU STACK segment
3.2. not detected -> do not emit GNU STACK segment

Additionally we will specify explicitly in the clang driver for Linux and FreeBSD -z noexecstack.

I think I don't like the very idea of using "marker sections" in input object files to change the linker behavior. That's too subtle and seems error-prone to me.

After thinking for a while, I started thinking that the first version of this patch is probably exactly what we want. I had been thinking that -z {no,}execstack and -z {no,}gnustack are four different options, but what we actually want to get is tri-state:

• emit PT_GNU_STACK with RWX permission
• emit PT_GNU_STACK with RW permission
• do not emit PT_GNU_STACK

So we could map them to -z {execstack,noexecstack,nognustack}, respectively, with default set to -z noexecstack. You guys can pass -z nognustack to the linker to tell the linker to not emit it at all. That's exactly this patch does.

Actually after a longer thought, I recommend to hardcode inside lld a check for TargetTriple.isOSNetBSD(). Using -z nognustack isn't portable to other linkers.

ELF/Writer.cpp

1979 ↗

(On Diff #181121)

Please go for if (!Config->TargetTriple.isOSNetBSD()) {

[MaskRay]

Should ZNognustack and ZExecstack be unified to a tri-state enum variable?

[mgorny]

@ruiu, what do you think? The current logic forces some precedence, i.e. if you pass -z nognustack, further -z {no,}execstack are ignored. I suppose we could just force passing -z nognustack as last option clang-wise.

[ruiu]

In D56554#1357385, @mgorny wrote:

@ruiu, what do you think? The current logic forces some precedence, i.e. if you pass -z nognustack, further -z {no,}execstack are ignored. I suppose we could just force passing -z nognustack as last option clang-wise.

Are you asking about making the flag tri-state? If so, I think it logically makes sense and feels more natural to represent it with two boolean flags. As you said, the last one should always takes precedence over the others among the three flags.

[mgorny]

Just to be clear, are you suggesting that I add -z gnustack as well?

[ruiu]

No, I'm suggesting you add execstack, noexecstack and nognustack as a tri-state -z flag. Does this sound good?

[mgorny]

In D56554#1369592, @ruiu wrote:

No, I'm suggesting you add execstack, noexecstack and nognustack as a tri-state -z flag. Does this sound good?

Do you mean that of those three options, the last one specified should take precedence?

[mgorny]

In D56554#1369606, @mgorny wrote:

In D56554#1369592, @ruiu wrote:

No, I'm suggesting you add execstack, noexecstack and nognustack as a tri-state -z flag. Does this sound good?

Do you mean that of those three options, the last one specified should take precedence?

(and, if yes, could you suggest how to implement it? getZOption() doesn't seem suitable.

[mgorny]

Ok, here's my proposition of using trinary enum.

[mgorny]

Ping.

[MaskRay]

This segment is not supported at all on NetBSD (stack is always non-executable), and the option is meant to be used to disable emitting it.

The string .note.GNU-stack takes only a few bytes in .shstrtab and a few for an Elf64_Shdr instance. Are there any tools warning about unknown section .note.GNU-stack on NetBSD?

[mgorny]

Besides our verbose purists? Not that I'm aware of.

[ruiu]

Overall looks good.

Do you mean that of those three options, the last one specified should take precedence?

Yes.

ELF/Config.h

191 ↗

(On Diff #183729)

Members are (roughly) sorted alphabetically, so move this below the last boolean member.

ELF/Driver.cpp

352 ↗

(On Diff #183729)

nit: no else after return.

369 ↗

(On Diff #183729)

If you have clang-format, please run it on this file.

[mgorny]

ELF/Config.h

191 ↗

(On Diff #183729)

I don't really comprehend the sort key here but done ;-).

ELF/Driver.cpp

369 ↗

(On Diff #183729)

File or function? Because the former would add some irrelevant changes to the diff, so if at all, I'd rather do it separately. File I've kept unformatted to make the patch easier to read. I've reformatted it now.

[mgorny]

Implemented @ruiu's suggestions.

[vit9696]

@ruiu, we have some hardware, which bootloader does not support PT_GNU_STACK segments, and would therefore benefit quite a bit from this option. As this does not really depend on NetBSD support, could you please merge this patch in time for 9.0? Thanks!

[MaskRay]

This patch makes sense to me. It can benefit other OS/environment as well.

lld/ELF/Driver.cpp

394

This is obvious. The comment can be removed.

[MaskRay]

lld/ELF/Driver.cpp

394

Not done. // default can be deleted I think.

[mgorny]

Rebased and updated coding style.

@ruiu, ping.

[mgorny]

lld/ELF/Driver.cpp

394

You didn't give me time to upload updated patch ;-).

[krytarowski]

This looks good as an intermediate step to make lld saner.

[MaskRay]

lld/ELF/Driver.cpp

394

You had plenty of time updating the patch but you still committed without changing this part...

[mgorny]

lld/ELF/Driver.cpp

394

I'm really sorry, I thought I did that. You commented before I actually uploaded the new patch, and no comments afterwards got me confused.

lld/ELF/Config.h

 enum class Target2Policy { Abs, Rel, GotRel };
 
 // For tracking ARM Float Argument PCS
 enum class ARMVFPArgKind { Default, Base, VFP, ToolChain };
 
 // For -z noseparate-code, -z separate-code and -z separate-loadable-segments.
 enum class SeparateSegmentKind { None, Code, Loadable };
 
+// For -z *stack
+enum class GnuStackKind { None, Exec, NoExec };
+
 struct SymbolVersion {
   llvm::StringRef name;
   bool isExternCpp;
   bool hasWildcard;
 };
 
 // This struct contains symbols version definition that
 // can be found in version script if it is used for link.
struct Configuration {
   bool zNow;
   bool zOrigin;
   bool zRelro;
   bool zRodynamic;
   bool zText;
   bool zRetpolineplt;
   bool zWxneeded;
   DiscardPolicy discard;
+  GnuStackKind zGnustack;
   ICFLevel icf;
   OrphanHandlingPolicy orphanHandling;
   SortSectionPolicy sortSection;
   StripPolicy strip;
   UnresolvedPolicy unresolvedSymbols;
   Target2Policy target2;
   ARMVFPArgKind armVFPArgs = ARMVFPArgKind::Default;
   BuildIdKind buildId = BuildIdKind::None;

lld/ELF/Driver.cpp

for (auto *arg : args.filtered_reverse(OPT_z)) {
     StringRef v = arg->getValue();
     if (v == "noseparate-code")
       return SeparateSegmentKind::None;
     if (v == "separate-code")
       return SeparateSegmentKind::Code;
     if (v == "separate-loadable-segments")
       return SeparateSegmentKind::Loadable;
   }
   return SeparateSegmentKind::None;
 }
 
+static GnuStackKind getZGnuStack(opt::InputArgList &args) {
+  for (auto *arg : args.filtered_reverse(OPT_z)) {
+    if (StringRef("execstack") == arg->getValue())
+      return GnuStackKind::Exec;
+    if (StringRef("noexecstack") == arg->getValue())
+      return GnuStackKind::NoExec;
+    if (StringRef("nognustack") == arg->getValue())
+      return GnuStackKind::None;
+  }
+
+  // default
+  return GnuStackKind::NoExec;
+}
+
 static bool isKnownZFlag(StringRef s) {
   return s == "combreloc" || s == "copyreloc" || s == "defs" ||
          s == "execstack" || s == "global" || s == "hazardplt" ||
          s == "ifunc-noplt" || s == "initfirst" || s == "interpose" ||
          s == "keep-text-section-prefix" || s == "lazy" || s == "muldefs" ||
          s == "separate-code" || s == "separate-loadable-segments" ||
          s == "nocombreloc" || s == "nocopyreloc" || s == "nodefaultlib" ||
          s == "nodelete" || s == "nodlopen" || s == "noexecstack" ||
+         s == "nognustack" ||
          s == "nokeep-text-section-prefix" || s == "norelro" ||
          s == "noseparate-code" || s == "notext" || s == "now" ||
          s == "origin" || s == "relro" || s == "retpolineplt" ||
          s == "rodynamic" || s == "text" || s == "undefs" || s == "wxneeded" ||
          s.startswith("common-page-size=") || s.startswith("max-page-size=") ||
          s.startswith("stack-size=");
 }
 
static void readConfigs(opt::InputArgList &args) {
   config->warnIfuncTextrel =
       args.hasFlag(OPT_warn_ifunc_textrel, OPT_no_warn_ifunc_textrel, false);
   config->warnSymbolOrdering =
       args.hasFlag(OPT_warn_symbol_ordering, OPT_no_warn_symbol_ordering, true);
   config->zCombreloc = getZFlag(args, "combreloc", "nocombreloc", true);
   config->zCopyreloc = getZFlag(args, "copyreloc", "nocopyreloc", true);
   config->zExecstack = getZFlag(args, "execstack", "noexecstack", false);
   config->zGlobal = hasZOption(args, "global");
+  config->zGnustack = getZGnuStack(args);
   config->zHazardplt = hasZOption(args, "hazardplt");
   config->zIfuncNoplt = hasZOption(args, "ifunc-noplt");
   config->zInitfirst = hasZOption(args, "initfirst");
   config->zInterpose = hasZOption(args, "interpose");
   config->zKeepTextSectionPrefix = getZFlag(
       args, "keep-text-section-prefix", "nokeep-text-section-prefix", false);
   config->zNodefaultlib = hasZOption(args, "nodefaultlib");
   config->zNodelete = hasZOption(args, "nodelete");

lld/ELF/Writer.cpp

if (part.ehFrame->isNeeded() && part.ehFrameHdr &&
     addHdr(PT_GNU_EH_FRAME, part.ehFrameHdr->getParent()->getPhdrFlags())
         ->add(part.ehFrameHdr->getParent());
 
   // PT_OPENBSD_RANDOMIZE is an OpenBSD-specific feature. That makes
   // the dynamic linker fill the segment with random data.
   if (OutputSection *cmd = findSection(".openbsd.randomdata", partNo))
     addHdr(PT_OPENBSD_RANDOMIZE, cmd->getPhdrFlags())->add(cmd);
 
+  if (config->zGnustack != GnuStackKind::None) {
     // PT_GNU_STACK is a special section to tell the loader to make the
     // pages for the stack non-executable. If you really want an executable
     // stack, you can pass -z execstack, but that's not recommended for
     // security reasons.
     unsigned perm = PF_R | PF_W;
-  if (config->zExecstack)
+    if (config->zGnustack == GnuStackKind::Exec)
       perm |= PF_X;
     addHdr(PT_GNU_STACK, perm)->p_memsz = config->zStackSize;
+  }
 
   // PT_OPENBSD_WXNEEDED is a OpenBSD-specific header to mark the executable
   // is expected to perform W^X violations, such as calling mprotect(2) or
   // mmap(2) with PROT_WRITE | PROT_EXEC, which is prohibited by default on
   // OpenBSD.
   if (config->zWxneeded)
     addHdr(PT_OPENBSD_WXNEEDED, PF_X);
 

lld/docs/ld.lld.1

 flag to indicate that the object cannot be unloaded from a process.
 .Pp
 .It Cm nodlopen
 Set the
 .Dv DF_1_NOOPEN
 flag to indicate that the object may not be opened by
 .Xr dlopen 3 .
 .Pp
+.It Cm nognustack
+Do not emit the
+.Dv PT_GNU_STACK
+segment.
+.Pp
 .It Cm norelro
 Do not indicate that portions of the object shold be mapped read-only
 after initial relocation processing.
 The object will omit the
 .Dv PT_GNU_RELRO
 segment.
 .Pp
 .It Cm notext

lld/test/ELF/gnustack.s

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t1
 
 # RUN: ld.lld %t1 -z execstack -o %t
 # RUN: llvm-readobj --program-headers -S %t | FileCheck --check-prefix=RWX %s
 
 # RUN: ld.lld %t1 -o %t
 # RUN: llvm-readobj --program-headers -S %t | FileCheck --check-prefix=RW %s
 
 # RUN: ld.lld %t1 -o %t -z noexecstack
 # RUN: llvm-readobj --program-headers -S %t | FileCheck --check-prefix=RW %s
 
+# RUN: ld.lld %t1 -o %t -z nognustack
+# RUN: llvm-readobj --program-headers -s %t | FileCheck --check-prefix=NOGNUSTACK %s
+
 # RW:      Type: PT_GNU_STACK
 # RW-NEXT: Offset: 0x0
 # RW-NEXT: VirtualAddress: 0x0
 # RW-NEXT: PhysicalAddress: 0x0
 # RW-NEXT: FileSize: 0
 # RW-NEXT: MemSize: 0
 # RW-NEXT: Flags [
 # RW-NEXT:   PF_R

 # RWX-NEXT: MemSize: 0
 # RWX-NEXT: Flags [
 # RWX-NEXT:   PF_R
 # RWX-NEXT:   PF_W
 # RWX-NEXT:   PF_X
 # RWX-NEXT: ]
 # RWX-NEXT: Alignment: 0
 
+# NOGNUSTACK-NOT: Type: PT_GNU_STACK
+
 .globl _start
 _start: