Page MenuHomePhabricator

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

Authored by mgorny on Jan 10 2019, 12:01 PM.

Details

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.

Diff Detail

Event Timeline

mgorny created this revision.Jan 10 2019, 12:01 PM
krytarowski added inline comments.Jan 10 2019, 12:06 PM
docs/ld.lld.1
515

section?

krytarowski added a comment.EditedJan 10 2019, 12:07 PM

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

mgorny marked an inline comment as done.Jan 10 2019, 12:10 PM
mgorny added inline comments.
docs/ld.lld.1
515

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

krytarowski added inline comments.Jan 10 2019, 12:14 PM
ELF/Writer.cpp
1980

section -> segment?

docs/ld.lld.1
515

I see, probably right.

krytarowski added inline comments.Jan 10 2019, 12:17 PM
ELF/Driver.cpp
887

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 added a comment.Jan 10 2019, 1:57 PM

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.

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 added a comment.Jan 10 2019, 3:29 PM

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.

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.

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 added a comment.Jan 10 2019, 3:56 PM

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?

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

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

ruiu added a comment.Jan 10 2019, 4:16 PM

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 added a comment.EditedJan 10 2019, 4:23 PM

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

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

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.

grimar added a subscriber: grimar.Jan 11 2019, 12:54 AM
krytarowski added a comment.EditedJan 11 2019, 10:53 AM

@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 added a comment.Jan 11 2019, 2:54 PM

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.

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–1988

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

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

mgorny marked 2 inline comments as done.Jan 14 2019, 8:55 PM

@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 added a comment.Thu, Jan 24, 9:23 AM

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

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

ruiu added a comment.Thu, Jan 24, 9:58 AM

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

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?

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 updated this revision to Diff 183729.Sat, Jan 26, 1:52 PM

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

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?

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

ruiu added a comment.Thu, Feb 7, 5:37 PM

Overall looks good.

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

Yes.

ELF/Config.h
191

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

ELF/Driver.cpp
352

nit: no else after return.

369

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