| Message ID | 20200430174204.GB29015@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | aarch64: branch protection support | expand |
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
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.
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.
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.
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
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