| Message ID | 20210310220519.16811-9-yu-cheng.yu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Control-flow Enforcement: Indirect Branch Tracking | expand |
On Wed, Mar 10, 2021 at 02:05:19PM -0800, Yu-cheng Yu wrote: > When CET is enabled, __vdso_sgx_enter_enclave() needs an endbr64 > in the beginning of the function. OK. What you should do is to explain what it does and why it's needed. > > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> > Cc: Andy Lutomirski <luto@kernel.org> > Cc: Dave Hansen <dave.hansen@linux.intel.com> > Cc: Jarkko Sakkinen <jarkko@kernel.org> > --- > arch/x86/entry/vdso/vsgx.S | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/x86/entry/vdso/vsgx.S b/arch/x86/entry/vdso/vsgx.S > index 86a0e94f68df..a70d4d09f713 100644 > --- a/arch/x86/entry/vdso/vsgx.S > +++ b/arch/x86/entry/vdso/vsgx.S > @@ -27,6 +27,9 @@ > SYM_FUNC_START(__vdso_sgx_enter_enclave) > /* Prolog */ > .cfi_startproc > +#ifdef CONFIG_X86_CET > + endbr64 > +#endif > push %rbp > .cfi_adjust_cfa_offset 8 > .cfi_rel_offset %rbp, 0 > -- > 2.21.0 > > /Jarkko
On 3/10/2021 2:39 PM, Jarkko Sakkinen wrote: > On Wed, Mar 10, 2021 at 02:05:19PM -0800, Yu-cheng Yu wrote: >> When CET is enabled, __vdso_sgx_enter_enclave() needs an endbr64 >> in the beginning of the function. > > OK. > > What you should do is to explain what it does and why it's needed. > The endbr marks a branch target. Without the "no-track" prefix, if an indirect call/jmp reaches a non-endbr opcode, a control-protection fault is raised. Usually endbr's are inserted by the compiler. For assembly, these have to be put in manually. I will add this in the commit log if there is another revision. Thanks! -- Yu-cheng >> >> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> >> Cc: Andy Lutomirski <luto@kernel.org> >> Cc: Dave Hansen <dave.hansen@linux.intel.com> >> Cc: Jarkko Sakkinen <jarkko@kernel.org> >> --- >> arch/x86/entry/vdso/vsgx.S | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/arch/x86/entry/vdso/vsgx.S b/arch/x86/entry/vdso/vsgx.S >> index 86a0e94f68df..a70d4d09f713 100644 >> --- a/arch/x86/entry/vdso/vsgx.S >> +++ b/arch/x86/entry/vdso/vsgx.S >> @@ -27,6 +27,9 @@ >> SYM_FUNC_START(__vdso_sgx_enter_enclave) >> /* Prolog */ >> .cfi_startproc >> +#ifdef CONFIG_X86_CET >> + endbr64 >> +#endif >> push %rbp >> .cfi_adjust_cfa_offset 8 >> .cfi_rel_offset %rbp, 0 >> -- >> 2.21.0 >> >> > > /Jarkko >
On Wed, Mar 10, 2021 at 02:55:55PM -0800, Yu, Yu-cheng wrote: > > > @@ -27,6 +27,9 @@ > > > SYM_FUNC_START(__vdso_sgx_enter_enclave) > > > /* Prolog */ > > > .cfi_startproc > > > +#ifdef CONFIG_X86_CET > > > + endbr64 > > > +#endif You can hide this ifdeffery in a macro and have ENDBR64 at the callsite and define .macro ENDBR64 #ifdef CONFIG_X86_CET endbr64 #endif .endm or so, perhaps. Ditto for endbr32. Thx.
On 3/10/21 2:55 PM, Yu, Yu-cheng wrote: > On 3/10/2021 2:39 PM, Jarkko Sakkinen wrote: >> On Wed, Mar 10, 2021 at 02:05:19PM -0800, Yu-cheng Yu wrote: >>> When CET is enabled, __vdso_sgx_enter_enclave() needs an endbr64 >>> in the beginning of the function. >> >> OK. >> >> What you should do is to explain what it does and why it's needed. >> > > The endbr marks a branch target. Without the "no-track" prefix, if an > indirect call/jmp reaches a non-endbr opcode, a control-protection fault > is raised. Usually endbr's are inserted by the compiler. For assembly, > these have to be put in manually. I will add this in the commit log if > there is another revision. Thanks! This is close, but it's missing a detail or two that I think is important for someone like Jarkko trying to figure out what it means for his subsystem or driver. I'd probably say: ENDBR is a special new instruction for the Indirect Branch Tracking (IBR) component of CET. IBT prevents attacks by ensuring that (most) indirect branches and function calls may only land at ENDBR instructions. Branches that don't follow the rules will result in control flow (#CF) exceptions. ENDBR is a noop when IBT is unsupported or disabled. Most ENDBR instructions are inserted automatically by the compiler, but branch targets written in assembly must have ENDBR added manually, like this one.
On 3/10/2021 3:20 PM, Dave Hansen wrote: > On 3/10/21 2:55 PM, Yu, Yu-cheng wrote: >> On 3/10/2021 2:39 PM, Jarkko Sakkinen wrote: >>> On Wed, Mar 10, 2021 at 02:05:19PM -0800, Yu-cheng Yu wrote: >>>> When CET is enabled, __vdso_sgx_enter_enclave() needs an endbr64 >>>> in the beginning of the function. >>> >>> OK. >>> >>> What you should do is to explain what it does and why it's needed. >>> >> >> The endbr marks a branch target. Without the "no-track" prefix, if an >> indirect call/jmp reaches a non-endbr opcode, a control-protection fault >> is raised. Usually endbr's are inserted by the compiler. For assembly, >> these have to be put in manually. I will add this in the commit log if >> there is another revision. Thanks! > > This is close, but it's missing a detail or two that I think is > important for someone like Jarkko trying to figure out what it means for > his subsystem or driver. > > I'd probably say: > > ENDBR is a special new instruction for the Indirect Branch Tracking > (IBR) component of CET. IBT prevents attacks by ensuring that (most) > indirect branches and function calls may only land at ENDBR > instructions. Branches that don't follow the rules will result in > control flow (#CF) exceptions. > > ENDBR is a noop when IBT is unsupported or disabled. Most ENDBR > instructions are inserted automatically by the compiler, but branch > targets written in assembly must have ENDBR added manually, like this one. > Ok, I will update. Thanks! -- Yu-cheng
On Wed, Mar 10, 2021 at 02:55:55PM -0800, Yu, Yu-cheng wrote: > On 3/10/2021 2:39 PM, Jarkko Sakkinen wrote: > > On Wed, Mar 10, 2021 at 02:05:19PM -0800, Yu-cheng Yu wrote: > > > When CET is enabled, __vdso_sgx_enter_enclave() needs an endbr64 > > > in the beginning of the function. > > > > OK. > > > > What you should do is to explain what it does and why it's needed. > > > > The endbr marks a branch target. Without the "no-track" prefix, if an > indirect call/jmp reaches a non-endbr opcode, a control-protection fault is > raised. Usually endbr's are inserted by the compiler. For assembly, these > have to be put in manually. I will add this in the commit log if there is > another revision. Thanks! Thanks for the explanation. There is another revision, because this is lacking from the commit message. Does it do any harm to put it there unconditionally? > > -- > Yu-cheng > > > > > > > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> > > > Cc: Andy Lutomirski <luto@kernel.org> > > > Cc: Dave Hansen <dave.hansen@linux.intel.com> > > > Cc: Jarkko Sakkinen <jarkko@kernel.org> > > > --- > > > arch/x86/entry/vdso/vsgx.S | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/arch/x86/entry/vdso/vsgx.S b/arch/x86/entry/vdso/vsgx.S > > > index 86a0e94f68df..a70d4d09f713 100644 > > > --- a/arch/x86/entry/vdso/vsgx.S > > > +++ b/arch/x86/entry/vdso/vsgx.S > > > @@ -27,6 +27,9 @@ > > > SYM_FUNC_START(__vdso_sgx_enter_enclave) > > > /* Prolog */ > > > .cfi_startproc > > > +#ifdef CONFIG_X86_CET > > > + endbr64 > > > +#endif > > > push %rbp > > > .cfi_adjust_cfa_offset 8 > > > .cfi_rel_offset %rbp, 0 > > > -- > > > 2.21.0 > > > > > > > > > > /Jarkko > > > > /Jarkko
On Thu, Mar 11, 2021 at 05:36:06AM +0200, Jarkko Sakkinen wrote:
> Does it do any harm to put it there unconditionally?
Blows up your text footprint and I$ pressure. These instructions are 4
bytes each.
Aside from that, they're a NOP, so only consume front-end resources
(hopefully) on older CPUs and when IBT is disabled.
On 3/11/2021 12:42 AM, Peter Zijlstra wrote: > On Thu, Mar 11, 2021 at 05:36:06AM +0200, Jarkko Sakkinen wrote: >> Does it do any harm to put it there unconditionally? > > Blows up your text footprint and I$ pressure. These instructions are 4 > bytes each. > > Aside from that, they're a NOP, so only consume front-end resources > (hopefully) on older CPUs and when IBT is disabled. > Thanks Peter. I think probably we'll do the macro Boris suggested. That takes care of the visual clutter, and eliminates the need of using .byte when the assembler is outdated. -- Yu-cheng
On Wed, Mar 10, 2021 at 03:20:20PM -0800, Dave Hansen wrote: > On 3/10/21 2:55 PM, Yu, Yu-cheng wrote: > > On 3/10/2021 2:39 PM, Jarkko Sakkinen wrote: > >> On Wed, Mar 10, 2021 at 02:05:19PM -0800, Yu-cheng Yu wrote: > >>> When CET is enabled, __vdso_sgx_enter_enclave() needs an endbr64 > >>> in the beginning of the function. > >> > >> OK. > >> > >> What you should do is to explain what it does and why it's needed. > >> > > > > The endbr marks a branch target. Without the "no-track" prefix, if an > > indirect call/jmp reaches a non-endbr opcode, a control-protection fault > > is raised. Usually endbr's are inserted by the compiler. For assembly, > > these have to be put in manually. I will add this in the commit log if > > there is another revision. Thanks! > > This is close, but it's missing a detail or two that I think is > important for someone like Jarkko trying to figure out what it means for > his subsystem or driver. > > I'd probably say: > > ENDBR is a special new instruction for the Indirect Branch Tracking > (IBR) component of CET. IBT prevents attacks by ensuring that (most) > indirect branches and function calls may only land at ENDBR > instructions. Branches that don't follow the rules will result in > control flow (#CF) exceptions. > > ENDBR is a noop when IBT is unsupported or disabled. Most ENDBR > instructions are inserted automatically by the compiler, but branch > targets written in assembly must have ENDBR added manually, like this one. Thank you, this clears the whole thing a lot. Doesn't this mean that it could be there just as well unconditionally? /Jarkko
On Thu, Mar 11, 2021 at 09:42:05AM +0100, Peter Zijlstra wrote: > On Thu, Mar 11, 2021 at 05:36:06AM +0200, Jarkko Sakkinen wrote: > > Does it do any harm to put it there unconditionally? > > Blows up your text footprint and I$ pressure. These instructions are 4 > bytes each. > > Aside from that, they're a NOP, so only consume front-end resources > (hopefully) on older CPUs and when IBT is disabled. OK, understood, thanks for the explanation. /Jarkko
On Fri, Mar 12, 2021 at 06:55:57PM +0200, Jarkko Sakkinen wrote: > On Wed, Mar 10, 2021 at 03:20:20PM -0800, Dave Hansen wrote: > > On 3/10/21 2:55 PM, Yu, Yu-cheng wrote: > > > On 3/10/2021 2:39 PM, Jarkko Sakkinen wrote: > > >> On Wed, Mar 10, 2021 at 02:05:19PM -0800, Yu-cheng Yu wrote: > > >>> When CET is enabled, __vdso_sgx_enter_enclave() needs an endbr64 > > >>> in the beginning of the function. > > >> > > >> OK. > > >> > > >> What you should do is to explain what it does and why it's needed. > > >> > > > > > > The endbr marks a branch target. Without the "no-track" prefix, if an > > > indirect call/jmp reaches a non-endbr opcode, a control-protection fault > > > is raised. Usually endbr's are inserted by the compiler. For assembly, > > > these have to be put in manually. I will add this in the commit log if > > > there is another revision. Thanks! > > > > This is close, but it's missing a detail or two that I think is > > important for someone like Jarkko trying to figure out what it means for > > his subsystem or driver. > > > > I'd probably say: > > > > ENDBR is a special new instruction for the Indirect Branch Tracking > > (IBR) component of CET. IBT prevents attacks by ensuring that (most) > > indirect branches and function calls may only land at ENDBR > > instructions. Branches that don't follow the rules will result in > > control flow (#CF) exceptions. > > > > ENDBR is a noop when IBT is unsupported or disabled. Most ENDBR > > instructions are inserted automatically by the compiler, but branch > > targets written in assembly must have ENDBR added manually, like this one. > > Thank you, this clears the whole thing a lot. > > Doesn't this mean that it could be there just as well unconditionally? Please, ignore the question (got the answer). /Jarkko
On 3/12/21 8:55 AM, Jarkko Sakkinen wrote: >> ENDBR is a special new instruction for the Indirect Branch Tracking >> (IBT) component of CET. IBT prevents attacks by ensuring that (most) >> indirect branches and function calls may only land at ENDBR >> instructions. Branches that don't follow the rules will result in >> control flow (#CF) exceptions. >> >> ENDBR is a noop when IBT is unsupported or disabled. Most ENDBR >> instructions are inserted automatically by the compiler, but branch >> targets written in assembly must have ENDBR added manually, like this one. > Thank you, this clears the whole thing a lot. > > Doesn't this mean that it could be there just as well unconditionally? It could be there unconditionally. But, I think it's still worth the #ifdef just out of the principle of being as tidy as possible. The #ifdef is basically as low cost and low complexity as you get. It is also somewhat self-documenting: "This instruction is only necessary when your CPU supports IBT".
diff --git a/arch/x86/entry/vdso/vsgx.S b/arch/x86/entry/vdso/vsgx.S index 86a0e94f68df..a70d4d09f713 100644 --- a/arch/x86/entry/vdso/vsgx.S +++ b/arch/x86/entry/vdso/vsgx.S @@ -27,6 +27,9 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave) /* Prolog */ .cfi_startproc +#ifdef CONFIG_X86_CET + endbr64 +#endif push %rbp .cfi_adjust_cfa_offset 8 .cfi_rel_offset %rbp, 0
When CET is enabled, __vdso_sgx_enter_enclave() needs an endbr64 in the beginning of the function. Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> Cc: Andy Lutomirski <luto@kernel.org> Cc: Dave Hansen <dave.hansen@linux.intel.com> Cc: Jarkko Sakkinen <jarkko@kernel.org> --- arch/x86/entry/vdso/vsgx.S | 3 +++ 1 file changed, 3 insertions(+)