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

[Michał Górny via Phabricator via cfe-commits]

mgorny marked an inline comment as done.
mgorny added inline comments.



Comment at: lld/ELF/Driver.cpp:357
+
+  // default
+  return GnuStackKind::NoExec;

MaskRay wrote:
> mgorny wrote:
> > MaskRay wrote:
> > > MaskRay wrote:
> > > > This is obvious. The comment can be removed.
> > > Not done. `// default` can be deleted I think.
> > You didn't give me time to upload updated patch ;-).
> You had plenty of time updating the patch but you still committed without 
> changing this part...
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: lld/ELF/Driver.cpp:357
+
+  // default
+  return GnuStackKind::NoExec;

mgorny wrote:
> MaskRay wrote:
> > MaskRay wrote:
> > > This is obvious. The comment can be removed.
> > Not done. `// default` can be deleted I think.
> You didn't give me time to upload updated patch ;-).
You had plenty of time updating the patch but you still committed without 
changing this part...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Michał Górny via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
mgorny marked an inline comment as done.
Closed by commit rG2a0fcae3d4d1: [lld] [ELF] Add -z nognustack opt 
to suppress emitting PT_GNU_STACK (authored by mgorny).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554

Files:
  lld/ELF/Config.h
  lld/ELF/Driver.cpp
  lld/ELF/Writer.cpp
  lld/docs/ld.lld.1
  lld/test/ELF/gnustack.s

Index: lld/test/ELF/gnustack.s
===
--- lld/test/ELF/gnustack.s
+++ lld/test/ELF/gnustack.s
@@ -10,6 +10,9 @@
 # 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
@@ -35,5 +38,7 @@
 # RWX-NEXT: ]
 # RWX-NEXT: Alignment: 0
 
+# NOGNUSTACK-NOT: Type: PT_GNU_STACK
+
 .globl _start
 _start:
Index: lld/docs/ld.lld.1
===
--- lld/docs/ld.lld.1
+++ lld/docs/ld.lld.1
@@ -655,6 +655,11 @@
 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.
Index: lld/ELF/Writer.cpp
===
--- lld/ELF/Writer.cpp
+++ lld/ELF/Writer.cpp
@@ -2172,14 +2172,16 @@
   if (OutputSection *cmd = findSection(".openbsd.randomdata", partNo))
 addHdr(PT_OPENBSD_RANDOMIZE, cmd->getPhdrFlags())->add(cmd);
 
-  // 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)
-perm |= PF_X;
-  addHdr(PT_GNU_STACK, perm)->p_memsz = config->zStackSize;
+  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->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
Index: lld/ELF/Driver.cpp
===
--- lld/ELF/Driver.cpp
+++ lld/ELF/Driver.cpp
@@ -394,6 +394,20 @@
   return SeparateSegmentKind::None;
 }
 
+static GnuStackKind getZGnuStack(opt::InputArgList ) {
+  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" ||
@@ -402,6 +416,7 @@
  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" ||
@@ -951,6 +966,7 @@
   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");
Index: lld/ELF/Config.h
===
--- lld/ELF/Config.h
+++ lld/ELF/Config.h
@@ -64,6 +64,9 @@
 // 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;

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

[Kamil Rytarowski via Phabricator via cfe-commits]

krytarowski added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Michał Górny via Phabricator via cfe-commits]

mgorny updated this revision to Diff 225915.
mgorny added a comment.

Rebased and updated coding style.

@ruiu, ping.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554

Files:
  lld/ELF/Config.h
  lld/ELF/Driver.cpp
  lld/ELF/Writer.cpp
  lld/docs/ld.lld.1
  lld/test/ELF/gnustack.s

Index: lld/test/ELF/gnustack.s
===
--- lld/test/ELF/gnustack.s
+++ lld/test/ELF/gnustack.s
@@ -10,6 +10,9 @@
 # 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
@@ -35,5 +38,7 @@
 # RWX-NEXT: ]
 # RWX-NEXT: Alignment: 0
 
+# NOGNUSTACK-NOT: Type: PT_GNU_STACK
+
 .globl _start
 _start:
Index: lld/docs/ld.lld.1
===
--- lld/docs/ld.lld.1
+++ lld/docs/ld.lld.1
@@ -655,6 +655,11 @@
 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.
Index: lld/ELF/Writer.cpp
===
--- lld/ELF/Writer.cpp
+++ lld/ELF/Writer.cpp
@@ -2172,14 +2172,16 @@
   if (OutputSection *cmd = findSection(".openbsd.randomdata", partNo))
 addHdr(PT_OPENBSD_RANDOMIZE, cmd->getPhdrFlags())->add(cmd);
 
