diff mbox series

[06/12] aarch64: fix RTLD_START for BTI

Message ID 20200430174204.GB29015@arm.com
State New
Headers show
Series aarch64: branch protection support | expand

Commit Message

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

Comments

Adhemerval Zanella May 7, 2020, 6:49 p.m. UTC | #1
On 30/04/2020 14:42, Szabolcs Nagy wrote:
> From 1e8662264c07e69d807761882e8d77f0916ae562 Mon Sep 17 00:00:00 2001
> From: Szabolcs Nagy <szabolcs.nagy@arm.com>
> Date: Tue, 31 Mar 2020 17:32:14 +0100
> Subject: [PATCH 06/12] aarch64: fix RTLD_START for BTI
> 
> Tailcalls must use x16 or x17 for the indirect branch instruction
> to be compatible with code that uses BTI c at function entries.
> (Other forms of indirect branches can only land on BTI j.)
> 
> Also added a BTI c at the ELF entry point of rtld, this is not
> strictly necessary since the kernel does not use indirect branch
> to get there, but it seems safest once building glibc itself with
> BTI is supported.

LGTM, thanks.

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

> ---
>  sysdeps/aarch64/dl-machine.h | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/sysdeps/aarch64/dl-machine.h b/sysdeps/aarch64/dl-machine.h
> index db3335e5ad..70b9ed3925 100644
> --- a/sysdeps/aarch64/dl-machine.h
> +++ b/sysdeps/aarch64/dl-machine.h
> @@ -125,6 +125,8 @@ elf_machine_runtime_setup (struct link_map *l, int lazy, int profile)
>  .globl _dl_start_user							\n\
>  .type _dl_start_user, %function						\n\
>  _start:									\n\
> +	// bti c							\n\
> +	hint	34							\n\

This is the BTI_C defined at sysdeps/aarch64/sysdep.h, why can't you use
it here?

>  	mov	" PTR "0, " PTR_SP "					\n\
>  	bl	_dl_start						\n\
>  	// returns user entry point in x0				\n\
> @@ -178,7 +180,8 @@ _dl_start_user:								\n\
>  	adrp	x0, _dl_fini						\n\
>  	add	" PTR "0, " PTR "0, #:lo12:_dl_fini			\n\
>  	// jump to the user_s entry point				\n\
> -	br      x21							\n\
> +	mov     x16, x21						\n\
> +	br      x16							\n\
>  ");
>  
>  #define elf_machine_type_class(type)					\
> -- 
> 2.17.1
Szabolcs Nagy May 7, 2020, 7:24 p.m. UTC | #2
The 05/07/2020 15:49, Adhemerval Zanella wrote:
> On 30/04/2020 14:42, Szabolcs Nagy wrote:
> > From 1e8662264c07e69d807761882e8d77f0916ae562 Mon Sep 17 00:00:00 2001
> > From: Szabolcs Nagy <szabolcs.nagy@arm.com>
> > Date: Tue, 31 Mar 2020 17:32:14 +0100
> > Subject: [PATCH 06/12] aarch64: fix RTLD_START for BTI
> > 
> > Tailcalls must use x16 or x17 for the indirect branch instruction
> > to be compatible with code that uses BTI c at function entries.
> > (Other forms of indirect branches can only land on BTI j.)
> > 
> > Also added a BTI c at the ELF entry point of rtld, this is not
> > strictly necessary since the kernel does not use indirect branch
> > to get there, but it seems safest once building glibc itself with
> > BTI is supported.
> 
> LGTM, thanks.
> 
> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
> 
> > ---
> >  sysdeps/aarch64/dl-machine.h | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/sysdeps/aarch64/dl-machine.h b/sysdeps/aarch64/dl-machine.h
> > index db3335e5ad..70b9ed3925 100644
> > --- a/sysdeps/aarch64/dl-machine.h
> > +++ b/sysdeps/aarch64/dl-machine.h
> > @@ -125,6 +125,8 @@ elf_machine_runtime_setup (struct link_map *l, int lazy, int profile)
> >  .globl _dl_start_user							\n\
> >  .type _dl_start_user, %function						\n\
> >  _start:									\n\
> > +	// bti c							\n\
> > +	hint	34							\n\
> 
> This is the BTI_C defined at sysdeps/aarch64/sysdep.h, why can't you use
> it here?

BTI_C is only defined for asm files, but this is inline asm in c.

