[PATCH] D64903: [ELF] Pad the last page of last PF_X PT_LOAD with traps when -z separate-code is specified

[Petr Hosek via Phabricator via llvm-commits]

phosek added a comment.

I have several points on this.

First, all the differences are demonstrated on uncompressed binaries, but if the binary size was really important for you, you'd always use compression in practice. For example, on Fuchsia where binary size is critical, we never leave binaries uncompressed on disk. To demonstrate this, I've used the same example that @MaskRay used:

  $ echo 'int main() {}' > t.c
  $ clang --target=aarch64-linux-gnu --sysroot=path/to/sysroot t.c -o t
  $ stat -c %s t
  201136
  $ lz4 t t.lz4
  $ stat -c %s t.lz4
  4265
  $ zstd t -o t.zstd
  $ stat -c %s t.zstd
  2435

With D64906 <https://reviews.llvm.org/D64906> and D64930 <https://reviews.llvm.org/D64930> applied:

  $ stat -c %s t
  70064
  $ stat -c %s t.lz4
  3730
  $ stat -c %s t.zstd
  2401

With zstd (which is default compressor used by Fuchsia), the difference between the two schemes is 34 bytes.

Second, with `-z noseparate-code`, you'll end up with a partial page wasting additional memory at runtime So the difference between `-z separate-code` and `-z noseparate-code` isn't really about security vs space as is presented in the description, it's primarily a trade-off between memory and disk space. We can reclaim most disk space loss using compression as shown above, but we cannot reclaim the memory loss.

Third, there's the security aspect. I agree that the reduction of the number of ROP gadgets is limited, but that doesn't mean we shouldn't do it. I've consulted this change with our security team today and their response is that we always want `-z separate-code`! Why? It's a basic security hygiene, best practice, defense in depth kind of measure. It's the same reason why we fill space between functions with traps, why we use stack protectors, why we use ASLR, etc. We have known attacks against each of those mechanisms, and not all of them have proven as effective as was believed in the past, but that doesn't mean we're going to disable them. The same applies here.

With all that being said, if there's a combination of flag that would allow preserving the current behavior, we'd set those in our driver like we already do for `-z rodynamic` <https://github.com/llvm/llvm-project/blob/master/clang/lib/Driver/ToolChains/Fuchsia.cpp#L50>, so I'm not going to block this change. However, I'd still much rather see the `-z separate-code` being the default because otherwise we're going to loose the advantages I described above (memory savings, security) on other platforms like Linux.

One potential solution might be to control the default behavior with a CMake option which is similar to the solution also used in binutils by ld (`--enable-separate-code`/`--disable-separate-code`). While this isn't being used to control the defaults in lld at the moment, it's used elsewhere in LLVM for that purpose (e.g. `ENABLE_LINKER_BUILD_ID` <https://github.com/llvm/llvm-project/blob/master/clang/CMakeLists.txt#L231> or `ENABLE_X86_RELAX_RELOCATIONS` <https://github.com/llvm/llvm-project/blob/master/clang/CMakeLists.txt#L233> in Clang) and enables each toolchain vendor to choose their defaults.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D64903