| Message ID | 20211126212258.7550-2-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: Further harden function pointers | expand |
On 26.11.2021 22:22, Andrew Cooper wrote: > @@ -279,6 +280,27 @@ static void init_or_livepatch _apply_alternatives(struct alt_instr *start, > > if ( dest ) > { > + /* > + * When building for CET-IBT, all function pointer targets > + * should have an endbr64 instruction. > + * > + * If this is not the case, leave a warning because > + * something is wrong with the build. > + * > + * Otherwise, skip the endbr64 instruction. This is a > + * marginal perf improvement which saves on instruction > + * decode bandwidth. > + */ > + if ( IS_ENABLED(CONFIG_HAS_CC_CET_IBT) ) > + { > + if ( is_endbr64(dest) ) I would have given my R-b, but I don't see where is_endbr64() is coming from, and you don't list any prereqs here or in the cover letter. I'm afraid I don't fancy going hunt for it in the many other pending patches. Hence only on the assumption that the helper has got introduced before: Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On 01/12/2021 08:10, Jan Beulich wrote: > On 26.11.2021 22:22, Andrew Cooper wrote: >> @@ -279,6 +280,27 @@ static void init_or_livepatch _apply_alternatives(struct alt_instr *start, >> >> if ( dest ) >> { >> + /* >> + * When building for CET-IBT, all function pointer targets >> + * should have an endbr64 instruction. >> + * >> + * If this is not the case, leave a warning because >> + * something is wrong with the build. >> + * >> + * Otherwise, skip the endbr64 instruction. This is a >> + * marginal perf improvement which saves on instruction >> + * decode bandwidth. >> + */ >> + if ( IS_ENABLED(CONFIG_HAS_CC_CET_IBT) ) >> + { >> + if ( is_endbr64(dest) ) > I would have given my R-b, but I don't see where is_endbr64() is coming > from, and you don't list any prereqs here or in the cover letter. I'm > afraid I don't fancy going hunt for it in the many other pending patches. > Hence only on the assumption that the helper has got introduced before: > Reviewed-by: Jan Beulich <jbeulich@suse.com> Oh sorry - this series is based on the CET-IBT series, which adds CONFIG_HAS_CC_CET_IBT and is_endbr64(). ~Andrew
diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c index ec24692e9595..5ae4c80d5119 100644 --- a/xen/arch/x86/alternative.c +++ b/xen/arch/x86/alternative.c @@ -18,6 +18,7 @@ #include <xen/delay.h> #include <xen/types.h> #include <asm/apic.h> +#include <asm/endbr.h> #include <asm/processor.h> #include <asm/alternative.h> #include <xen/init.h> @@ -279,6 +280,27 @@ static void init_or_livepatch _apply_alternatives(struct alt_instr *start, if ( dest ) { + /* + * When building for CET-IBT, all function pointer targets + * should have an endbr64 instruction. + * + * If this is not the case, leave a warning because + * something is wrong with the build. + * + * Otherwise, skip the endbr64 instruction. This is a + * marginal perf improvement which saves on instruction + * decode bandwidth. + */ + if ( IS_ENABLED(CONFIG_HAS_CC_CET_IBT) ) + { + if ( is_endbr64(dest) ) + dest += 4; + else + printk(XENLOG_WARNING + "altcall %ps dest %ps has no endbr64\n", + orig, dest); + } + disp = dest - (orig + 5); ASSERT(disp == (int32_t)disp); *(int32_t *)(buf + 1) = disp;
When converting indirect to direct calls, there is no need to execute endbr64 instructions. Detect and optimise this case, leaving a warning in the case that no endbr64 was found, as it likely indicates a build error. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> --- xen/arch/x86/alternative.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)