This is an archive of the discontinued LLVM Phabricator instance.

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

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
11150

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

11157

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
11154–11156

I think this can be simplified using getSDagStackGuard().

11157

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
11150

Line too long.

11157

Please remove dead code.

11158

Line too long.

11185

Line too long.

11188

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.Jul 11 2022, 7:10 AM

fixed lit test

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

formatted comments

shchenz edited reviewers, added: Restricted Project; removed: power-llvm-team.Jul 19 2022, 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.Jul 28 2022, 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.Jul 28 2022, 11:19 PM
clang/test/CodeGen/PowerPC/builtins-ppc-stackprotect.c
12

nit: do we need -O2?

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

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?

11142

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.Aug 2 2022, 8:36 AM
pscoro marked 3 inline comments as done.

Fixed test cases and AIX implementation

pscoro marked 8 inline comments as done.Aug 2 2022, 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.Aug 3 2022, 9:18 AM

removed comments

nemanjai added inline comments.Aug 9 2022, 5:46 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
11126

Again, this comment is nothing more than a re-reading of the code. As such, it is not useful. The comment should say what is happening and why. Something along the lines of:

// The kill_canary intrinsic only makes sense when the Stack Protector
// feature is on in the function. It can also not be used in conjunction
// with safe stack because the latter splits the stack and the canary
// value isn't used (i.e. safe stack supersedes stack protector).
// In situations where the kill_canary intrinsic is not supported,
// we simply replace uses of its chain with its input chain, causing
// the SDAG CSE to remove the node.
11144–11173

The common code should just be common. Seems like the only thing that changes is how we load the value. Please refactor this to something like:

if (useLoadStackGuardNode()) {
  ...
  Load = ... // stack guard load
} else if (Value *GV = getSDagStackGuard(*M)) {
  ...
  Load = ... // canary word global load
} else
  llvm_unreachable("Unhandled stack guard case");

Store = ... // common store
return Store; // (or you can just create the store in-line and return it directly)
llvm/test/CodeGen/PowerPC/kill-canary-intrinsic.ll
3

Please add one RUN line with -O0 to ensure that this works as expected with Fast ISel.

pscoro updated this revision to Diff 451133.Aug 9 2022, 6:35 AM

Small revisions

pscoro marked 3 inline comments as done.Aug 9 2022, 6:37 AM
nemanjai accepted this revision.Aug 9 2022, 7:58 AM

LGTM.

This revision is now accepted and ready to land.Aug 9 2022, 7:58 AM
shchenz added inline comments.Aug 9 2022, 6:52 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
11168

Nit: Maybe we need to set the chain input for store as the chain output of the load.

Op->getOperand(0) -> Load->getValue(1)

There is data dependency between the load and the store, so above change will not impact the correctness. But that makes more sense from memory access order part.

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

We may need to check why we are not storing not r3, r3 directly to 120(r1) at -O0, like std r3, 120(r1). Seems now we are storing to 119(r1)

pscoro updated this revision to Diff 452740.Aug 15 2022, 10:39 AM

chain input fix

pscoro marked an inline comment as done.Aug 15 2022, 10:40 AM
pscoro updated this revision to Diff 452990.Aug 16 2022, 6:52 AM

reverted previous fix

pscoro updated this revision to Diff 453105.Aug 16 2022, 1:05 PM

Re added chain fix

pscoro updated this revision to Diff 453272.Aug 17 2022, 6:32 AM

Reverted chain revision again, final