-
Notifications
You must be signed in to change notification settings - Fork 13.4k
[llvm-exegesis][AArch64] Check for PAC keys before disabling them #138643
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@llvm/pr-subscribers-tools-llvm-exegesis Author: Abhilash Majumder (abhilash1910) Changes[llvm-exegesis][AArch64] Check for PAC keys before disabling them. Tries to resolve aarch64 error arising from accessible PAC keys from PR: Disable pauth and ldgm as unsupported instructions fixed. The suspected reason is in some systems, the pac keys are present and need to be checked before setting them. This is followup to merge the changes with following changes to attempt to fixup the arch64 failure. Changes: Apply checks to verify if PAC is enabled in some systems and disabling them . @lakshayk-nv please help to verify and review. Full diff: https://github.com/llvm/llvm-project/pull/138643.diff 1 Files Affected:
diff --git a/llvm/tools/llvm-exegesis/lib/AArch64/Target.cpp b/llvm/tools/llvm-exegesis/lib/AArch64/Target.cpp
index a1eb5a46f21fc..d1da6f41461a7 100644
--- a/llvm/tools/llvm-exegesis/lib/AArch64/Target.cpp
+++ b/llvm/tools/llvm-exegesis/lib/AArch64/Target.cpp
@@ -207,16 +207,26 @@ class ExegesisAArch64Target : public ExegesisTarget {
if (isPointerAuth(Opcode)) {
#if defined(__aarch64__) && defined(__linux__)
+
+ // Fix for some systems where PAC/PAG keys are present but is
+ // explicitly getting disabled
+ unsigned long pac_keys = 0;
+ if (prctl(PR_PAC_GET_ENABLED_KEYS, &pac_keys, 0, 0, 0) < 0) {
+ return "Failed to get PAC key status";
+ }
+
// Disable all PAC keys. Note that while we expect the measurements to
// be the same with PAC keys disabled, they could potentially be lower
// since authentication checks are bypassed.
- if (prctl(PR_PAC_SET_ENABLED_KEYS,
- PR_PAC_APIAKEY | PR_PAC_APIBKEY | PR_PAC_APDAKEY |
- PR_PAC_APDBKEY, // all keys
- 0, // disable all
- 0, 0) < 0) {
- return "Failed to disable PAC keys";
- }
+ if (pac_keys != 0) {
+ if (prctl(PR_PAC_SET_ENABLED_KEYS,
+ PR_PAC_APIAKEY | PR_PAC_APIBKEY | PR_PAC_APDAKEY |
+ PR_PAC_APDBKEY, // all keys
+ 0, // disable all
+ 0, 0) < 0) {
+ return "Failed to disable PAC keys";
+ }
+ }
#else
return "Unsupported opcode: isPointerAuth";
#endif
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please run clang-format
over the patch to fix the code formatting.
// For systems without PAC, this is a No-op but with PAC, it is | ||
// safer to check the existing key state and then disable/enable them. | ||
// Hence the guard placed for switching. | ||
unsigned long pac_keys = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@asl this is a linux pac question so I'm not sure if this is correct behavior and will leave it to you
I am curious though - this seems like a PAC behavior choice based on compile time process configuration rather than specified target info? Is this expected behavior (I don't understand how ptrauth works on linux)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Tagging @atrosinenko
Essentially we expect two things:
- If full pauth ABI is enabled, it is done in platform-specific manner. E.g. via ABI field in target quadruple for common platforms, or some other way for downstream
- PAC-RET is expected to work regardless of settings since it is an ABI-neutral change.
My expectation is that PAC instructions in hint space should be handled gracefully regardless of PAC state. But maybe I am missing some context.
[llvm-exegesis][AArch64] Check for PAC keys before disabling them.
Tries to resolve aarch64 error arising from accessible PAC keys from PR: Disable pauth and ldgm as unsupported instructions fixed. The suspected reason is in some systems, the pac keys are present and need to be checked before setting them.
This is followup to merge the changes with following changes to attempt to fixup the arch64 failure.
Changes:
Apply checks to verify if PAC is enabled in some systems and disabling them .
@lakshayk-nv please help to verify and review.
cc @sjoerdmeijer @davemgreen