Re: [PATCH] AArch64 pauth: Indicate unmasked addresses in backtrace

Pedro Alves
2019-07-17T14:44:04Z

On 7/17/19 2:35 PM, Alan Hayward wrote:

>
>> Once they learn something, often being concise
>> helps -- or in other words, once you learn what "<unmasked>" or "U" or whatever
>> is, and you're used to it, what would you rather see? What's the main
>> information you're looking for when staring at the backtrace? Thoughts
>> like that should guide the output too, IMO.
>
> PAC is the official abbreviation for the feature, so maybe :PAC works best.

Reading https://lwn.net/Articles/767695/ , I see mention of

Insert a PAC into a pointer
Strip a PAC from a pointer

Would ":PAC" make it read like the address _includes_ PAC bits?

>
> (gdb) bt
> 0 0x0000000000400490 in puts@plt ()
> 1 0x00000000004005dc in foo ("hello") at cbreak-lib.c:6
> 2 0x0000000000400604:PAC in bar () at cbreak-lib.c:12
> 3 0x0000000000400620:PAC in barbar () at cbreak.c:17
> 4 0x00000000004005b4 in main () at cbreak-3.c:10
>
>
> Some of my attempts at different representations:
> 2 0x0000000000400604* in bar () at cbreak-lib.c:12
> 2 0x0000000000400604! in bar () at cbreak-lib.c:12
> 2 0x0000000000400604U in bar () at cbreak-lib.c:122
> 2 0x0000000000400604:U in bar () at cbreak-lib.c:122
> 2 0x0000000000400604 in bar () at cbreak-lib.c:12
> 2 0x0000000000400604[U] in bar () at cbreak-lib.c:12
> 2 0x0000000000400604<M> in bar () at cbreak-lib.c:12
> 2 0x0000000000400604 in bar () at cbreak-lib.c:12
> 2 0x0000000000400604<PAC> in bar () at cbreak-lib.c:12
> 2 0x0000000000400604PAC in bar () at cbreak-lib.c:12
> 2 0x0000000000400604:PAC in bar () at cbreak-lib.c:12
> 2 0x0000000000400604,PAC in bar () at cbreak-lib.c:12
>
> I found a single character was too hidden. A single character or symbol was also
> a little confusing - my brain read U as unsigned, * as pointer, [] as an array.
>
> I also like ,PAC as it might be easier to add future extensions.

I'd go with either:

2 0x0000000000400604 (PAC) in bar () at cbreak-lib.c:12
2 0x0000000000400604 [PAC] in bar () at cbreak-lib.c:12

Not having the space may make it a little bit harder
to focus on low digits of the address.

> my brain read U as unsigned, * as pointer, [] as an array.

If you make it like 0x0000000000400604U, then I can see that.

But not so much with:

2 0x0000000000400604 [U] in bar () at cbreak-lib.c:12

You don't have to use a single letter, though:

2 0x0000000000400604 [UN] in bar () at cbreak-lib.c:12

[] seems natural as a way to group some flags/properties to me.

We already use it here for example:

(top-gdb) info registers $eflags
eflags 0x206 [ PF IF ]

I guess I'm saying that it depends on context, and I wouldn't
be worried with [] being confused with C arrays. Afterall,
< and > also have meaning in C/C++... More than one meaning,
actually. :-)

>>> /* If address signing is enabled, mask off the signature bits from ADDR, using
>>> - the register values in THIS_FRAME. */
>>> + the register values in THIS_FRAME. IS_LR indicates if ADDR is from the link
>>> + register for the current frame. */
>>>
>>> static CORE_ADDR
>>> aarch64_frame_unmask_address (struct gdbarch_tdep *tdep,
>>> - struct frame_info *this_frame,
>>> - CORE_ADDR addr)
>>> + struct frame_info *this_frame, CORE_ADDR addr,
>>> + bool is_lr = true)
>>
>> I didn't see anywhere in the patch that passes is_lr == true?
>>
>> Looks like the patch is a nop?
>
> Yes, the is_lr check will always be true.
> The function is designed to be for any address, but right now it’s only used for
> the link register. But, I didn’t want someone in the future using it as a generic
> address unmask function and it causing the link register to be marked.
>
> An other option was to add a warning to the comment block above the function. Or
> do the lr marking in the caller (but then that adds extra logic).
>
> Maybe the better option would be to rename the function to
> aarch64_frame_unmask_lr_address.

Yes, I think that renaming is the best option. People remove
unnecessary parameters all the time, when they notice some
parameter isn't really necessary. If someone needs a more
generic aarch64_frame_unmask_address for all kinds of
addresses, they can always factor out a new aarch64_frame_unmask_address
that looks like today's version, and
make aarch64_frame_unmask_lr_address call it + mark the frame.

Thanks,
Pedro Alves