Message ID | 20200430174518.GG29015@arm.com |
---|---|
State | New |
Headers | show |
Series | aarch64: branch protection support | expand |
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. */
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
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
* 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.
* 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?
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
* 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.
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/
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
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