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.
Details
Diff Detail
Event Timeline
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. |
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.
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.
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.
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.
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.
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.
I don't like to sprinkle too many asserts in code.