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

[Vedant Kumar via Phabricator via lldb-commits]

vsk added a comment.

Thanks for the review!



================
Comment at: lldb/packages/Python/lldbsuite/test/functionalities/ptrauth_diagnostics/BLRAA_error/blraa.c:19
+
+// Before:
+#if 0
----------------
DavidSpickett wrote:
> 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)
These 'Before' blocks show the reader how lldb interpreted auth-related exceptions prior to this change, and show the expected codegen (handy if the test ever regresses).


================
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']))])
----------------
DavidSpickett wrote:
> 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)
Yes, that's right. I don't believe there's anything Darwin-specific hardcoded into these test cases, and hope that they're reusable for AArch64 Linux testing.


================
Comment at: lldb/packages/Python/lldbsuite/test/functionalities/ptrauth_diagnostics/brkC47x_code/brkC47x.c:10
+      "mov x16, %[target] \n"
+      "brk 0xc470 \n"
+      /* Outputs */  :
----------------
DavidSpickett wrote:
> Can you explain in this test what `brk 0xc470` means?
In certain cases, AppleClang may insert auth traps in software: `Use brk 0xc470 + aut key, where 0x70 == 'p', 0xc4 == 'a' + 'c'`.


================
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) {
----------------
DavidSpickett wrote:
> 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?
Will do.


================
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();
----------------
DavidSpickett wrote:
> Worth asserting that abi_sp is valid? Though for Mach it's probably going to be all of the time.
I went ahead and added the assert.


================
Comment at: lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp:121
+    uint64_t bad_address = X16Val.GetAsUInt64();
+    if (bad_address == LLDB_INVALID_ADDRESS)
+      return false;
----------------
DavidSpickett wrote:
> 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.
RegisterValue::GetAsUInt64() should return UINT64_MAX if it fails. I'll clean this up by checking whether the earlier ReadRegister call succeeded.


================
Comment at: lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp:124
+
+    uint64_t fixed_bad_address = abi_sp->FixCodeAddress(bad_address);
+    Address brk_address;
----------------
DavidSpickett wrote:
> 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.
For now we assume that the code address fixup is always appropriate for Darwin/arm64e.


================
Comment at: lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp:126
+    Address brk_address;
+    if (!target.ResolveLoadAddress(fixed_bad_address, brk_address))
+      return false;
----------------
DavidSpickett wrote:
> What does it mean here that the address failed to resolve?
It's possible that lldb doesn't know about the image the fixed address points to (it could be a garbage value). In this case we conservatively don't hint that there's a ptrauth issue.


================
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) {
----------------
DavidSpickett wrote:
> 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.
Will do.


================
Comment at: lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp:185
+      Address blr_address;
+      if (!target.ResolveLoadAddress(return_pc - 4, blr_address))
+        return false;
----------------
DavidSpickett wrote:
> 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.
Ignoring it (i.e. declining to hint at ptrauth failure) seems fine.


================
Comment at: lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp:204
+  //
+  // Is there a motivating, non-malicious code snippet that corrupts LR?
+
----------------
DavidSpickett wrote:
> 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?
We'd like to have lldb be smart enough to diagnose this. It's just that, early on, in our survey of auth issues encountered during bringup, this wasn't a common case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102428