Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[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

There are a very large number of changes, so older changes are hidden. Show Older Changes
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.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
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.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
10702

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.
10720–10749

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
10744

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