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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
