[AArch64][PAC] Emit a fatal error when ptrauth target feature is missing #62

kovdan01 · 2023-12-04T08:05:05Z

Previously, in expandPtrAuthPseudo we expanded MOVaddrPAC, LOADgotPAC and LOADauthptrgot pseudo-instructions without taking presense of PAuth subtarget feature into account. In case of LOADauthptrgot, it resulted in undesired so-called $auth_ptr$ stub and a corresponding AUTH relocation. In case MOVaddrPAC and LOADgotPAC, it resulted in pac-specific instructions (e.g. paciza) emitted which was only caught via assertion during machine instructions verification (and not caught at all when assertions are disabled).

This patch makes us emit a fatal error and fail fast in such cases.

Previously, in `expandPtrAuthPseudo` we expanded `MOVaddrPAC`, `LOADgotPAC` and `LOADauthptrgot` pseudo-instructions without taking presense of PAuth subtarget feature into account. In case of `LOADauthptrgot`, it resulted in undesired so-called `$auth_ptr$` stub and a corresponding AUTH relocation. In case `MOVaddrPAC` and `LOADgotPAC`, it resulted in pac-specific instructions (e.g. `paciza`) emitted which was only caught via assertion during machine instructions verification (and not caught at all when assertions are disabled). This patch makes us emit a fatal error and fail fast in such cases.

asl · 2023-12-05T23:36:40Z

@atrosinenko please review

The option is not present on assertion-disabled builds. A test without the option (and without verifying debug output ensuring which particular pseudo-instructions are expanded) is also added - it looks reasonable since the PR itself is intended to add proper error handling for assertion-disabled builds.

atrosinenko

I wonder if other code paths have to be guarded as well.

atrosinenko · 2023-12-06T13:01:11Z

llvm/test/CodeGen/AArch64/GlobalISel/ptrauth-lowering-err-debug.ll

+
+;--- MOVaddrPAC.ll
+
+; RUN: not --crash llc -debug -mtriple aarch64-elf MOVaddrPAC.ll 2>&1 | \


-debug may be replaced with -debug-only=aarch64-expand-hardened-pseudos for less verbose debug output (not sure which is more canonical in tests, though).

Thanks, fixed in 52afe42

…sing

kovdan01 · 2023-12-07T02:31:13Z

@atrosinenko Thanks for review. Several points:

See 81cb742 and 3821445 for additional tests on other code paths

I wonder if other code paths have to be guarded as well.
Switched to CHECK-NEXT where suitable 9ed8fc6

atrosinenko

Is AArch64ExpandHardenedPseudos::expandAuthLoad method expected to contain a similar check?

atrosinenko · 2023-12-07T19:08:54Z

llvm/test/CodeGen/AArch64/GlobalISel/ptrauth-lowering-err-debug.ll

+define i8* @foo() #0 {
+  %tmp = bitcast { i8*, i32, i64, i64 }* @g_weak.ptrauth to i8*
+  ret i8* %tmp
+}
+
+@g_weak = extern_weak global i32
+@g_weak.ptrauth = private constant { i8*, i32, i64, i64 } { i8* bitcast (i32* @g_weak to i8*), i32 0, i64 0, i64 0 }, section "llvm.ptrauth"


Typed pointers.

Fixed in 8e30a83

atrosinenko · 2023-12-07T19:11:43Z

llvm/test/CodeGen/AArch64/GlobalISel/ptrauth-lowering-err.ll

+define i8* @foo() #0 {
+  %tmp = bitcast { i8*, i32, i64, i64 }* @g_weak.ptrauth to i8*
+  ret i8* %tmp
+}
+
+@g_weak = extern_weak global i32
+@g_weak.ptrauth = private constant { i8*, i32, i64, i64 } { i8* bitcast (i32* @g_weak to i8*), i32 0, i64 0, i64 0 }, section "llvm.ptrauth"


Typed pointers.

Fixed in 8e30a83

… feature

kovdan01 · 2023-12-08T21:28:55Z

Is AArch64ExpandHardenedPseudos::expandAuthLoad method expected to contain a similar check?

@atrosinenko In bffc609 I've added checks in AArch64DAGToDAGISel::tryAuthLoad and before AArch64InstructionSelector::selectAuthLoad - those are the places where LDRA* pseudos are emitted. So, we should only go to AArch64ExpandHardenedPseudos::expandAuthLoad after these checks - inserted a corresponding assertion there.

atrosinenko

