This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Fix X86 & X86_64 PLT retpoline padding
ClosedPublic

Authored by andrewng on Mar 20 2018, 8:21 AM.

Details

Summary

The PLT retpoline support for X86 and X86_64 did not include the padding
when writing the header and entries. This issue was revealed when linker
scripts were used, as this disables the built-in behaviour of filling
the last page of executable segments with trap instructions. This
particular behaviour was hiding the missing padding.

Diff Detail

Event Timeline

andrewng created this revision.Mar 20 2018, 8:21 AM
ruiu added a comment.Mar 20 2018, 10:41 AM

Instead of doing this, we should fill executable segments with trap instructions even if linker script is in use, shouldn't we?

ELF/Arch/X86.cpp
184

I don't like to sprinkle too many asserts in code.

Instead of doing this, we should fill executable segments with trap instructions even if linker script is in use, shouldn't we?

The issue with linker scripts is that they force SingleRoRx. This causes text and data to be mixed.

These tests should probably use --no-rosegment instead of linkerscripts.

ruiu added a comment.Mar 20 2018, 5:00 PM

Instead of doing this, we should fill executable segments with trap instructions even if linker script is in use, shouldn't we?

The issue with linker scripts is that they force SingleRoRx. This causes text and data to be mixed.

Hm, then what is the point of this change? We could fill unused .plt bytes with trap instructions, but you still have plenty of bytes that are executable if all data is executable.

Do we need this fix in 5.0.2 and 6.0.1?

grimar added a subscriber: grimar.Mar 21 2018, 1:55 AM

Instead of doing this, we should fill executable segments with trap instructions even if linker script is in use, shouldn't we?

The current behaviour is only to fill the last page of executable segments with trap instructions. So even in the non linker script case, there still could be zero rather than trap instruction padding in the PLT. I don't know how big an issue this really is, but it feels better to be padding with trap instructions.

Instead of doing this, we should fill executable segments with trap instructions even if linker script is in use, shouldn't we?

The issue with linker scripts is that they force SingleRoRx. This causes text and data to be mixed.

These tests should probably use --no-rosegment instead of linkerscripts.

I don't think that this issue is directly related to this behaviour of linker scripts.

ELF/Arch/X86.cpp
184

Would you like me to remove all the asserts or just this one?

> The current behaviour is only to fill the last page of executable segments with trap instructions. So even in the non linker script case, there still could be zero rather than trap instruction padding in the PLT. I don't know how big an issue this really is, but it feels better to be padding with trap instructions.

OK, I think there is a far more general issue at play:

https://bugs.llvm.org/show_bug.cgi?id=36853

We should probably fix the non linkerscript non --no-rosegment case first and see if it covers this too.

ruiu added a comment.Mar 22 2018, 1:20 PM

Do we need this fix in 5.0.2 and 6.0.1?

I don't think so because the feature is "good to have". GNU linkers don't do this.

Do you want it in .0.1 releases?

I think the PLT writing should handle the padding itself. It certainly feels more correct. It looks though D44775 is heading in that direction too.

andrewng updated this revision to Diff 139806.Mar 26 2018, 9:25 AM

Removed assertions and tidied up white space a little.

espindola accepted this revision.Mar 28 2018, 9:37 PM

Given the discussion on the other threads I agree. There are two independent issues:

  • Space outside of some sections not having traps
  • Space inside some synthetic sections not having traps.

This patch fixes the second one.
Sorry about the confusion on my side.

This revision is now accepted and ready to land.Mar 28 2018, 9:37 PM
This revision was automatically updated to reflect the committed changes.