diff mbox series

[11/12] aarch64: redefine RETURN_ADDRESS to strip PAC

Message ID 20200430174518.GG29015@arm.com
State Superseded
Headers
Series aarch64: branch protection support |

Commit Message

Szabolcs Nagy April 30, 2020, 5:45 p.m. UTC

Comments

Adhemerval Zanella May 8, 2020, 5:44 p.m. UTC | #1 
On 30/04/2020 14:45, Szabolcs Nagy wrote:
> RETURN_ADDRESS is used at several places in glibc to mean a valid
> code address of the call site, but with pac-ret that includes a
> pointer authentication code, so the definition is adjusted.
> 
> XPAC is added unconditionally for now, but it's only needed if
> glibc is compiled with -mbranch-protection=pac-ret. Inline asm
> is used instead of __builtin_aarch64_xpaclri since that's an
> undocumented builtin and not available in all supported gccs.
> ---
>  sysdeps/aarch64/sysdep.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/sysdeps/aarch64/sysdep.h b/sysdeps/aarch64/sysdep.h
> index 63a04a70cd..87f19b9bef 100644
> --- a/sysdeps/aarch64/sysdep.h
> +++ b/sysdeps/aarch64/sysdep.h
> @@ -35,6 +35,16 @@
>  
>  #define PTR_SIZE	(1<<PTR_LOG_SIZE)
>  
> +/* Strip pointer authentication code from pointer p.  */
> +#define XPAC(p) ({					\
> +  register void *__ra asm ("x30") = (p);		\
> +  asm ("hint 7 // xpaclri" : "+r"(__ra));		\
> +  __ra;})
> +
> +/* This is needed when glibc is built with -mbranch-protection=pac-ret.  */
> +#undef RETURN_ADDRESS
> +#define RETURN_ADDRESS(n) XPAC(__builtin_return_address(n))
> +

Maybe use a inline function instead?

  #ifndef __ASSEMBLER__
  # include <sys/cdefs.h>
  /* Strip pointer authentication code from pointer p.  */
  static __always_inline void *
  return_address (unsigned int n)
  { 
    register void *ra asm ("x30") = __builtin_return_address (n);
    asm ("hint 7 // xpaclri" : "+r" (ra));
    return ra;
  }

  /* This is needed when glibc is built with -mbranch-protection=pac-ret.  */
  # undef RETURN_ADDRESS
  # define RETURN_ADDRESS(n) return_address (n)
  #endif


>  #ifdef	__ASSEMBLER__
>  
>  /* Syntactic details of assembler.  */
Szabolcs Nagy May 11, 2020, 12:38 p.m. UTC | #2 
The 05/08/2020 14:44, Adhemerval Zanella via Libc-alpha wrote:
> On 30/04/2020 14:45, Szabolcs Nagy wrote:
> > +++ b/sysdeps/aarch64/sysdep.h
> > @@ -35,6 +35,16 @@
> >  
> >  #define PTR_SIZE	(1<<PTR_LOG_SIZE)
> >  
> > +/* Strip pointer authentication code from pointer p.  */
> > +#define XPAC(p) ({					\
> > +  register void *__ra asm ("x30") = (p);		\
> > +  asm ("hint 7 // xpaclri" : "+r"(__ra));		\
> > +  __ra;})
> > +
> > +/* This is needed when glibc is built with -mbranch-protection=pac-ret.  */
> > +#undef RETURN_ADDRESS
> > +#define RETURN_ADDRESS(n) XPAC(__builtin_return_address(n))
> > +
> 
> Maybe use a inline function instead?

macro seems more reliable to me than always_inline
when poking at __builtin_return_address and x30,
but i'm not against always_inline if that's
considered better.

i'd prefer separate xpac (since it can be used
not just with __builtin_return_address e.g. for
stored code address in jmpbuf, which currently
uses ptrmangling)

>   #ifndef __ASSEMBLER__
>   # include <sys/cdefs.h>

what is cdefs.h for?