-  // 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)
-perm |= PF_X;
-  addHdr(PT_GNU_STACK, perm)->p_memsz = config->zStackSize;
+  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->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
Index: lld/ELF/Driver.cpp
===
--- lld/ELF/Driver.cpp
+++ lld/ELF/Driver.cpp
@@ -394,6 +394,20 @@
   return SeparateSegmentKind::None;
 }
 
+static GnuStackKind getZGnuStack(opt::InputArgList ) {
+  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" ||
@@ -402,6 +416,7 @@
  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" ||
@@ -951,6 +966,7 @@
   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");
Index: lld/ELF/Config.h
===
--- lld/ELF/Config.h
+++ lld/ELF/Config.h
@@ -64,6 +64,9 @@
 // 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;
@@ -216,6 +219,7 @@
   bool zRetpolineplt;
   bool zWxneeded;
   DiscardPolicy discard;
+  GnuStackKind zGnustack;
   ICFLevel icf;
   OrphanHandlingPolicy 

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

[Michał Górny via Phabricator via cfe-commits]

mgorny marked 2 inline comments as done.
mgorny added inline comments.



Comment at: lld/ELF/Driver.cpp:357
+
+  // default
+  return GnuStackKind::NoExec;

MaskRay wrote:
> MaskRay wrote:
> > This is obvious. The comment can be removed.
> Not done. `// default` can be deleted I think.
You didn't give me time to upload updated patch ;-).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay accepted this revision.
MaskRay added inline comments.
This revision is now accepted and ready to land.



Comment at: lld/ELF/Driver.cpp:357
+
+  // default
+  return GnuStackKind::NoExec;

MaskRay wrote:
> This is obvious. The comment can be removed.
Not done. `// default` can be deleted I think.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

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




Comment at: lld/ELF/Driver.cpp:357
+
+  // default
+  return GnuStackKind::NoExec;

This is obvious. The comment can be removed.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[vit9696 via Phabricator via cfe-commits]

vit9696 added a comment.

@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!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Michał Górny via Phabricator via cfe-commits]

mgorny updated this revision to Diff 190681.
mgorny marked 2 inline comments as done.
mgorny added a comment.
Herald added a project: LLVM.

Implemented @ruiu's suggestions.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554

Files:
  lld/ELF/Config.h
  lld/ELF/Driver.cpp
  lld/ELF/Writer.cpp
  lld/docs/ld.lld.1
  lld/test/ELF/gnustack.s

Index: lld/test/ELF/gnustack.s
===
--- lld/test/ELF/gnustack.s
+++ lld/test/ELF/gnustack.s
@@ -10,6 +10,9 @@
 # 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
@@ -35,5 +38,7 @@
 # RWX-NEXT: ]
 # RWX-NEXT: Alignment: 0
 
+# NOGNUSTACK-NOT: Type: PT_GNU_STACK
+
 .globl _start
 _start:
Index: lld/docs/ld.lld.1
===
--- lld/docs/ld.lld.1
+++ lld/docs/ld.lld.1
@@ -578,6 +578,10 @@
 .Dv DF_1_NOOPEN
 flag to indicate that the object may not be opened by
 .Xr dlopen 3 .
+.It Cm nognustack
+Do not emit the
+.Dv PT_GNU_STACK
+segment.
 .It Cm norelro
 Do not indicate that portions of the object shold be mapped read-only
 after initial relocation processing.
Index: lld/ELF/Writer.cpp
===
--- lld/ELF/Writer.cpp
+++ lld/ELF/Writer.cpp
@@ -2019,14 +2019,16 @@
   if (OutputSection *Cmd = findSection(".openbsd.randomdata"))
 AddHdr(PT_OPENBSD_RANDOMIZE, Cmd->getPhdrFlags())->add(Cmd);
 
-  // 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)
-Perm |= PF_X;
-  AddHdr(PT_GNU_STACK, Perm)->p_memsz = Config->ZStackSize;
+  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->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
Index: lld/ELF/Driver.cpp
===
--- lld/ELF/Driver.cpp
+++ lld/ELF/Driver.cpp
@@ -344,6 +344,20 @@
   return Default;
 }
 
