Bug 28947 - GDB does not remove AArch64 pointer signatures before doing memory accesses
Summary: GDB does not remove AArch64 pointer signatures before doing memory accesses
Status: RESOLVED FIXED
Alias: None
Product: gdb
Classification: Unclassified
Component: gdb (show other bugs)
Version: HEAD
: P2 normal
Target Milestone: ---
Assignee: Luis Machado
URL:
Keywords:
Depends on:
Blocks: 29421
  Show dependency treegraph
 
Reported: 2022-03-07 11:32 UTC by David Spickett
Modified: 2022-12-16 11:22 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Last reconfirmed: 2022-03-07 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description David Spickett 2022-03-07 11:32:53 UTC
I'm using a somewhat recent build from source:
```
$ ./gdb --version
GNU gdb (GDB) 12.0.50.20220208-git
```

binutils @ 481153777e278b71e694fd2db6b897f7a9e3dcb8

Compile the following:
```
#include <stddef.h>

int main(int argc, char const *argv[]) {
  char* buf[16];

#define sign_ptr(ptr) \
  __asm__ __volatile__("pacdza %0" \
                       : "=r"(ptr) \
                       : "r"(ptr))

  // Set top byte including where memory tag bits would go.
  char *buf_with_non_address = (char *)((size_t)buf | (size_t)0xff << 56);
  sign_ptr(buf_with_non_address);
  // Address is now:
  // <4 bit user tag><4 bit memory tag><pointer signature><virtual address>

  return 0; // Set break point at this line.
}
```

With:
```
aarch64-unknown-linux-gnu-gcc -march=armv8.3-a main.c -o prog -g
```

Then run it on an AArch64 machine that has pointer authentication enabled. Break at the indicated line where main returns.

Expected behaviour: GDB would know that pointer signatures are "non-address bits" (not part of the virtual address) and remove them before attempting to do a memory access on a signed pointer.

Actual behaviour: GDB does not remove these bits so it attempts to read a completely different memory location. (one cannot be mapped in any case)

```
(gdb) p &buf
$4 = (char *(*)[16]) 0xfffffffff268
(gdb) p buf_with_non_address
$5 = 0xff75fffffffff268 <error: Cannot access memory at address 0xff75fffffffff268>
(gdb) x buf
0xfffffffff268: 0x00000000
(gdb) x buf_with_non_address
0xff75fffffffff268:     Cannot access memory at address 0xff75fffffffff268
```

I've been looking at this issue for lldb and it seems that other non-address bits like the top byte can be left in when giving ptrace a virtual address. Probably because the hardware ignores it for us. (this behaviour isn't guaranteed by the kernel though as far as I know)

With pointer authentication, running code is expected to "auth" the pointer before use. This would remove the signature bits if successful. So a memory read/write with a signature still attached is always going to be a failure of some kind.

GDB or GDB server should remove these signature bits before it gets to ptrace so that the user doesn't have to manually auth these pointers.

For what it's worth, for lldb I'm looking at doing this in lldb not lldb-server. Keeps the server simple and fixes some other things like our memory caching along the way. Same approach might work for GDB as well.
Comment 1 Luis Machado 2022-03-07 11:46:15 UTC
Thanks David. I'll take a look at it.
Comment 2 Luis Machado 2022-03-07 13:28:06 UTC
Doing a quick check, it seems GDB only sanitizes the top byte. If there are non-address bits at the second-to-last byte, it won't clear those.

(gdb) set argv=0xff00fffffffff4d8
(gdb) p *argv
$3 = 0xfffffffff715 "..."
Comment 3 Luis Machado 2022-05-24 08:04:15 UTC
Thinking a bit more about this, I'm not sure if GDB/debuggers should go out of their way to remove signature bits from the pointers.

Accessing memory using a signed pointer is invalid anyway, and will result in a fault. Having debuggers remove that information may cause confusion for a developer that is trying to debug a PAC-related crash of some kind, as it will not show the signature part of the pointer.

I can imagine a scenario where a pointer wasn't signed properly, but GDB will strip the signature of that pointer and will show things as if they were correct, when in fact they are not. Does that make sense?

Accessing memory using a tagged pointer is valid though, but debuggers need to be cautious not to pass tagged pointers down to syscalls. GDB does this.
Comment 4 Luis Machado 2022-05-24 08:06:33 UTC
Does DWARF have operations to sign/unsign arbitrary registers? As far as I remember, it only deals with the return address.
Comment 5 David Spickett 2022-05-24 09:06:07 UTC
TLDR: I agree that outside of literal memory read/write we should be careful about whether the signature is shown to the user.

> Thinking a bit more about this, I'm not sure if GDB/debuggers should go out of their way to remove signature bits from the pointers.

I agree sometimes it doesn't make sense to remove it e.g. printing a symbol's value:

(lldb) p fn_ptr
(char (*)(size_t, int)) $0 = 0x003d0000004006ac (actual=0x00000000004006ac a.out`checked_mmap at main.c:13:48)

lldb has decided to show both if it finds a symbol using the stripped address. So if you weren't expecting it to be signed then you'd hopefully realise here but if you are in some PAC situation it's more convenient (this code was added by Apple who have an ABI that uses it).

> Accessing memory using a signed pointer is invalid anyway, and will result in a fault. Having debuggers remove that information may cause confusion for a developer that is trying to debug a PAC-related crash of some kind, as it will not show the signature part of the pointer.
> I can imagine a scenario where a pointer wasn't signed properly, but GDB will strip the signature of that pointer and will show things as if they were correct, when in fact they are not. Does that make sense?

I think we agree here. If we're printing a symbol or a register or the result of some expression - leave the bits in.

If you're giving that to some command that shows the content of memory - remove the bits and use the virtual address.

We could tell the user that the signature is invalid, maybe by JIT-ing an auth sequence but I think going ahead and reading the memory for them makes sense for now.
(Also we don't know which key was used or 100% whether it's a code or data pointer, even if the user told us what key was used. I might look into this, it could make an interesting add on script even if it is a total hack)

> Accessing memory using a tagged pointer is valid though, but debuggers need to be cautious not to pass tagged pointers down to syscalls. GDB does this.

This is 99% of the situations I'm stripping the address in lldb. Any time you're saying what is the value of this symbol/register, it's left intact. Only if you try to do a memory read with it do we remove the bits. So the example above you see the signature but if you give the symbol name to "memory read" it'll just use the virtual address.

I guess the overall thing here is should the user be restricted during debug - by what is allowed to happen at runtime. Given that debuggers can do things like write to read only pages and read tagged memory with any tag they want, the answer seems to be no. However, where it makes sense we could tell the user that it would not work at runtime.

It's tricky. You could overflow into the upper bits of a pointer via some bad math and because you're on a ptrauth system the debugger removes them and it seems like it all works but fails at runtime. I don't know of a way of still catching that whilst not making it inconvenient to work with signed pointers if you are opting into them.
(or you could be mixing signed and unsigned pointers in the same program)
Comment 6 Luis Machado 2022-05-26 08:23:44 UTC
I see your point about making it a bit more convenient for the user. It's just that we're dealing with best-effort cases here, as we don't know some of the intentions the developer has before going and processing pointers.

It wouldn't be nice to have two distinct behaviors for the debuggers though.

As GDB already removes the (hardcoded) top 8 bits, I'm considering modifying the hook to actually return a stripped pointer as opposed to letting generic code remove X number of top bits.

This way we can make sure we output the proper stripped pointer. GDB had an issue before where it failed to deal with kernel pointers, as it had to sign-extend them while stripping the top bits.

I'll put something together along with a testcase.
Comment 7 Sourceware Commits 2022-12-16 11:19:56 UTC
The master branch has been updated by Luis Machado <luisgpm@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=d88cb738e6a7a7179dfaff8af78d69250c852af1

commit d88cb738e6a7a7179dfaff8af78d69250c852af1
Author: Luis Machado <luis.machado@arm.com>
Date:   Tue May 24 23:31:09 2022 +0100

    [aarch64] Fix removal of non-address bits for PAuth
    
    PR gdb/28947
    
    The address_significant gdbarch setting was introduced as a way to remove
    non-address bits from pointers, and it is specified by a constant.  This
    constant represents the number of address bits in a pointer.
    
    Right now AArch64 is the only architecture that uses it, and 56 was a
    correct option so far.
    
    But if we are using Pointer Authentication (PAuth), we might use up to 2 bytes
    from the address space to store the required information.  We could also have
    cases where we're using both PAuth and MTE.
    
    We could adjust the constant to 48 to cover those cases, but this doesn't
    cover the case where GDB needs to sign-extend kernel addresses after removal
    of the non-address bits.
    
    This has worked so far because bit 55 is used to select between kernel-space
    and user-space addresses.  But trying to clear a range of bits crossing the
    bit 55 boundary requires the hook to be smarter.
    
    The following patch renames the gdbarch hook from significant_addr_bit to
    remove_non_address_bits and passes a pointer as opposed to the number of
    bits.  The hook is now responsible for removing the required non-address bits
    and sign-extending the address if needed.
    
    While at it, make GDB and GDBServer share some more code for aarch64 and add a
    new arch-specific testcase gdb.arch/aarch64-non-address-bits.exp.
    
    Bug-url: https://sourceware.org/bugzilla/show_bug.cgi?id=28947
    
    Approved-By: Simon Marchi <simon.marchi@efficios.com>
Comment 8 Luis Machado 2022-12-16 11:22:13 UTC
Fixed on master.