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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

In D64903#1590894 <https://reviews.llvm.org/D64903#1590894>, @MaskRay wrote:

> In D64903#1590893 <https://reviews.llvm.org/D64903#1590893>, @grimar wrote:
>
> > My first impression: seems adding the new `-z separate-code`/`-z noseparate-code` option is a good thing to do.
>
>
> +1
>
> > But it is probably arguable what should be the default. Can we just keep the existent behavior as a default?
>
> The description summaries my reasoning why `-z noseparate-code` should be the default. `-z separate-code` is a dubious feature that **intends** (not proven) to reduce the number of ROP gadgets (which is actually very ineffective because attackers can find plenty of gadgets in the text segment, no need to find gadgets in non-code regions)


It's a little off-topic, but I agree with Fangrui with this point. I don't think `-z separate-code` is a very effective defense against a ROP/JOP. If a program is not very small, it shouldn't be too hard to find a gadget from a text segment, and I don't think an additional piece of data that is at the remaining part of the last page of a text segment can significantly increase a risk. If your program is vulnerable to ROP/JOP, and gadgets are at predictable places, it's a game end regardless of an existence of some extra data at the end of a text segment.

> I have another patch D64906 <https://reviews.llvm.org/D64906> to demonstrate how much it can save if we use -z noseparate-code by default:
> 
>   # a is a trivial executable: `int main() {}`
>   % stat -c %s a   # before this patch
>   200712
>   % stat -c %s a   # D64906, -z noseparate-code
>   7256
>   % stat -c %s a   # D64906, -z separate-code. This still pays the cost of two alignments
>   135656
> 
> 
> 
> 
>   // before this patch
>     LOAD           0x000000 0x0000000000000000 0x0000000000000000 0x000564 0x000564 R   0x10000
>     LOAD           0x010000 0x0000000000010000 0x0000000000010000 0x0004ec 0x0004ec R E 0x10000
>     LOAD           0x020000 0x0000000000020000 0x0000000000020000 0x0001e0 0x0001e0 RW  0x10000
>     LOAD           0x030000 0x0000000000030000 0x0000000000030000 0x000010 0x000050 RW  0x10000
>   
>   // D64906, -z noseparate-code
>     LOAD           0x000000 0x0000000000000000 0x0000000000000000 0x000564 0x000564 R   0x10000                   
>     LOAD           0x000580 0x0000000000010580 0x0000000000010580 0x0004ec 0x0004ec R E 0x10000
>     LOAD           0x000a70 0x0000000000020a70 0x0000000000020a70 0x0001e0 0x0001e0 RW  0x10000                  
>     LOAD           0x000c50 0x0000000000030c50 0x0000000000030c50 0x000010 0x000050 RW  0x10000
>   
>   // D64906, -z separate-code
>     LOAD           0x000000 0x0000000000000000 0x0000000000000000 0x000564 0x000564 R   0x10000
>     LOAD           0x010000 0x0000000000010000 0x0000000000010000 0x0004ec 0x0004ec R E 0x10000
>     LOAD           0x020000 0x0000000000020000 0x0000000000020000 0x0001e0 0x0001e0 RW  0x10000
>     LOAD           0x0201e0 0x00000000000301e0 0x00000000000301e0 0x000010 0x000050 RW  0x10000




Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D64903