[v3,13/13] aarch64: fix _mcount for pac-ret

Message ID 3d422103bcd42547829b53fa07d3891e5bfabba3.1589552055.git.szabolcs.nagy@arm.com
State New
Headers show
Series
  • aarch64: branch protection support
Related show

Commit Message

Szabolcs Nagy May 15, 2020, 2:40 p.m.
gcc -pg with -mbranch-protection=pac-ret passes signed return address
to _mcount, so _mcount now has to always strip pac from the frompc
since that's from user code that may be built with pac-ret.

This is a backward incompatible _mcount abi change introduced by
return address signing support in gcc-7.

TODO: fix -pg on the gcc side?
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94791
---
 sysdeps/aarch64/machine-gmon.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.17.1

Comments

H.J. Lu via Libc-alpha May 26, 2020, 11:33 a.m. | #1
On 15/05/2020 11:40, Szabolcs Nagy wrote:
> gcc -pg with -mbranch-protection=pac-ret passes signed return address

> to _mcount, so _mcount now has to always strip pac from the frompc

> since that's from user code that may be built with pac-ret.

> 

> This is a backward incompatible _mcount abi change introduced by

> return address signing support in gcc-7.


Which change are you referring about specifically?

Besides it, LGTM.

Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>


> 

> TODO: fix -pg on the gcc side?

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94791

> ---

>  sysdeps/aarch64/machine-gmon.h | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/sysdeps/aarch64/machine-gmon.h b/sysdeps/aarch64/machine-gmon.h

> index 730a23b781..328cbdda16 100644

> --- a/sysdeps/aarch64/machine-gmon.h

> +++ b/sysdeps/aarch64/machine-gmon.h

> @@ -30,5 +30,5 @@ static inline void mcount_internal (u_long frompc, u_long selfpc)

>  #define MCOUNT                                                    \

>  void __mcount (void *frompc)                                      \

>  {                                                                 \

> -  mcount_internal ((u_long) frompc, (u_long) RETURN_ADDRESS (0)); \

> +  mcount_internal ((u_long) strip_pac (frompc), (u_long) RETURN_ADDRESS (0)); \

>  }

>
Szabolcs Nagy May 26, 2020, 6:38 p.m. | #2
The 05/26/2020 08:33, Adhemerval Zanella via Libc-alpha wrote:
> 

> 

> On 15/05/2020 11:40, Szabolcs Nagy wrote:

> > gcc -pg with -mbranch-protection=pac-ret passes signed return address

> > to _mcount, so _mcount now has to always strip pac from the frompc

> > since that's from user code that may be built with pac-ret.

> > 

> > This is a backward incompatible _mcount abi change introduced by

> > return address signing support in gcc-7.

> 

> Which change are you referring about specifically?


gcc-7 introduced -msigned-return-address (which was
later deprecated by -mbranch-protection=pac-ret) and
the code generation with -pg is not compatible with
existing _mcount runtime implementations (because
now _mcount needs unconditional xpac)

i'm still trying to see if this _mcount change is
needed or we can change gcc to do the right thing
(and not pass signed address to _mcount), but i
need internal consensus about such abi break first.

> 

> Besides it, LGTM.

> 

> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>

> 

> > 

> > TODO: fix -pg on the gcc side?

> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94791

> > ---

> >  sysdeps/aarch64/machine-gmon.h | 2 +-

> >  1 file changed, 1 insertion(+), 1 deletion(-)

> > 

> > diff --git a/sysdeps/aarch64/machine-gmon.h b/sysdeps/aarch64/machine-gmon.h

> > index 730a23b781..328cbdda16 100644

> > --- a/sysdeps/aarch64/machine-gmon.h

> > +++ b/sysdeps/aarch64/machine-gmon.h

> > @@ -30,5 +30,5 @@ static inline void mcount_internal (u_long frompc, u_long selfpc)

> >  #define MCOUNT                                                    \

> >  void __mcount (void *frompc)                                      \

> >  {                                                                 \

> > -  mcount_internal ((u_long) frompc, (u_long) RETURN_ADDRESS (0)); \

> > +  mcount_internal ((u_long) strip_pac (frompc), (u_long) RETURN_ADDRESS (0)); \

> >  }

> > 


--
H.J. Lu via Libc-alpha May 26, 2020, 7:13 p.m. | #3
On 26/05/2020 15:38, Szabolcs Nagy wrote:
> The 05/26/2020 08:33, Adhemerval Zanella via Libc-alpha wrote:

>>

>>

>> On 15/05/2020 11:40, Szabolcs Nagy wrote:

>>> gcc -pg with -mbranch-protection=pac-ret passes signed return address

>>> to _mcount, so _mcount now has to always strip pac from the frompc

>>> since that's from user code that may be built with pac-ret.

>>>

>>> This is a backward incompatible _mcount abi change introduced by

>>> return address signing support in gcc-7.

>>

>> Which change are you referring about specifically?

> 

> gcc-7 introduced -msigned-return-address (which was

> later deprecated by -mbranch-protection=pac-ret) and

> the code generation with -pg is not compatible with

> existing _mcount runtime implementations (because

> now _mcount needs unconditional xpac)


Thanks, could you add this explanation on commit message as
well?

> 

> i'm still trying to see if this _mcount change is

> needed or we can change gcc to do the right thing

> (and not pass signed address to _mcount), but i

> need internal consensus about such abi break first.

> 

>>

>> Besides it, LGTM.

>>

>> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>

>>

>>>

>>> TODO: fix -pg on the gcc side?

>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94791

>>> ---

>>>  sysdeps/aarch64/machine-gmon.h | 2 +-

>>>  1 file changed, 1 insertion(+), 1 deletion(-)

>>>

>>> diff --git a/sysdeps/aarch64/machine-gmon.h b/sysdeps/aarch64/machine-gmon.h

>>> index 730a23b781..328cbdda16 100644

>>> --- a/sysdeps/aarch64/machine-gmon.h

>>> +++ b/sysdeps/aarch64/machine-gmon.h

>>> @@ -30,5 +30,5 @@ static inline void mcount_internal (u_long frompc, u_long selfpc)

>>>  #define MCOUNT                                                    \

>>>  void __mcount (void *frompc)                                      \

>>>  {                                                                 \

>>> -  mcount_internal ((u_long) frompc, (u_long) RETURN_ADDRESS (0)); \

>>> +  mcount_internal ((u_long) strip_pac (frompc), (u_long) RETURN_ADDRESS (0)); \

>>>  }

>>>

>

Patch

diff --git a/sysdeps/aarch64/machine-gmon.h b/sysdeps/aarch64/machine-gmon.h
index 730a23b781..328cbdda16 100644
--- a/sysdeps/aarch64/machine-gmon.h
+++ b/sysdeps/aarch64/machine-gmon.h
@@ -30,5 +30,5 @@  static inline void mcount_internal (u_long frompc, u_long selfpc)
 #define MCOUNT                                                    \
 void __mcount (void *frompc)                                      \
 {                                                                 \
-  mcount_internal ((u_long) frompc, (u_long) RETURN_ADDRESS (0)); \
+  mcount_internal ((u_long) strip_pac (frompc), (u_long) RETURN_ADDRESS (0)); \
 }