This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Add ROP Protection to prologue and epilogue
ClosedPublic

Authored by stefanp on Mar 25 2021, 1:46 PM.

Details

Summary

Added hashst to the prologue and hashchk to the epilogue.
The hash for the prologue and epilogue must always be stored as the first
element in the local variable space on the stack.

Diff Detail

Event Timeline

stefanp created this revision.Mar 25 2021, 1:46 PM
stefanp requested review of this revision.Mar 25 2021, 1:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2021, 1:46 PM
stefanp added reviewers: lei, amyk, Restricted Project.Mar 25 2021, 1:46 PM
nemanjai added inline comments.Mar 31 2021, 10:23 AM
llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
155

I think this needs to be added to FastISEL as well and we need to add a test case with -O0.

stefanp updated this revision to Diff 335314.Apr 5 2021, 1:32 PM

Updated test case to include -O0.

Initially I agreed that we needed to add the CreateStackObject to FastISEL as
well. However, FastISEL is only triggered when

SelectionDAGISel::runOnMachineFunction(MF);

is called. Since this CreateStackObject comes before that call it will be done
for ROP at all opt levels.

amyk added inline comments.Apr 5 2021, 4:38 PM
llvm/test/CodeGen/PowerPC/ppc64-rop-protection.ll
136

Since the test file is large, I'm wondering if some of the CHECKs can be combined/maybe we can use some common check-prefixes (if that's a good idea)?
For example, it looks like LE-P9-O0 and LE-P8-O0 look similar. As well as BE-P9 and BE-P8.

stefanp added inline comments.Apr 7 2021, 7:09 AM
llvm/test/CodeGen/PowerPC/ppc64-rop-protection.ll
136

Since the test file is large, I'm wondering if some of the CHECKs can be combined/maybe we can use some common check-prefixes (if that's a good idea)?
For example, it looks like LE-P9-O0 and LE-P8-O0 look similar. As well as BE-P9 and BE-P8.

I agree that this test is really large (~3500 lines).

When I wrote the original testcase I didn't use update_llc_test_checks.py and I had merged a number of these tests. I was using CHECK, CHECK-NEXT, CHECK-DAG, and I was trying to get rid of lines of code that didn't matter. After that, I added the -O0 tests and realized that this was all becoming too hard to maintain. Therefore, I switched to using the script to generate the checks. I could certainly merge some of these tests but I fear that would make things hard to maintain again. If two codegen patterns are the same now there is no guarantee that two months from now that will be the case and whoever is updating the test will have a hard time figuring out how to update it.

As a result I would prefer to leave things as they are now.
The other option is to have only the most basic checks that would work in all situations.

CHECK-LABEL:
CHECK: hashst <OFFSET>
CHECK: hashchk <OFFSET>
CHECK: blr

I would be happy to (significantly!) shirk the test like that but I am afraid that it might not cover enough code...
Anyway, let me know what you think.

nemanjai accepted this revision.Apr 8 2021, 7:40 AM

LGTM. I agree that the test case is extremely verbose but this is one of those things that unfortunately has so many interactions that it has to. And producing checks with a script makes it more maintainable as well as making it easy to diagnose any failures.

llvm/test/CodeGen/PowerPC/ppc64-rop-protection.ll
359

Please add a comment that this is technically a violation of the ABI since it is outside the red zone, but that the restriction is likely to be removed in an upcoming revision. Similar comment should go into the code.

This revision is now accepted and ready to land.Apr 8 2021, 7:40 AM
stefanp updated this revision to Diff 343552.May 6 2021, 6:25 PM

Rebased to top of trunk.

Added two comments.

This revision was automatically updated to reflect the committed changes.