| Message ID | 20210316151320.6123-7-yu-cheng.yu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Control-flow Enforcement: Indirect Branch Tracking | expand |
On 3/16/21 8:13 AM, Yu-cheng Yu wrote: > --- a/arch/x86/entry/calling.h > +++ b/arch/x86/entry/calling.h > @@ -392,3 +392,21 @@ For 32-bit we have the following conventions - kernel is built with > .endm > > #endif /* CONFIG_SMP */ > +/* > + * ENDBR is an instruction for the Indirect Branch Tracking (IBT) component > + * of CET. IBT prevents attacks by ensuring that (most) indirect branches > + * 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. > + */ > +.macro ENDBR > +#ifdef CONFIG_X86_CET > +#ifdef __i386__ > + endbr32 > +#else > + endbr64 > +#endif > +#endif > +.endm Is "#ifdef __i386__" the right thing to use here? I guess ENDBR only ends up getting used in the VDSO, but there's a lot of non-userspace-exposed stuff in calling.h. It seems a bit weird to have the normally userspace-only __i386__ in there. I don't see any existing direct use of __i386__ in arch/x86/entry/vdso.
On 3/16/2021 8:49 AM, Dave Hansen wrote: > On 3/16/21 8:13 AM, Yu-cheng Yu wrote: >> --- a/arch/x86/entry/calling.h >> +++ b/arch/x86/entry/calling.h >> @@ -392,3 +392,21 @@ For 32-bit we have the following conventions - kernel is built with >> .endm >> >> #endif /* CONFIG_SMP */ >> +/* >> + * ENDBR is an instruction for the Indirect Branch Tracking (IBT) component >> + * of CET. IBT prevents attacks by ensuring that (most) indirect branches >> + * 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. >> + */ >> +.macro ENDBR >> +#ifdef CONFIG_X86_CET >> +#ifdef __i386__ >> + endbr32 >> +#else >> + endbr64 >> +#endif >> +#endif >> +.endm > > Is "#ifdef __i386__" the right thing to use here? I guess ENDBR only > ends up getting used in the VDSO, but there's a lot of > non-userspace-exposed stuff in calling.h. It seems a bit weird to have > the normally userspace-only __i386__ in there. > > I don't see any existing direct use of __i386__ in arch/x86/entry/vdso. > Good point. My thought was, __i386__ comes from the compiler having the -m32 command-line option, and it is not dependent on anything else. Alternatively, there is another compiler-defined macro _CET_ENDBR that can be used. We can put the following in calling.h: #ifdef __CET__ #include <cet.h> #else #define _CET_ENDBR #endif and then use _CET_ENDBR in other files. How is that? In the future, in case we have kernel-mode IBT, ENDBR macros are also needed for other assembly files. Thanks, Yu-cheng
On 3/16/21 10:12 AM, Yu, Yu-cheng wrote: > On 3/16/2021 8:49 AM, Dave Hansen wrote: ... >> Is "#ifdef __i386__" the right thing to use here? I guess ENDBR only >> ends up getting used in the VDSO, but there's a lot of >> non-userspace-exposed stuff in calling.h. It seems a bit weird to have >> the normally userspace-only __i386__ in there. >> >> I don't see any existing direct use of __i386__ in arch/x86/entry/vdso. > > Good point. My thought was, __i386__ comes from the compiler having the > -m32 command-line option, and it is not dependent on anything else. > > Alternatively, there is another compiler-defined macro _CET_ENDBR that > can be used. We can put the following in calling.h: > > #ifdef __CET__ > #include <cet.h> > #else > #define _CET_ENDBR > #endif > > and then use _CET_ENDBR in other files. How is that? > > In the future, in case we have kernel-mode IBT, ENDBR macros are also > needed for other assembly files. First of all, I think putting the macro in calling.h is wrong if it will be used exclusively in the VDSO. If it's VDSO-only, please put it in a local header in the vdso/ directory, maybe even a new cet.h. Also, Boris asked for two *different* macros for 32 and 64-bit: https://lore.kernel.org/linux-api/20210310231731.GK23521@zn.tnic/ Could you do that in the next version, please?
On Tue, Mar 16, 2021 at 10:12:39AM -0700, Yu, Yu-cheng wrote: > Alternatively, there is another compiler-defined macro _CET_ENDBR that can > be used. We can put the following in calling.h: Not calling.h - this is apparently needed in vdso code only so I guess some header there, arch/x86/include/asm/vdso.h maybe? In the #else /* __ASSEMBLER__ */ branch maybe... > #ifdef __CET__ > #include <cet.h> > #else > #define _CET_ENDBR > #endif > > and then use _CET_ENDBR in other files. How is that? What does that macro do? Issue an ENDBR only?
On 3/16/2021 10:30 AM, Borislav Petkov wrote: > On Tue, Mar 16, 2021 at 10:12:39AM -0700, Yu, Yu-cheng wrote: >> Alternatively, there is another compiler-defined macro _CET_ENDBR that can >> be used. We can put the following in calling.h: > > Not calling.h - this is apparently needed in vdso code only so I guess > some header there, arch/x86/include/asm/vdso.h maybe? In the > > #else /* __ASSEMBLER__ */ > > branch maybe... > >> #ifdef __CET__ >> #include <cet.h> >> #else >> #define _CET_ENDBR >> #endif >> >> and then use _CET_ENDBR in other files. How is that? > > What does that macro do? Issue an ENDBR only? > Yes, issue endbr32, endbr64, or nothing when cet is not enabled.
On 3/16/2021 10:28 AM, Dave Hansen wrote: > On 3/16/21 10:12 AM, Yu, Yu-cheng wrote: >> On 3/16/2021 8:49 AM, Dave Hansen wrote: > ... >>> Is "#ifdef __i386__" the right thing to use here? I guess ENDBR only >>> ends up getting used in the VDSO, but there's a lot of >>> non-userspace-exposed stuff in calling.h. It seems a bit weird to have >>> the normally userspace-only __i386__ in there. >>> >>> I don't see any existing direct use of __i386__ in arch/x86/entry/vdso. >> >> Good point. My thought was, __i386__ comes from the compiler having the >> -m32 command-line option, and it is not dependent on anything else. >> >> Alternatively, there is another compiler-defined macro _CET_ENDBR that >> can be used. We can put the following in calling.h: >> >> #ifdef __CET__ >> #include <cet.h> >> #else >> #define _CET_ENDBR >> #endif >> >> and then use _CET_ENDBR in other files. How is that? >> >> In the future, in case we have kernel-mode IBT, ENDBR macros are also >> needed for other assembly files. > > First of all, I think putting the macro in calling.h is wrong if it will > be used exclusively in the VDSO. If it's VDSO-only, please put it in a > local header in the vdso/ directory, maybe even a new cet.h. > > Also, Boris asked for two *different* macros for 32 and 64-bit: > > https://lore.kernel.org/linux-api/20210310231731.GK23521@zn.tnic/ > > Could you do that in the next version, please? > Yes, we can do two macros, probably in arch/x86/include/asm/vdso.h.
On 3/16/21 10:44 AM, Yu, Yu-cheng wrote: >> Also, Boris asked for two *different* macros for 32 and 64-bit: >> >> https://lore.kernel.org/linux-api/20210310231731.GK23521@zn.tnic/ >> >> Could you do that in the next version, please? > > Yes, we can do two macros, probably in arch/x86/include/asm/vdso.h. *But*, please do leverage _CET_ENDBR if you can. It seems awfully close to what we need. If it works out, just use it for the 32/64-bit switch.
On Tue, Mar 16, 2021 at 10:12:39AM -0700, Yu, Yu-cheng wrote: > Alternatively, there is another compiler-defined macro _CET_ENDBR that can > be used. We can put the following in calling.h: > > #ifdef __CET__ > #include <cet.h> > #else > #define _CET_ENDBR > #endif > > and then use _CET_ENDBR in other files. How is that? > > In the future, in case we have kernel-mode IBT, ENDBR macros are also needed > for other assembly files. Can we please call it IBT_ENDBR or just ENDBR. CET is a horrible name, seeing how it is not specific.
On 3/16/2021 12:57 PM, Peter Zijlstra wrote: > On Tue, Mar 16, 2021 at 10:12:39AM -0700, Yu, Yu-cheng wrote: >> Alternatively, there is another compiler-defined macro _CET_ENDBR that can >> be used. We can put the following in calling.h: >> >> #ifdef __CET__ >> #include <cet.h> >> #else >> #define _CET_ENDBR >> #endif >> >> and then use _CET_ENDBR in other files. How is that? >> >> In the future, in case we have kernel-mode IBT, ENDBR macros are also needed >> for other assembly files. > > Can we please call it IBT_ENDBR or just ENDBR. CET is a horrible name, > seeing how it is not specific. > _CET_ENDBR is from the compiler and we cannot change it. We can do: #define ENDBR _CET_ENDBR How is that?
On Tue, Mar 16, 2021 at 01:05:30PM -0700, Yu, Yu-cheng wrote: > On 3/16/2021 12:57 PM, Peter Zijlstra wrote: > > On Tue, Mar 16, 2021 at 10:12:39AM -0700, Yu, Yu-cheng wrote: > > > Alternatively, there is another compiler-defined macro _CET_ENDBR that can > > > be used. We can put the following in calling.h: > > > > > > #ifdef __CET__ > > > #include <cet.h> > > > #else > > > #define _CET_ENDBR > > > #endif > > > > > > and then use _CET_ENDBR in other files. How is that? > > > > > > In the future, in case we have kernel-mode IBT, ENDBR macros are also needed > > > for other assembly files. > > > > Can we please call it IBT_ENDBR or just ENDBR. CET is a horrible name, > > seeing how it is not specific. > > > > _CET_ENDBR is from the compiler and we cannot change it. We can do: > > #define ENDBR _CET_ENDBR > > How is that? Do we really want to include compiler headers? AFAICT it's not a built-in. Also what about clang? This thing seems trivial enough to build our own, it's a single damn instruction. That also means we don't have to worry about changes to header files we don't control.
On 3/16/2021 1:15 PM, Peter Zijlstra wrote: > On Tue, Mar 16, 2021 at 01:05:30PM -0700, Yu, Yu-cheng wrote: >> On 3/16/2021 12:57 PM, Peter Zijlstra wrote: >>> On Tue, Mar 16, 2021 at 10:12:39AM -0700, Yu, Yu-cheng wrote: >>>> Alternatively, there is another compiler-defined macro _CET_ENDBR that can >>>> be used. We can put the following in calling.h: >>>> >>>> #ifdef __CET__ >>>> #include <cet.h> >>>> #else >>>> #define _CET_ENDBR >>>> #endif >>>> >>>> and then use _CET_ENDBR in other files. How is that? >>>> >>>> In the future, in case we have kernel-mode IBT, ENDBR macros are also needed >>>> for other assembly files. >>> >>> Can we please call it IBT_ENDBR or just ENDBR. CET is a horrible name, >>> seeing how it is not specific. >>> >> >> _CET_ENDBR is from the compiler and we cannot change it. We can do: >> >> #define ENDBR _CET_ENDBR >> >> How is that? > > Do we really want to include compiler headers? AFAICT it's not a > built-in. Also what about clang? > > This thing seems trivial enough to build our own, it's a single damn > instruction. That also means we don't have to worry about changes to > header files we don't control. > Then, what about moving what I had earlier to vdso.h? If we don't want __i386__ either, then make it two macros. +.macro ENDBR +#ifdef CONFIG_X86_CET +#ifdef __i386__ + endbr32 +#else + endbr64 +#endif +#endif +.endm
On 3/16/2021 1:26 PM, Yu, Yu-cheng wrote: > On 3/16/2021 1:15 PM, Peter Zijlstra wrote: >> On Tue, Mar 16, 2021 at 01:05:30PM -0700, Yu, Yu-cheng wrote: >>> On 3/16/2021 12:57 PM, Peter Zijlstra wrote: >>>> On Tue, Mar 16, 2021 at 10:12:39AM -0700, Yu, Yu-cheng wrote: >>>>> Alternatively, there is another compiler-defined macro _CET_ENDBR >>>>> that can >>>>> be used. We can put the following in calling.h: >>>>> >>>>> #ifdef __CET__ >>>>> #include <cet.h> >>>>> #else >>>>> #define _CET_ENDBR >>>>> #endif >>>>> >>>>> and then use _CET_ENDBR in other files. How is that? >>>>> >>>>> In the future, in case we have kernel-mode IBT, ENDBR macros are >>>>> also needed >>>>> for other assembly files. >>>> >>>> Can we please call it IBT_ENDBR or just ENDBR. CET is a horrible name, >>>> seeing how it is not specific. >>>> >>> >>> _CET_ENDBR is from the compiler and we cannot change it. We can do: >>> >>> #define ENDBR _CET_ENDBR >>> >>> How is that? >> >> Do we really want to include compiler headers? AFAICT it's not a >> built-in. Also what about clang? >> >> This thing seems trivial enough to build our own, it's a single damn >> instruction. That also means we don't have to worry about changes to >> header files we don't control. >> > > Then, what about moving what I had earlier to vdso.h? > If we don't want __i386__ either, then make it two macros. > > +.macro ENDBR > +#ifdef CONFIG_X86_CET > +#ifdef __i386__ > + endbr32 > +#else > + endbr64 > +#endif > +#endif > +.endm I will make it like the following: diff --git a/arch/x86/include/asm/vdso.h b/arch/x86/include/asm/vdso.h index 98aa103eb4ab..4c0262dcb93d 100644 --- a/arch/x86/include/asm/vdso.h +++ b/arch/x86/include/asm/vdso.h @@ -52,6 +52,15 @@ extern int map_vdso_once(const struct vdso_image *image, unsigned long addr); extern bool fixup_vdso_exception(struct pt_regs *regs, int trapnr, unsigned long error_code, unsigned long fault_addr); -#endif /* __ASSEMBLER__ */ +#else /* __ASSEMBLER__ */ + +#ifdef CONFIG_X86_CET +#define ENDBR64 endbr64 +#define ENDBR32 endbr32 +#else /*!CONFIG_X86_CET */ +#define ENDBR64 +#define ENDBR32 +#endif +#endif /* __ASSEMBLER__ */ #endif /* _ASM_X86_VDSO_H */
On Tue, Mar 16, 2021 at 01:26:52PM -0700, Yu, Yu-cheng wrote: > Then, what about moving what I had earlier to vdso.h? > If we don't want __i386__ either, then make it two macros. vdso.h seems to use CONFIG_X86_{64,32} resp. > +.macro ENDBR > +#ifdef CONFIG_X86_CET And shouldn't that be CONFIG_X86_IBT ? > +#ifdef __i386__ #ifdef CONFIG_X86_32 > + endbr32 > +#else > + endbr64 > +#endif > +#endif > +.endm
diff --git a/arch/x86/entry/calling.h b/arch/x86/entry/calling.h index 07a9331d55e7..a63d33f7f069 100644 --- a/arch/x86/entry/calling.h +++ b/arch/x86/entry/calling.h @@ -392,3 +392,21 @@ For 32-bit we have the following conventions - kernel is built with .endm #endif /* CONFIG_SMP */ +/* + * ENDBR is an instruction for the Indirect Branch Tracking (IBT) component + * of CET. IBT prevents attacks by ensuring that (most) indirect branches + * 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. + */ +.macro ENDBR +#ifdef CONFIG_X86_CET +#ifdef __i386__ + endbr32 +#else + endbr64 +#endif +#endif +.endm
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. There are two ENDBR versions: one for 64-bit and the other for 32. Introduce a macro to eliminate ifdeffery at call sites. Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> Cc: Andy Lutomirski <luto@kernel.org> Cc: Borislav Petkov <bp@alien8.de> Cc: Dave Hansen <dave.hansen@linux.intel.com> Cc: Jarkko Sakkinen <jarkko@kernel.org> Cc: Peter Zijlstra <peterz@infradead.org> --- arch/x86/entry/calling.h | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)