+static GnuStackKind getZGnuStack(opt::InputArgList ) {
+  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" ||
@@ -351,10 +365,11 @@
  S == "keep-text-section-prefix" || S == "lazy" || S == "muldefs" ||
  S == "nocombreloc" || S == "nocopyreloc" || S == "nodefaultlib" ||
  S == "nodelete" || S == "nodlopen" || S == "noexecstack" ||
- S == "nokeep-text-section-prefix" || S == "norelro" || S == "notext" ||
- S == "now" || S == "origin" || S == "relro" || S == "retpolineplt" ||
- S == "rodynamic" || S == "text" || S == "wxneeded" ||
- S.startswith("max-page-size=") || S.startswith("stack-size=");
+ S == "nognustack" || S == "nokeep-text-section-prefix" ||
+ S == "norelro" || S == "notext" || S == "now" || S == "origin" ||
+ S == "relro" || S == "retpolineplt" || S == "rodynamic" ||
+ S == "text" || S == "wxneeded" || S.startswith("max-page-size=") ||
+ S.startswith("stack-size=");
 }
 
 // Report an error for an unknown -z option.
@@ -873,8 +888,8 @@
   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->ZInitfirst = hasZOption(Args, 

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

[Michał Górny via Phabricator via cfe-commits]

mgorny marked 5 inline comments as done.
mgorny added inline comments.



Comment at: ELF/Config.h:191
   bool ZGlobal;
+  GnuStackKind ZGnustack;
   bool ZHazardplt;

ruiu wrote:
> Members are (roughly) sorted alphabetically, so move this below the last 
> boolean member.
I don't really comprehend the sort key here but done ;-).



Comment at: ELF/Driver.cpp:369
  S == "nodelete" || S == "nodlopen" || S == "noexecstack" ||
+ S == "nognustack" ||
  S == "nokeep-text-section-prefix" || S == "norelro" || S == "notext" 
||

ruiu wrote:
> If you have clang-format, please run it on this file.
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

Overall looks good.

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

Yes.




Comment at: ELF/Config.h:191
   bool ZGlobal;
+  GnuStackKind ZGnustack;
   bool ZHazardplt;

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



Comment at: ELF/Driver.cpp:352
+  return GnuStackKind::Exec;
+else if (StringRef("noexecstack") == Arg->getValue())
+  return GnuStackKind::NoExec;

nit: no `else` after `return`.



Comment at: ELF/Driver.cpp:369
  S == "nodelete" || S == "nodlopen" || S == "noexecstack" ||
+ S == "nognustack" ||
  S == "nokeep-text-section-prefix" || S == "norelro" || S == "notext" 
||

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Michał Górny via Phabricator via cfe-commits]

mgorny added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

> 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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Michał Górny via Phabricator via cfe-commits]

mgorny added a comment.

Ping.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Michał Górny via Phabricator via cfe-commits]

mgorny updated this revision to Diff 183729.
mgorny added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554

Files:
  ELF/Config.h
  ELF/Driver.cpp
  ELF/Writer.cpp
  docs/ld.lld.1
  test/ELF/gnustack.s

Index: test/ELF/gnustack.s
===
--- test/ELF/gnustack.s
+++ test/ELF/gnustack.s
@@ -10,6 +10,9 @@
 # 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
@@ -35,5 +38,7 @@
 # RWX-NEXT: ]
 # RWX-NEXT: Alignment: 0
 
+# NOGNUSTACK-NOT: Type: PT_GNU_STACK
+
 .globl _start
 _start:
Index: docs/ld.lld.1
===
--- docs/ld.lld.1
+++ docs/ld.lld.1
@@ -509,6 +509,10 @@
 .Dv DF_1_NOOPEN
 flag to indicate that the object may not be opened by
 .Xr dlopen 3 .
+.It Cm nognustack
+Do not emit the
+.Dv PT_GNU_STACK
+segment.
 .It Cm norelro
 Do not indicate that portions of the object shold be mapped read-only
 after initial relocation processing.
Index: ELF/Writer.cpp
===
--- ELF/Writer.cpp
+++ ELF/Writer.cpp
@@ -1976,14 +1976,16 @@
   if (OutputSection *Cmd = findSection(".openbsd.randomdata"))
 AddHdr(PT_OPENBSD_RANDOMIZE, Cmd->getPhdrFlags())->add(Cmd);
 
-  // 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)
-Perm |= PF_X;
-  AddHdr(PT_GNU_STACK, Perm)->p_memsz = Config->ZStackSize;
+  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->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
Index: ELF/Driver.cpp
===
--- ELF/Driver.cpp
+++ ELF/Driver.cpp
@@ -345,6 +345,20 @@
   return Default;
 }
 
