Page MenuHomePhabricator

[PowerPC] implemented @llvm.ppc.kill.canary to corrupt stack guard
Needs ReviewPublic

Authored by pscoro on Jul 1 2022, 2:30 PM.

Details

Reviewers
shchenz
rzurob
nemanjai
Group Reviewers
Restricted Project
Summary

If stack protection is enabled, the @llvm.ppc.kill.canary() intrinsic will load the stack guard canary word, corrupt it, and store it back. This feature exists for debugging/testing stack protection.

Diff Detail

Event Timeline

pscoro created this revision.Jul 1 2022, 2:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2022, 2:30 PM
pscoro requested review of this revision.Jul 1 2022, 2:30 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 1 2022, 2:30 PM
pscoro edited the summary of this revision. (Show Details)Jul 1 2022, 2:33 PM
pscoro added reviewers: shchenz, rzurob.
pscoro added a comment.Jul 1 2022, 2:35 PM

Sorry about making a separate review for this again, I encountered some troubles with git and decided that moving my code to a fresh branch would be quicker than figuring out how to fix the problem, the previous review will be abandoned.

pscoro added inline comments.Jul 1 2022, 2:42 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
10718

To address a comment on the previous review, I fixed the XOR to do what I intended. When you XOR bits against 1, you are guaranteed to not return the same bit because of exclusivity. Therefore XORing the canary word against 0xFFFFFFFF (0b1111...111) guarantees that the corrupted canary word is never the same as the original

10725

Addressing a comment from the previous review, GV != nullptr can not be an assert because linux implements stack guard loading differently than aix. This review now also supports linux as well

pscoro updated this revision to Diff 441793.Jul 1 2022, 3:08 PM

Combined aix and linux IR lit tests into a single file

Kai added a subscriber: Kai.Jul 5 2022, 6:55 AM

Did you consider to make this more generalized? From skimming through the change, I see only 2 ppc-specific things:

  • Loading of the canary word
  • Value of XORWord

The first bullet is solved by the the other inline comment I made.
The second bullet can be solved by using all 64 bits. If a 32 bit value is used, the upper bits are ignored because the VT should be MVT::i32 in this case.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
10722–10724

I think this can be simplified using getSDagStackGuard().

10725

You can simplify the code by reordering the conditions:

if (useLoadStackGuardNode()) {
  // LOAD_STACK_GUARD
} else if (Value *GV = getSDagStackGuard(M)) {
  // AIX implementation
} else
  llvm_unreachable("Unhandled stack guard case");
pscoro updated this revision to Diff 442320.Jul 5 2022, 8:48 AM

small revisions

pscoro marked 2 inline comments as done.Jul 5 2022, 8:56 AM
pscoro updated this revision to Diff 442368.Jul 5 2022, 11:40 AM

removed commented code

Kai added a comment.Jul 5 2022, 12:04 PM

Some more nits, otherwise LGTM.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
10718

Line too long.

10725

Please remove dead code.

10726

Line too long.

10753

Line too long.

10756

Line too long.

nemanjai requested changes to this revision.Jul 5 2022, 12:08 PM

Please run clang-format, rebase and re-upload. It doesn't apply cleanly to ToT.

This revision now requires changes to proceed.Jul 5 2022, 12:08 PM
pscoro updated this revision to Diff 442598.Jul 6 2022, 8:40 AM

clean up

pscoro updated this revision to Diff 442611.Jul 6 2022, 9:27 AM

undo full file clang-format, localized clang-format

pscoro updated this revision to Diff 442615.Jul 6 2022, 9:39 AM

trying to localize calng-format again

pscoro updated this revision to Diff 442630.Jul 6 2022, 10:26 AM

patch appication troubleshooting

pscoro updated this revision to Diff 442640.Jul 6 2022, 11:09 AM

trying to get patch to apply

pscoro updated this revision to Diff 442643.Jul 6 2022, 11:11 AM

patch application

pscoro abandoned this revision.Jul 6 2022, 12:06 PM
pscoro updated this revision to Diff 442678.Jul 6 2022, 1:41 PM

patch fix

pscoro added inline comments.Jul 6 2022, 1:56 PM
llvm/include/llvm/Debuginfod/Debuginfod.h
34 ↗(On Diff #442678)

Im really unsure how this change got in here, this is not added by me and on a freshly created branch and pull

Any ideas? I'll look into this tomorrow

pscoro updated this revision to Diff 443630.Mon, Jul 11, 7:10 AM

fixed lit test

pscoro updated this revision to Diff 445853.Tue, Jul 19, 9:37 AM

formatted comments

shchenz edited reviewers, added: Restricted Project; removed: power-llvm-team.Tue, Jul 19, 10:29 PM
shchenz added a subscriber: power-llvm-team.

Seems current revision is not right. You select the wrong base commit when you create the diff?

lei added a subscriber: lei.Thu, Jul 28, 6:08 AM
lei added inline comments.
llvm/test/CodeGen/PowerPC/kill-canary-intrinsic.ll
5

missing LE run test line?

shchenz added inline comments.Thu, Jul 28, 11:19 PM
clang/test/CodeGen/PowerPC/builtins-ppc-stackprotect.c
12

nit: do we need -O2?

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
10702

Is it possible to handle this when we generate Intrinsic::ppc_kill_canary, for example in the FE? Passing and handling this useless intrinsic in all the opt passes should be avoided?

10718

nit: we may don't need this magic number here, we can use DAG.getAllOnesConstant() when we generate the XOR below?

llvm/test/CodeGen/PowerPC/kill-canary-intrinsic.ll
6

+1, we should add a RUN line for 64bit Linux LE for -mtriple=powerpc64le-unknown-linux-gnu

and for 32-bit AIX too.

22

nit: we may can add nounwind attribute to the function to remove the unnecessary cfi related instructions in the test.

35

The functionality here seems wrong, AFAIK, we should overwrite the content in the slack slot instead of the data csect. Which means the store here should be std r4, 120(r1) instead of std r4, 0(r3). Stack protection aims to check that the stack content is not changed unexpectedly.

60

Linux seems right...

pscoro updated this revision to Diff 449299.Tue, Aug 2, 8:36 AM
pscoro marked 3 inline comments as done.

Fixed test cases and AIX implementation

pscoro marked 8 inline comments as done.Tue, Aug 2, 8:41 AM
pscoro added inline comments.
llvm/test/CodeGen/PowerPC/kill-canary-intrinsic.ll
12

How many test cases should I include? I tested all these machine triples on Power 7-10, bought I thought having 16 run lines may be too much, so I only included the power 7 and 8 run lines in the review.

pscoro updated this revision to Diff 449683.Wed, Aug 3, 9:18 AM

removed comments