Sorry for too many iterations, I just spotted the tests are in the llvm/lib/Target/AArch64/GISel/ directory. Maybe these could be moved to llvm/lib/Target/AArch64 and updated to test both DAGISel and GlobalISel like it is done in ptrauth-intrinsic-blend.ll. This should make TODO unneeded, I guess.

kovdan01 · 2023-12-11T17:02:48Z

@atrosinenko Thanks, testing both with -global-isel=0 and -global-isel=1 is definitely needed. It revealed several new cases with failures.

I don't feel the approach initially taken suites best now - trying to guard every place where we potentially emit auth-specific instructions makes things harder to maintain, understand, and there are way too many points responsible for checking such a simple things.

I suppose the following approach could be better.

For each function, if PAuth subtarget feature is not enabled, see if there are "ptrauth-*" attributes ("ptrauth-calls", "ptrauth-returns", etc). We can implement a separate pass for such a check.
If PAuth subtarget feature is not enabled, check if there are any objects in "llvm.ptrauth" section.
Check llvm ptrauth intrinsics separately.

Please let me know your thoughts on this. Thanks!

kovdan01 · 2023-12-12T06:53:30Z

As discussed with @asl, we should move the checks to frontend. See #65.

kovdan01 added the pauth label Dec 4, 2023

kovdan01 requested review from asl and atrosinenko December 4, 2023 08:05

atrosinenko reviewed Dec 6, 2023

View reviewed changes

kovdan01 added 4 commits December 7, 2023 03:42

Use -debug-only=aarch64-expand-hardened-pseudos instead of -debug

52afe42

Ensure that ptrauth subtarget feature is checked for ptrauth-returns

81cb742

Ensure AUTH_CALL is not emitted when ptrauth subtarget feature is mis…

3821445

…sing

Use CHECK-NEXT instead of CHECK when suitable

9ed8fc6

kovdan01 requested a review from atrosinenko December 7, 2023 03:13

atrosinenko reviewed Dec 7, 2023

View reviewed changes

kovdan01 added 2 commits December 9, 2023 00:12

Add error messages for emitting LDRA* pseudos without PAuth subtarget…

bffc609

… feature

Use opaque pointers

8e30a83

kovdan01 requested a review from atrosinenko December 8, 2023 21:29

atrosinenko reviewed Dec 11, 2023

View reviewed changes

kovdan01 mentioned this pull request Dec 12, 2023

[PAuth][clang] Error on passing PAuth-specific options on non-PAuth target #65

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AArch64][PAC] Emit a fatal error when ptrauth target feature is missing #62

[AArch64][PAC] Emit a fatal error when ptrauth target feature is missing #62

kovdan01 commented Dec 4, 2023

asl commented Dec 5, 2023

atrosinenko left a comment

atrosinenko Dec 6, 2023

asl Dec 6, 2023

kovdan01 Dec 7, 2023

kovdan01 commented Dec 7, 2023

atrosinenko left a comment

atrosinenko Dec 7, 2023

kovdan01 Dec 8, 2023

atrosinenko Dec 7, 2023

kovdan01 Dec 8, 2023

kovdan01 commented Dec 8, 2023

atrosinenko left a comment

kovdan01 commented Dec 11, 2023

kovdan01 commented Dec 12, 2023


		;--- MOVaddrPAC.ll

		; RUN: not --crash llc -debug -mtriple aarch64-elf MOVaddrPAC.ll 2>&1 \| \

[AArch64][PAC] Emit a fatal error when ptrauth target feature is missing #62

Are you sure you want to change the base?

[AArch64][PAC] Emit a fatal error when ptrauth target feature is missing #62

Conversation

kovdan01 commented Dec 4, 2023

asl commented Dec 5, 2023

atrosinenko left a comment

Choose a reason for hiding this comment

atrosinenko Dec 6, 2023

Choose a reason for hiding this comment

asl Dec 6, 2023

Choose a reason for hiding this comment

kovdan01 Dec 7, 2023

Choose a reason for hiding this comment

kovdan01 commented Dec 7, 2023

atrosinenko left a comment

Choose a reason for hiding this comment

atrosinenko Dec 7, 2023

Choose a reason for hiding this comment

kovdan01 Dec 8, 2023

Choose a reason for hiding this comment

atrosinenko Dec 7, 2023

Choose a reason for hiding this comment

kovdan01 Dec 8, 2023

Choose a reason for hiding this comment

kovdan01 commented Dec 8, 2023

atrosinenko left a comment

Choose a reason for hiding this comment

kovdan01 commented Dec 11, 2023

kovdan01 commented Dec 12, 2023