+static GnuStackKind getZGnuStack(opt::InputArgList ) {
+  for (auto *Arg : Args.filtered_reverse(OPT_z)) {
+if (StringRef("execstack") == Arg->getValue())
+  return GnuStackKind::Exec;
+else if (StringRef("noexecstack") == Arg->getValue())
+  return GnuStackKind::NoExec;
+else 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" ||
@@ -352,6 +366,7 @@
  S == "keep-text-section-prefix" || S == "lazy" || S == "muldefs" ||
  S == "nocombreloc" || S == "nocopyreloc" || S == "nodefaultlib" ||
  S == "nodelete" || S == "nodlopen" || S == "noexecstack" ||
+ S == "nognustack" ||
  S == "nokeep-text-section-prefix" || S == "norelro" || S == "notext" ||
  S == "now" || S == "origin" || S == "relro" || S == "retpolineplt" ||
  S == "rodynamic" || S == "text" || S == "wxneeded" ||
@@ -868,8 +883,8 @@
   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->ZInitfirst = hasZOption(Args, "initfirst");
   Config->ZInterpose = hasZOption(Args, "interpose");
Index: ELF/Config.h
===
--- ELF/Config.h
+++ ELF/Config.h
@@ -61,6 +61,9 @@
 // For tracking ARM Float Argument PCS
 enum class ARMVFPArgKind { Default, Base, VFP, ToolChain };
 
+// For -z *stack
+enum class GnuStackKind { None, Exec, NoExec };
+
 struct SymbolVersion {
   llvm::StringRef Name;
   bool IsExternCpp;
@@ -184,8 +187,8 @@
   bool WriteAddends;
   bool ZCombreloc;
   bool ZCopyreloc;
-  bool 

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

[Michał Górny via Phabricator via cfe-commits]

mgorny added a comment.

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.


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Michał Górny via Phabricator via cfe-commits]

mgorny added a comment.

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?


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

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


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Michał Górny via Phabricator via cfe-commits]

mgorny added a comment.

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


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

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.


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Michał Górny via Phabricator via cfe-commits]

mgorny marked 2 inline comments as done.
mgorny added a comment.

@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.


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

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


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kamil Rytarowski via Phabricator via cfe-commits]

krytarowski added a comment.

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.




Comment at: ELF/Writer.cpp:1979
 
-  // 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)
-Perm |= PF_X;
-  AddHdr(PT_GNU_STACK, Perm)->p_memsz = Config->ZStackSize;
+  if (!Config->ZNognustack) {
+// PT_GNU_STACK is a special section to tell the loader to make the

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


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

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.


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kamil Rytarowski via Phabricator via cfe-commits]

krytarowski added a comment.

@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 deserves to use it.

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


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Michał Górny via Phabricator via cfe-commits]

mgorny added a comment.

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.


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Michał Górny via Phabricator via cfe-commits]

mgorny added a comment.

@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.


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kamil Rytarowski via Phabricator via cfe-commits]

