Page MenuHomePhabricator

[AArch64] __builtin_return_address for PAuth.
AcceptedPublic

Authored by danielkiss on Feb 24 2020, 5:08 AM.

Details

Summary

This change adds the support for __builtin_return_address
for ARMv8.3A Pointer Authentication.
Location of the authentication code in the pointer depends on
the system configuration, therefore a dedicated instruction is used for
effectively removing the authentication code without
authenticating the pointer.

Diff Detail

Event Timeline

danielkiss created this revision.Feb 24 2020, 5:08 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 24 2020, 5:08 AM
danielkiss marked 2 inline comments as done.Feb 25 2020, 4:56 AM
danielkiss added inline comments.
clang/test/CodeGen/arm64-extractreturnaddress.c
15 ↗(On Diff #246183)

CHECK-NEXT: entry:
CHECK-NEXT: ret i8* inttoptr (i64 42 to i8*)

16 ↗(On Diff #246183)

CHECK-PAC: %0 = call i8* @llvm.extractreturnaddress.p0i8.p0i8(i8* inttoptr (i64 42 to i8*))
CHECK-PAC83: %0 = call i8* @llvm.extractreturnaddress.p0i8.p0i8(i8* inttoptr (i64 42 to i8*))
CHECK-BPROT8: %0 = call i8* @llvm.extractreturnaddress.p0i8.p0i8(i8* inttoptr (i64 42 to i8*))
CHECK-BPROT83: %0 = call i8* @llvm.extractreturnaddress.p0i8.p0i8(i8* inttoptr (i64 42 to i8*))

danielkiss added a reviewer: olista01.

rebased and test is updated.

danielkiss edited reviewers, added: chill; removed: momchil.velikov.Apr 1 2020, 1:43 AM
chill added a comment.Apr 7 2020, 8:01 AM

Needs a test in clang/test that __builtin_extract_return_address is translated to llvm.extractreturnaddress.

What if LLVM IR contains a call to llvm.extractreturnaddress, but the target is not AArch64?

llvm/include/llvm/CodeGen/ISDOpcodes.h
491–492

Needs a comment about EXTRACTRETURNADDR. And also a slightly different grouping, so the non-commented/undocumented things stand out.

chill added inline comments.Apr 7 2020, 8:03 AM
clang/lib/CodeGen/TargetInfo.cpp
5151 ↗(On Diff #254921)

Can drop the extra braces here.

5156 ↗(On Diff #254921)

Is this necessary, the intrinsic already has IntrNoMem?

chill added a comment.Apr 7 2020, 8:04 AM

Needs a test in clang/test that __builtin_extract_return_address is translated to llvm.extractreturnaddress.

Nevermind, I'm blind.

Rebased and address review comments.

danielkiss marked 4 inline comments as done.Apr 8 2020, 10:50 AM
danielkiss added inline comments.
llvm/include/llvm/CodeGen/ISDOpcodes.h
491–492

agree, I. added documentation for all now.

danielkiss marked an inline comment as done.
danielkiss retitled this revision from [AArch64] __builtin_extract_return_addr for PAuth. to [AArch64] __builtin_return_address for PAuth..
danielkiss edited the summary of this revision. (Show Details)

Changing the implementation from builtin_extract_return_addr to builtin_return_address.
Stripping PAC in __builtin_return_address is a better solution, because:

  • we should not expose PAC bits to the user because it could be passed around and that would be an ABI break.
  • builtin_extract_return_addr has a pair, the builtin_frob_return_addr which won't be supported with PAC.

I'm thinking of adding a warning if __builtin_frob_return_addr is used with PAC on.
Same change will be proposed from gcc too.

chill added a comment.EditedJul 21 2020, 3:43 PM

I'm afraid the patch does not work yet. For example, when the following program

void *f() {
	void g();
	g();
	return __builtin_return_address(0);
}

is compiled with

./bin/clang -target aarch64-eabi -march=armv8.3-a  -mbranch-protection=pac-ret -S -O2 h.c

The issue is that the definition of the instructions XPAC{D,I} is incorrect: it does not mention at all the operand to those insns.
Another problem is that the patch does not work with -O0. When compiling without optimisations, AArch64 backend used GlobalISel.

I have patches for these two issues. I'll post the one for XPAC{D,I} tomorrow and perhaps in a couple of days the GlobalISel one and we're good to go.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
6495

We shouldn't be hardcoding the X0 register here. We already have the encoded return address in ReturnAddress
can simply do:

SDNode *St = DAG.getMachineNode(AArch64::XPACI, DL, VT, ReturnAddress);
6500

Rename Reg to Chain.

chill added a comment.Jul 22 2020, 1:30 AM

The issue is that the definition of the instructions XPAC{D,I} is incorrect: it does not mention at all the operand to those insns.

Err, they do mention the operand, but only as an input one, it should be input/output.

fix review comments.

danielkiss marked 2 inline comments as done.Jul 23 2020, 2:17 AM
chill accepted this revision.Jul 23 2020, 10:21 AM

LGTM. Thanks!

This revision is now accepted and ready to land.Jul 23 2020, 10:21 AM
chill requested changes to this revision.Jul 31 2020, 3:03 AM
chill added a reviewer: ab.

Let's postpone this just for a little bit, to settle on an approach to depth > 0.

This revision now requires changes to proceed.Jul 31 2020, 3:04 AM
chill added a comment.Jul 31 2020, 3:05 AM

Let's postpone this just for a little bit, to settle on an approach to depth > 0.

This is with regard to https://reviews.llvm.org/D84502#inline-779900

Always emit xpac* instructions as discussed in D84502.

chill accepted this revision.Aug 20 2020, 12:47 AM
This revision is now accepted and ready to land.Aug 20 2020, 12:47 AM

updating tests too. check-llvm, check-clang passes.

@chill Could you check the latest update? I think it should be submitted with D84502 together.