[Lldb-commits] [PATCH] D102428: [StopInfoMachException] Summarize arm64e BLRAx/LDRAx auth failures

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a reviewer: DavidSpickett.
DavidSpickett added a comment.

I think the test files should go in `test/API/functionalities/ptrauth_diagnostics/` instead.



================
Comment at: lldb/packages/Python/lldbsuite/test/functionalities/ptrauth_diagnostics/BLRAA_error/blraa.c:19
+
+// Before:
+#if 0
----------------
What is the purpose of the `// Before:` blocks here? Simply to give you a clue what happened if the test fails, or something else?

(I guess I'm really saying "Before", before what exactly)


================
Comment at: lldb/packages/Python/lldbsuite/test/functionalities/ptrauth_diagnostics/LDRAA_error/TestPtrauthLDRAADiagnostic.py:5
+lldbinline.MakeInlineTest(__file__, globals(),
+        [decorators.skipIf(archs=decorators.no_match(['arm64e']))])
----------------
If you're on/connecting to arm64e you can assume a toolchain that supports ptrauth, correct?

(we (Linaro) will probably extend these tests for to run on AArch64 Linux and there we would need to check toolchain support or use hint space)


================
Comment at: lldb/packages/Python/lldbsuite/test/functionalities/ptrauth_diagnostics/brkC47x_code/brkC47x.c:10
+      "mov x16, %[target] \n"
+      "brk 0xc470 \n"
+      /* Outputs */  :
----------------
Can you explain in this test what `brk 0xc470` means?


================
Comment at: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp:1055
+  auto InstrDesc = m_instr_info_up->get(mc_inst.getOpcode());
+  bool IsBrkC47x = false;
+  if (InstrDesc.isTrap() && mc_inst.getNumOperands() == 1) {
----------------
Comment what the meaning of the break code is. (or range of codes, 0-4 I guess)

Best guess is that this brk causes the cpu to authenticate a code held in x16, some kind of control flow check?


================
Comment at: lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp:92
+  Process &process = *exe_ctx.GetProcessPtr();
+  ABISP abi_sp = process.GetABI();
+  const ArchSpec &arch = target.GetArchitecture();
----------------
Worth asserting that abi_sp is valid? Though for Mach it's probably going to be all of the time.


================
Comment at: lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp:121
+    uint64_t bad_address = X16Val.GetAsUInt64();
+    if (bad_address == LLDB_INVALID_ADDRESS)
+      return false;
----------------
Are you comparing against `LLDB_INVALID_ADDRESS` to check whether you were able to read x16, or do you not expect to see `UINT64_MAX` to ever fail to authenticate in this context?

Seems to me like 0xF....F ending up in a register would likely cause an auth failure but I could have the wrong end of the stick here.


================
Comment at: lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp:124
+
+    uint64_t fixed_bad_address = abi_sp->FixCodeAddress(bad_address);
+    Address brk_address;
----------------
Do we know that the address in x16 is always a code address?

Though if MacOS isn't using separate ptrauth masks maybe it doesn't matter.


================
Comment at: lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp:126
+    Address brk_address;
+    if (!target.ResolveLoadAddress(fixed_bad_address, brk_address))
+      return false;
----------------
What does it mean here that the address failed to resolve?


================
Comment at: lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp:179
+  // If an authenticated call or tail call results in an exception, stripping
+  // the bad address should give the current PC.
+  if (bad_address != current_pc && fixed_bad_address == current_pc) {
----------------
I would add something like:
```
The current PC which points to the address we tried to branch to.
```
Which gives some context to the `return_pc - 4` earlier.


================
Comment at: lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp:185
+      Address blr_address;
+      if (!target.ResolveLoadAddress(return_pc - 4, blr_address))
+        return false;
----------------
Again what would it mean if this doesn't resolve? (just wondering if there's some handling to be done or ignoring it is fine)

Guessing that it could mean that the branch was to some random address that isn't in any memory currently mapped.


================
Comment at: lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp:204
+  //
+  // Is there a motivating, non-malicious code snippet that corrupts LR?
+
----------------
I don't know if you only want "correct" code but I've mistakenly clobbered LR in inline assembly before without putting it in the clobber list. Would that count?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102428/new/

https://reviews.llvm.org/D102428