>   /* Strip pointer authentication code from pointer p.  */
>   static __always_inline void *
>   return_address (unsigned int n)
>   { 
>     register void *ra asm ("x30") = __builtin_return_address (n);
>     asm ("hint 7 // xpaclri" : "+r" (ra));
>     return ra;
>   }
> 
>   /* This is needed when glibc is built with -mbranch-protection=pac-ret.  */
>   # undef RETURN_ADDRESS
>   # define RETURN_ADDRESS(n) return_address (n)
>   #endif
Adhemerval Zanella May 11, 2020, 7:15 p.m. UTC | #3 
On 11/05/2020 09:38, Szabolcs Nagy wrote:
> The 05/08/2020 14:44, Adhemerval Zanella via Libc-alpha wrote:
>> On 30/04/2020 14:45, Szabolcs Nagy wrote:
>>> +++ b/sysdeps/aarch64/sysdep.h
>>> @@ -35,6 +35,16 @@
>>>  
>>>  #define PTR_SIZE	(1<<PTR_LOG_SIZE)
>>>  
>>> +/* Strip pointer authentication code from pointer p.  */
>>> +#define XPAC(p) ({					\
>>> +  register void *__ra asm ("x30") = (p);		\
>>> +  asm ("hint 7 // xpaclri" : "+r"(__ra));		\
>>> +  __ra;})
>>> +
>>> +/* This is needed when glibc is built with -mbranch-protection=pac-ret.  */
>>> +#undef RETURN_ADDRESS
>>> +#define RETURN_ADDRESS(n) XPAC(__builtin_return_address(n))
>>> +
>>
>> Maybe use a inline function instead?
> 
> macro seems more reliable to me than always_inline
> when poking at __builtin_return_address and x30,
> but i'm not against always_inline if that's
> considered better.

I would prefer a static inline unless a macro is really required
(either due some compiler limitation or bug).

> 
> i'd prefer separate xpac (since it can be used
> not just with __builtin_return_address e.g. for
> stored code address in jmpbuf, which currently
> uses ptrmangling)

Ack.

> 
>>   #ifndef __ASSEMBLER__
>>   # include <sys/cdefs.h>
> 
> what is cdefs.h for?

The __always_inline macro.

> 
>>   /* Strip pointer authentication code from pointer p.  */
>>   static __always_inline void *
>>   return_address (unsigned int n)
>>   { 
>>     register void *ra asm ("x30") = __builtin_return_address (n);
>>     asm ("hint 7 // xpaclri" : "+r" (ra));
>>     return ra;
>>   }
>>
>>   /* This is needed when glibc is built with -mbranch-protection=pac-ret.  */
>>   # undef RETURN_ADDRESS
>>   # define RETURN_ADDRESS(n) return_address (n)
>>   #endif
Florian Weimer May 11, 2020, 7:21 p.m. UTC | #4 
* Adhemerval Zanella via Libc-alpha:

> On 11/05/2020 09:38, Szabolcs Nagy wrote:
>> The 05/08/2020 14:44, Adhemerval Zanella via Libc-alpha wrote:
>>> On 30/04/2020 14:45, Szabolcs Nagy wrote:
>>>> +++ b/sysdeps/aarch64/sysdep.h
>>>> @@ -35,6 +35,16 @@
>>>>  
>>>>  #define PTR_SIZE	(1<<PTR_LOG_SIZE)
>>>>  
>>>> +/* Strip pointer authentication code from pointer p.  */
>>>> +#define XPAC(p) ({					\
>>>> +  register void *__ra asm ("x30") = (p);		\
>>>> +  asm ("hint 7 // xpaclri" : "+r"(__ra));		\
>>>> +  __ra;})
>>>> +
>>>> +/* This is needed when glibc is built with -mbranch-protection=pac-ret.  */
>>>> +#undef RETURN_ADDRESS
>>>> +#define RETURN_ADDRESS(n) XPAC(__builtin_return_address(n))
>>>> +
>>>
>>> Maybe use a inline function instead?
>> 
>> macro seems more reliable to me than always_inline
>> when poking at __builtin_return_address and x30,
>> but i'm not against always_inline if that's
>> considered better.
>
> I would prefer a static inline unless a macro is really required
> (either due some compiler limitation or bug).

I think __builtin_return_address is ill-defined: Does the frame count
that vanishes due to inlining?

So it's probably a case similar to alloca, where a macro has to be
used.
Florian Weimer May 11, 2020, 7:22 p.m. UTC | #5 
* Szabolcs Nagy:

> +/* This is needed when glibc is built with -mbranch-protection=pac-ret.  */
> +#undef RETURN_ADDRESS
> +#define RETURN_ADDRESS(n) XPAC(__builtin_return_address(n))

This looks suspicious.  Is __builtin_return_address ever useful
without the decoding?  If not, why doesn't GCC emit the PAC removal
itself?
Adhemerval Zanella May 11, 2020, 8:13 p.m. UTC | #6 
On 11/05/2020 16:21, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> On 11/05/2020 09:38, Szabolcs Nagy wrote:
>>> The 05/08/2020 14:44, Adhemerval Zanella via Libc-alpha wrote:
>>>> On 30/04/2020 14:45, Szabolcs Nagy wrote:
>>>>> +++ b/sysdeps/aarch64/sysdep.h
>>>>> @@ -35,6 +35,16 @@
>>>>>  
>>>>>  #define PTR_SIZE	(1<<PTR_LOG_SIZE)
>>>>>  
>>>>> +/* Strip pointer authentication code from pointer p.  */
>>>>> +#define XPAC(p) ({					\
>>>>> +  register void *__ra asm ("x30") = (p);		\
>>>>> +  asm ("hint 7 // xpaclri" : "+r"(__ra));		\
>>>>> +  __ra;})
>>>>> +
>>>>> +/* This is needed when glibc is built with -mbranch-protection=pac-ret.  */
>>>>> +#undef RETURN_ADDRESS
>>>>> +#define RETURN_ADDRESS(n) XPAC(__builtin_return_address(n))
>>>>> +
>>>>
>>>> Maybe use a inline function instead?
>>>
>>> macro seems more reliable to me than always_inline
>>> when poking at __builtin_return_address and x30,
>>> but i'm not against always_inline if that's
>>> considered better.
>>
>> I would prefer a static inline unless a macro is really required
>> (either due some compiler limitation or bug).
> 
> I think __builtin_return_address is ill-defined: Does the frame count
> that vanishes due to inlining?
> 
> So it's probably a case similar to alloca, where a macro has to be
> used.

This is at least what documentation states [1]:

"When inlining the expected behavior is that the function returns the address 
of the function that is returned to. To work around this behavior use the 
noinline function attribute."

[1] https://gcc.gnu.org/onlinedocs/gcc/Return-Address.html
Florian Weimer May 11, 2020, 8:18 p.m. UTC | #7 
* Adhemerval Zanella:

> On 11/05/2020 16:21, Florian Weimer wrote:
>> * Adhemerval Zanella via Libc-alpha:
>> 
>>> On 11/05/2020 09:38, Szabolcs Nagy wrote:
>>>> The 05/08/2020 14:44, Adhemerval Zanella via Libc-alpha wrote:
>>>>> On 30/04/2020 14:45, Szabolcs Nagy wrote:
>>>>>> +++ b/sysdeps/aarch64/sysdep.h
>>>>>> @@ -35,6 +35,16 @@
>>>>>>  
>>>>>>  #define PTR_SIZE	(1<<PTR_LOG_SIZE)
>>>>>>  
>>>>>> +/* Strip pointer authentication code from pointer p.  */
>>>>>> +#define XPAC(p) ({					\
>>>>>> +  register void *__ra asm ("x30") = (p);		\
>>>>>> +  asm ("hint 7 // xpaclri" : "+r"(__ra));		\
>>>>>> +  __ra;})
>>>>>> +
>>>>>> +/* This is needed when glibc is built with -mbranch-protection=pac-ret.  */
>>>>>> +#undef RETURN_ADDRESS
>>>>>> +#define RETURN_ADDRESS(n) XPAC(__builtin_return_address(n))
>>>>>> +
>>>>>
>>>>> Maybe use a inline function instead?
>>>>
>>>> macro seems more reliable to me than always_inline
>>>> when poking at __builtin_return_address and x30,
>>>> but i'm not against always_inline if that's
>>>> considered better.
>>>
>>> I would prefer a static inline unless a macro is really required
>>> (either due some compiler limitation or bug).
>> 
>> I think __builtin_return_address is ill-defined: Does the frame count
>> that vanishes due to inlining?
>> 
>> So it's probably a case similar to alloca, where a macro has to be
>> used.
>
> This is at least what documentation states [1]:
>
> "When inlining the expected behavior is that the function returns the address 
> of the function that is returned to. To work around this behavior use the 
> noinline function attribute."
>
> [1] https://gcc.gnu.org/onlinedocs/gcc/Return-Address.html