krytarowski added a comment.

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 noexecstack` in the NetBSD driver (once we will 
get an agreement that we customize the linker through the driver).


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

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.


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kamil Rytarowski via Phabricator via cfe-commits]

krytarowski added a comment.

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


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kamil Rytarowski via Phabricator via cfe-commits]

krytarowski added a comment.

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


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

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?


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kamil Rytarowski via Phabricator via cfe-commits]

krytarowski added a comment.

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


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kamil Rytarowski via Phabricator via cfe-commits]

krytarowski added a comment.

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.


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

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.


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kamil Rytarowski via Phabricator via cfe-commits]

krytarowski added a comment.

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 recognize this  
ELF segment.


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

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.


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kamil Rytarowski via Phabricator via cfe-commits]

krytarowski added inline comments.



Comment at: ELF/Driver.cpp:874
   Config->ZGlobal = hasZOption(Args, "global");
+  Config->ZNognustack = hasZOption(Args, "nognustack");
   Config->ZHazardplt = hasZOption(Args, "hazardplt");

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.


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kamil Rytarowski via Phabricator via cfe-commits]

krytarowski added inline comments.



Comment at: ELF/Writer.cpp:1980
+  if (!Config->ZNognustack) {
+// 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

section -> segment?



Comment at: docs/ld.lld.1:515
+.Dv PT_GNU_STACK
+segment.
 .It Cm norelro

mgorny wrote:
> krytarowski wrote:
> > section?
> I think 'segment' is actually the correct term here.
I see, probably right.


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Michał Górny via Phabricator via cfe-commits]

mgorny marked an inline comment as done.
mgorny added inline comments.



Comment at: docs/ld.lld.1:515
+.Dv PT_GNU_STACK
+segment.
 .It Cm norelro

krytarowski wrote:
> section?
I think 'segment' is actually the correct term here.


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kamil Rytarowski via Phabricator via cfe-commits]

krytarowski added a comment.

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


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kamil Rytarowski via Phabricator via cfe-commits]

krytarowski added inline comments.



Comment at: docs/ld.lld.1:515
+.Dv PT_GNU_STACK
+segment.
 .It Cm norelro

section?


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56554/new/

https://reviews.llvm.org/D56554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Michał Górny via Phabricator via cfe-commits]

mgorny created this revision.
mgorny added reviewers: ruiu, krytarowski.
Herald added subscribers: llvm-commits, arichardson, emaste.
Herald added a reviewer: espindola.

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.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D56554

Files:
  ELF/Config.h
  ELF/Driver.cpp
  ELF/Writer.cpp
  docs/ld.lld.1
  test/ELF/gnustack.s


Index: test/ELF/gnustack.s
===
--- test/ELF/gnustack.s
+++ test/ELF/gnustack.s
@@ -10,6 +10,9 @@
 # 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
@@ -35,5 +38,7 @@
 # RWX-NEXT: ]
 # RWX-NEXT: Alignment: 0
 
+# NOGNUSTACK-NOT: Type: PT_GNU_STACK
+
 .globl _start
 _start:
Index: docs/ld.lld.1
===
--- docs/ld.lld.1
+++ docs/ld.lld.1
@@ -509,6 +509,10 @@
 .Dv DF_1_NOOPEN
 flag to indicate that the object may not be opened by
 .Xr dlopen 3 .
+.It Cm nognustack
+Do not emit the
+.Dv PT_GNU_STACK
+segment.
 .It Cm norelro
 Do not indicate that portions of the object shold be mapped read-only
 after initial relocation processing.
Index: ELF/Writer.cpp
===
--- ELF/Writer.cpp
+++ ELF/Writer.cpp
@@ -1976,14 +1976,16 @@
   if (OutputSection *Cmd = findSection(".openbsd.randomdata"))
 AddHdr(PT_OPENBSD_RANDOMIZE, Cmd->getPhdrFlags())->add(Cmd);
 
-  // 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)
-Perm |= PF_X;
-  AddHdr(PT_GNU_STACK, Perm)->p_memsz = Config->ZStackSize;
+  if (!Config->ZNognustack) {
+// 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)
+  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
Index: ELF/Driver.cpp
===
--- ELF/Driver.cpp
+++ ELF/Driver.cpp
@@ -352,6 +352,7 @@
  S == "keep-text-section-prefix" || S == "lazy" || S == "muldefs" ||
  S == "nocombreloc" || S == "nocopyreloc" || S == "nodefaultlib" ||
  S == "nodelete" || S == "nodlopen" || S == "noexecstack" ||
+ S == "nognustack" ||
  S == "nokeep-text-section-prefix" || S == "norelro" || S == "notext" 
||
  S == "now" || S == "origin" || S == "relro" || S == "retpolineplt" ||
  S == "rodynamic" || S == "text" || S == "wxneeded" ||
@@ -870,6 +871,7 @@
   Config->ZCopyreloc = getZFlag(Args, "copyreloc", "nocopyreloc", true);
   Config->ZExecstack = getZFlag(Args, "execstack", "noexecstack", false);
   Config->ZGlobal = hasZOption(Args, "global");
+  Config->ZNognustack = hasZOption(Args, "nognustack");
   Config->ZHazardplt = hasZOption(Args, "hazardplt");
   Config->ZInitfirst = hasZOption(Args, "initfirst");
   Config->ZInterpose = hasZOption(Args, "interpose");
Index: ELF/Config.h
===
--- ELF/Config.h
+++ ELF/Config.h
@@ -193,6 +193,7 @@
   bool ZNodefaultlib;
   bool ZNodelete;
   bool ZNodlopen;
+  bool ZNognustack;
   bool ZNow;
   bool ZOrigin;
   bool ZRelro;


Index: test/ELF/gnustack.s
===
--- test/ELF/gnustack.s
+++ test/ELF/gnustack.s
@@ -10,6 +10,9 @@
 # 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
@@ -35,5 +38,7 @@
 # RWX-NEXT: ]
 # RWX-NEXT: Alignment: 0
 
+# NOGNUSTACK-NOT: Type: PT_GNU_STACK
+
 .globl _start
 _start:
Index: docs/ld.lld.1
===
--- docs/ld.lld.1
+++ docs/ld.lld.1
@@ -509,6 +509,10 @@
 .Dv DF_1_NOOPEN
 flag to indicate