thanks for the reviews so far, i'm working on a v2 of the patchset
(changing configury bits and how all this is enabled) will post it
when the tests finish running.
Adhemerval Zanella May 7, 2020, 7:55 p.m. UTC | #3
On 07/05/2020 16:24, Szabolcs Nagy wrote:
> The 05/07/2020 15:49, Adhemerval Zanella wrote:
>> On 30/04/2020 14:42, Szabolcs Nagy wrote:
>>> From 1e8662264c07e69d807761882e8d77f0916ae562 Mon Sep 17 00:00:00 2001
>>> From: Szabolcs Nagy <szabolcs.nagy@arm.com>
>>> Date: Tue, 31 Mar 2020 17:32:14 +0100
>>> Subject: [PATCH 06/12] aarch64: fix RTLD_START for BTI
>>>
>>> Tailcalls must use x16 or x17 for the indirect branch instruction
>>> to be compatible with code that uses BTI c at function entries.
>>> (Other forms of indirect branches can only land on BTI j.)
>>>
>>> Also added a BTI c at the ELF entry point of rtld, this is not
>>> strictly necessary since the kernel does not use indirect branch
>>> to get there, but it seems safest once building glibc itself with
>>> BTI is supported.
>>
>> LGTM, thanks.
>>
>> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
>>
>>> ---
>>>  sysdeps/aarch64/dl-machine.h | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/sysdeps/aarch64/dl-machine.h b/sysdeps/aarch64/dl-machine.h
>>> index db3335e5ad..70b9ed3925 100644
>>> --- a/sysdeps/aarch64/dl-machine.h
>>> +++ b/sysdeps/aarch64/dl-machine.h
>>> @@ -125,6 +125,8 @@ elf_machine_runtime_setup (struct link_map *l, int lazy, int profile)
>>>  .globl _dl_start_user							\n\
>>>  .type _dl_start_user, %function						\n\
>>>  _start:									\n\
>>> +	// bti c							\n\
>>> +	hint	34							\n\
>>
>> This is the BTI_C defined at sysdeps/aarch64/sysdep.h, why can't you use
>> it here?
> 
> BTI_C is only defined for asm files, but this is inline asm in c.

Ack.

> 
> thanks for the reviews so far, i'm working on a v2 of the patchset
> (changing configury bits and how all this is enabled) will post it
> when the tests finish running.
> 

Could you send the new version as inline instead of attachment? It is
default of git send-email and slight easier to review with most email
clients.
Szabolcs Nagy May 7, 2020, 8:14 p.m. UTC | #4
The 05/07/2020 16:55, Adhemerval Zanella wrote:
> > thanks for the reviews so far, i'm working on a v2 of the patchset
> > (changing configury bits and how all this is enabled) will post it
> > when the tests finish running.
> > 
> 
> Could you send the new version as inline instead of attachment? It is
> default of git send-email and slight easier to review with most email
> clients.

ok.

i sent as attachments because on some patches
i'm not the author which is visible in an
attachment but not with inline patch..?

maybe i should make myself the 'author' and
add the original author as 'co-authored-by'?

not sure what's the best policy here.
Adhemerval Zanella May 7, 2020, 8:20 p.m. UTC | #5
On 07/05/2020 17:14, Szabolcs Nagy wrote:
> The 05/07/2020 16:55, Adhemerval Zanella wrote:
>>> thanks for the reviews so far, i'm working on a v2 of the patchset
>>> (changing configury bits and how all this is enabled) will post it
>>> when the tests finish running.
>>>
>>
>> Could you send the new version as inline instead of attachment? It is
>> default of git send-email and slight easier to review with most email
>> clients.
> 
> ok.
> 
> i sent as attachments because on some patches
> i'm not the author which is visible in an
> attachment but not with inline patch..?
> 
> maybe i should make myself the 'author' and
> add the original author as 'co-authored-by'?
> 
> not sure what's the best policy here.
> 

At least with git send-email, it does handle different author. For
instance https://sourceware.org/pipermail/libc-alpha/2020-April/112686.html
diff mbox series

Patch

From 1e8662264c07e69d807761882e8d77f0916ae562 Mon Sep 17 00:00:00 2001
From: Szabolcs Nagy <szabolcs.nagy@arm.com>
Date: Tue, 31 Mar 2020 17:32:14 +0100
Subject: [PATCH 06/12] aarch64: fix RTLD_START for BTI

Tailcalls must use x16 or x17 for the indirect branch instruction
to be compatible with code that uses BTI c at function entries.
(Other forms of indirect branches can only land on BTI j.)

Also added a BTI c at the ELF entry point of rtld, this is not
strictly necessary since the kernel does not use indirect branch
to get there, but it seems safest once building glibc itself with
BTI is supported.
---
 sysdeps/aarch64/dl-machine.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/sysdeps/aarch64/dl-machine.h b/sysdeps/aarch64/dl-machine.h
index db3335e5ad..70b9ed3925 100644
--- a/sysdeps/aarch64/dl-machine.h
+++ b/sysdeps/aarch64/dl-machine.h
@@ -125,6 +125,8 @@  elf_machine_runtime_setup (struct link_map *l, int lazy, int profile)
 .globl _dl_start_user							\n\
 .type _dl_start_user, %function						\n\
 _start:									\n\
+	// bti c							\n\
+	hint	34							\n\
 	mov	" PTR "0, " PTR_SP "					\n\
 	bl	_dl_start						\n\
 	// returns user entry point in x0				\n\
@@ -178,7 +180,8 @@  _dl_start_user:								\n\
 	adrp	x0, _dl_fini						\n\
 	add	" PTR "0, " PTR "0, #:lo12:_dl_fini			\n\
 	// jump to the user_s entry point				\n\
-	br      x21							\n\
+	mov     x16, x21						\n\
+	br      x16							\n\
 ");
 
 #define elf_machine_type_class(type)					\
-- 
2.17.1