Hmm, okay.  It's still weird not to count those frames, but at least
it's documented.
Adhemerval Zanella May 11, 2020, 8:45 p.m. UTC | #8 
On 11/05/2020 16:22, Florian Weimer wrote:
> * Szabolcs Nagy:
> 
>> +/* This is needed when glibc is built with -mbranch-protection=pac-ret.  */
>> +#undef RETURN_ADDRESS
>> +#define RETURN_ADDRESS(n) XPAC(__builtin_return_address(n))
> 
> This looks suspicious.  Is __builtin_return_address ever useful
> without the decoding?  If not, why doesn't GCC emit the PAC removal
> itself?
> 

Kernel seems to be working on same assumptions [1], and I would say
the builtin works with the assumption it should return the return
address as is in current frame state. Maybe extend gcc should
extern __builtin_extract_return_addr to remove the PAC bits?


[1] https://patchwork.kernel.org/patch/11195099/
Szabolcs Nagy May 12, 2020, 8:42 a.m. UTC | #9 
The 05/11/2020 21:22, Florian Weimer wrote:
> * Szabolcs Nagy:
> 
> > +/* This is needed when glibc is built with -mbranch-protection=pac-ret.  */
> > +#undef RETURN_ADDRESS
> > +#define RETURN_ADDRESS(n) XPAC(__builtin_return_address(n))
> 
> This looks suspicious.  Is __builtin_return_address ever useful
> without the decoding?  If not, why doesn't GCC emit the PAC removal
> itself?

the only user that needs it is libgcc because it
has dwarf info about the stack pointer and pac
state so it can authenticate the return address,
but normally that's not available (and code that
cares about this would need pac-specific changes).

i consider this a major bug in the current pac-ret
implementation that makes it unusable in existing
software (it's unreasonable to recommend XPAC for
__builtin_return_address users, they will have to
disable pac-ret, but there is no easy way to test
for pac-ret or to disable it without disabling other
things), but not everybody agrees with me on this.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94891
diff mbox series

Patch

From 2223e5ed1d78634ef59f0a7efbd3a9885a8da53f Mon Sep 17 00:00:00 2001
From: Szabolcs Nagy <szabolcs.nagy@arm.com>
Date: Wed, 15 Apr 2020 17:40:45 +0100
Subject: [PATCH 11/12] aarch64: redefine RETURN_ADDRESS to strip PAC

RETURN_ADDRESS is used at several places in glibc to mean a valid
code address of the call site, but with pac-ret that includes a
pointer authentication code, so the definition is adjusted.

XPAC is added unconditionally for now, but it's only needed if
glibc is compiled with -mbranch-protection=pac-ret. Inline asm
is used instead of __builtin_aarch64_xpaclri since that's an
undocumented builtin and not available in all supported gccs.
---
 sysdeps/aarch64/sysdep.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/sysdeps/aarch64/sysdep.h b/sysdeps/aarch64/sysdep.h
index 63a04a70cd..87f19b9bef 100644
--- a/sysdeps/aarch64/sysdep.h
+++ b/sysdeps/aarch64/sysdep.h
@@ -35,6 +35,16 @@ 
 
 #define PTR_SIZE	(1<<PTR_LOG_SIZE)
 
+/* Strip pointer authentication code from pointer p.  */
+#define XPAC(p) ({					\
+  register void *__ra asm ("x30") = (p);		\
+  asm ("hint 7 // xpaclri" : "+r"(__ra));		\
+  __ra;})
+
+/* This is needed when glibc is built with -mbranch-protection=pac-ret.  */
+#undef RETURN_ADDRESS
+#define RETURN_ADDRESS(n) XPAC(__builtin_return_address(n))
+
 #ifdef	__ASSEMBLER__
 
 /* Syntactic details of assembler.  */
-- 
2.17.1