Page MenuHomePhabricator

[LLDB] Support AArch64/Linux watchpoint on tagged addresses
ClosedPublic

Authored by omjavaid on Apr 27 2021, 6:40 AM.

Details

Summary

AArch64 architecture support virtual addresses with some of the top bits ignored.
These ignored bits can host memory tags or bit masks that can serve to check for
authentication of address integrity. We need to clear away the top ignored bits
from watchpoint address to reliably hit watchpoints as well set watchpoints on
addresses containing tags or masks in their top bits.

This patch adds support to watch tagged addresses on AArch64/Linux.

Diff Detail

Event Timeline

omjavaid created this revision.Apr 27 2021, 6:40 AM
DavidSpickett added inline comments.Apr 27 2021, 7:42 AM
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
892

I'm guessing this returns a Status, I think that has a .Success().

lldb/test/API/commands/watchpoints/watch_tagged_addr/TestWatchTaggedAddress.py
35

These docstrings should be more specific. E.g. this test puts a watchpoint on an untagged address, where the one below is tagged.

104

You could remove some of the continues by adding a command to the watchpoint:

(lldb) watchpoint command add 1 -o continue

Then just check the stats at the end after a single continue.

I thought at first this would be bad because you wouldn't know which hit was missed, but we don't know that with the current setup either.

DavidSpickett added inline comments.Apr 27 2021, 7:45 AM
lldb/source/Plugins/Process/Utility/NativeRegisterContextDBReg_arm64.h
77

This is going to be used by FreeBSD, have you checked if it enables top byte ignore at all?

omjavaid updated this revision to Diff 342608.May 3 2021, 5:41 PM

This fixes review comments.

omjavaid added inline comments.May 3 2021, 5:44 PM
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
892

Fixed in current rev.

lldb/source/Plugins/Process/Utility/NativeRegisterContextDBReg_arm64.h
77

In current rev I have moved a default implementation of this function which only ignores top byte. Should work for all other platforms.

lldb/test/API/commands/watchpoints/watch_tagged_addr/TestWatchTaggedAddress.py
35

Fixed in current rev.

104

I would like to avoid this and keep the current version as it makes testcase more readable for newbies.

DavidSpickett added a subscriber: mgorny.

@mgorny Does FreeBSD do anything with the top byte ignore feature?

lldb/source/Plugins/Process/Utility/NativeRegisterContextDBReg_arm64.cpp
476

I think it was fine where it was, my point was just to check whether FreeBSD enables TBI at all.

Seems like it should be an OS level query. 99% of the time we'd be fine assuming it's enabled but I don't like the idea of pretending that it is and giving a false impression.

Unless assuming TBI is safe just because the hardware registers don't hold those bits anyway. In which case the comment should justify that.

omjavaid added inline comments.May 4 2021, 5:14 AM
lldb/source/Plugins/Process/Utility/NativeRegisterContextDBReg_arm64.cpp
476

My first choice was to ignore FreeBSD implementation unless someone from FreeBSD does it after testing. The test-case in this patch is also enabled only for Linux/AArch 64.
But as an after thought, to make things uniform across LLDB, I made this change given that we are ignoring top-byte of watchpoint install address from all platforms with ABISysV on host side. Also watchpoint address in DBGWVR is 48 bits in normal VA mode and 52 bits in LVA mode so thought ignoring top byte across all platforms seemed safe.
But lets wait for what @mgorny has to say on this.

@labath @mgorny any comments on this rev?

DavidSpickett added a comment.EditedJun 16 2021, 6:22 AM

Since you landed the underlying ABI patch, looking at this again.

Seems like no comments on the BSD side, in which case to be safe we could just have the default FixWatchpointHitAddress make no changes. Then remove TBI and PAC for AArch64 Linux, no surprises that way.

Also should the test case include signing the pointer? Though it is a shame to limit it to only systems with PAC, it is part of what we're testing for so I think it should be included.
(I looked at the nop space instructions but they're only for signing instruction pointers)

I did this in the follow up change https://reviews.llvm.org/D103626#change-luoDAG6B3rHK

omjavaid updated this revision to Diff 355735.Jun 30 2021, 5:43 PM

This update makes native register context changes restricted to Linux/Arm64. Also tweeks test case a little bit.

PS: This test may fail on QEMU. There is QEMU bug that reports wrong watchpoint hit address in case top byte of watched address is tagged.

For TBI, this looks good as is, I would like to see PAC tested as well.

omjavaid updated this revision to Diff 357101.Jul 7 2021, 4:18 PM
omjavaid edited the summary of this revision. (Show Details)

This adds pointer signing before we set watchpoint on the tagged_ptr. LLDB should be able to successfully set a watchpoint on a signed pointer. We can hit the watchpoint on tagged + signed pointer using variable but need to authenticate before increment using tagged pointer.

DavidSpickett added inline comments.Jul 8 2021, 1:28 AM
lldb/test/API/commands/watchpoints/watch_tagged_addr/main.c
12

Add comments to these inline asm lines to explain the instructions.

DavidSpickett accepted this revision.Jul 8 2021, 1:28 AM

LGTM with the comments added.

This revision is now accepted and ready to land.Jul 8 2021, 1:28 AM
This revision was landed with ongoing or failed builds.Jul 11 2021, 7:39 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 11 2021, 7:39 PM
andrew added a subscriber: andrew.Aug 10 2021, 4:16 AM

FreeBSD doesn't currently support TBI. I'm trying to decide if it will be enabled everywhere, or just when needed (e.g. for HWASAN) due to how it interacts with PAC.

I have a patch in review in the FreeBSD phabricator to add PAC support.