| Message ID | 20211126163830.30151-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On 26.11.2021 17:38, Andrew Cooper wrote: > --- a/xen/arch/x86/efi/stub.c > +++ b/xen/arch/x86/efi/stub.c > @@ -11,6 +11,8 @@ > #include <efi/efidevp.h> > #include <efi/efiapi.h> > > +bool __initdata efi_no_cet_ibt; I'm having trouble seeing what this is needed for - when this file gets built, neither boot.c nor runtime.c will get compiled, and hence there should not be any reference to the symbol that needs satisfying. > @@ -735,6 +736,14 @@ static void __init efi_init(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTabl > > StdOut = SystemTable->ConOut; > StdErr = SystemTable->StdErr ?: StdOut; > + > +#ifdef CONFIG_X86 CONFIG_XEN_IBT? > + /* > + * Heuristic. Look under an arbitrary function pointer to see if UEFI was > + * compiled with CET-IBT support. Experimentally some are not. > + */ > + efi_no_cet_ibt = !is_endbr64(efi_rs->GetTime); I'm afraid I consider this insufficient. Even if the core EFI was built with IBT support, some driver may not have been. Hence I think there needs to be a command line control to force turning off IBT. The only question is whether we want to also honor its positive form - that would, afaict, be a recipe for a guaranteed crash if used wrongly (and it would be meaningless when used on IBT-aware firmware). Not only in context of such a command line option I'm also inclined to suggest to invert the polarity of the variable, naming it "efi_cet_ibt" (and the command line sub-option "no-ibt" or "no-cet-ibt"). Jan
On 06/12/2021 11:06, Jan Beulich wrote: > On 26.11.2021 17:38, Andrew Cooper wrote: >> --- a/xen/arch/x86/efi/stub.c >> +++ b/xen/arch/x86/efi/stub.c >> @@ -11,6 +11,8 @@ >> #include <efi/efidevp.h> >> #include <efi/efiapi.h> >> >> +bool __initdata efi_no_cet_ibt; > I'm having trouble seeing what this is needed for - when this file gets > built, neither boot.c nor runtime.c will get compiled, and hence there > should not be any reference to the symbol that needs satisfying. > >> @@ -735,6 +736,14 @@ static void __init efi_init(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTabl >> >> StdOut = SystemTable->ConOut; >> StdErr = SystemTable->StdErr ?: StdOut; >> + >> +#ifdef CONFIG_X86 > CONFIG_XEN_IBT? > >> + /* >> + * Heuristic. Look under an arbitrary function pointer to see if UEFI was >> + * compiled with CET-IBT support. Experimentally some are not. >> + */ >> + efi_no_cet_ibt = !is_endbr64(efi_rs->GetTime); > I'm afraid I consider this insufficient. Even if the core EFI was built > with IBT support, some driver may not have been. That's not an issue. Everything is built together in practice. > Hence I think there > needs to be a command line control to force turning off IBT. The only > question is whether we want to also honor its positive form - that > would, afaict, be a recipe for a guaranteed crash if used wrongly (and > it would be meaningless when used on IBT-aware firmware). It turns out that IBT support is lacking from tianocore, so nothing is going to support IBT for a good while yet. https://bugzilla.tianocore.org/show_bug.cgi?id=3726 is the proposed change to the spec to support this. In the meantime, I'm just going to blanket disable IBT for RS calls. ~Andrew
On 10.12.2021 18:16, Andrew Cooper wrote: > On 06/12/2021 11:06, Jan Beulich wrote: >> On 26.11.2021 17:38, Andrew Cooper wrote: >>> --- a/xen/arch/x86/efi/stub.c >>> +++ b/xen/arch/x86/efi/stub.c >>> @@ -11,6 +11,8 @@ >>> #include <efi/efidevp.h> >>> #include <efi/efiapi.h> >>> >>> +bool __initdata efi_no_cet_ibt; >> I'm having trouble seeing what this is needed for - when this file gets >> built, neither boot.c nor runtime.c will get compiled, and hence there >> should not be any reference to the symbol that needs satisfying. >> >>> @@ -735,6 +736,14 @@ static void __init efi_init(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTabl >>> >>> StdOut = SystemTable->ConOut; >>> StdErr = SystemTable->StdErr ?: StdOut; >>> + >>> +#ifdef CONFIG_X86 >> CONFIG_XEN_IBT? >> >>> + /* >>> + * Heuristic. Look under an arbitrary function pointer to see if UEFI was >>> + * compiled with CET-IBT support. Experimentally some are not. >>> + */ >>> + efi_no_cet_ibt = !is_endbr64(efi_rs->GetTime); >> I'm afraid I consider this insufficient. Even if the core EFI was built >> with IBT support, some driver may not have been. > > That's not an issue. Everything is built together in practice. I'd be willing to take your word on this for everything that comes right with the firmware. I'd further be willing to accept that there are no add-in card BIOSes which may get involved. But I highly doubt that what you say applies to all software which may get loaded ahead of starting Xen. Such software may very well register hooks with core EFI. >> Hence I think there >> needs to be a command line control to force turning off IBT. The only >> question is whether we want to also honor its positive form - that >> would, afaict, be a recipe for a guaranteed crash if used wrongly (and >> it would be meaningless when used on IBT-aware firmware). > > It turns out that IBT support is lacking from tianocore, so nothing is > going to support IBT for a good while yet. > > https://bugzilla.tianocore.org/show_bug.cgi?id=3726 is the proposed > change to the spec to support this. > > In the meantime, I'm just going to blanket disable IBT for RS calls. Yeah, that's going to be okay for the time being. Jan
diff --git a/xen/arch/x86/efi/stub.c b/xen/arch/x86/efi/stub.c index 998493262641..5e44913e52db 100644 --- a/xen/arch/x86/efi/stub.c +++ b/xen/arch/x86/efi/stub.c @@ -11,6 +11,8 @@ #include <efi/efidevp.h> #include <efi/efiapi.h> +bool __initdata efi_no_cet_ibt; + /* * Here we are in EFI stub. EFI calls are not supported due to lack * of relevant functionality in compiler and/or linker. diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index f5af71837d5a..c19f993af922 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -21,6 +21,7 @@ #include <xen/string.h> #include <xen/stringify.h> #ifdef CONFIG_X86 +#include <asm/endbr.h> /* * Keep this arch-specific modified include in the common file, as moving * it to the arch specific include file would obscure that special care is @@ -735,6 +736,14 @@ static void __init efi_init(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTabl StdOut = SystemTable->ConOut; StdErr = SystemTable->StdErr ?: StdOut; + +#ifdef CONFIG_X86 + /* + * Heuristic. Look under an arbitrary function pointer to see if UEFI was + * compiled with CET-IBT support. Experimentally some are not. + */ + efi_no_cet_ibt = !is_endbr64(efi_rs->GetTime); +#endif } static void __init efi_console_set_mode(void) diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c index d2fdc28df3e0..ef54863542db 100644 --- a/xen/common/efi/runtime.c +++ b/xen/common/efi/runtime.c @@ -21,6 +21,7 @@ struct efi_rs_state { * don't strictly need that. */ unsigned long __aligned(32) cr3; + unsigned long msr_s_cet; #endif }; @@ -61,6 +62,7 @@ UINTN __read_mostly efi_apple_properties_len; /* Bit field representing available EFI features/properties. */ unsigned int efi_flags; +bool __read_mostly efi_no_cet_ibt; struct efi __read_mostly efi = { .acpi = EFI_INVALID_TABLE_ADDR, @@ -113,6 +115,17 @@ struct efi_rs_state efi_rs_enter(void) switch_cr3_cr4(mfn_to_maddr(efi_l4_mfn), read_cr4()); + /* + * If UEFI doesn't appear to be CET-IBT compatible, stash and clobber + * ENDBR_EN. Always read the current CET setting, because CET-SS isn't + * configured until very late on the BSP. + */ + if ( cpu_has_xen_ibt && efi_no_cet_ibt ) + { + rdmsrl(MSR_S_CET, state.msr_s_cet); + wrmsrl(MSR_S_CET, state.msr_s_cet & ~CET_ENDBR_EN); + } + return state; } @@ -122,6 +135,10 @@ void efi_rs_leave(struct efi_rs_state *state) if ( !state->cr3 ) return; + + if ( state->msr_s_cet ) + wrmsrl(MSR_S_CET, state->msr_s_cet); + switch_cr3_cr4(state->cr3, read_cr4()); if ( is_pv_vcpu(curr) && !is_idle_vcpu(curr) ) { diff --git a/xen/include/xen/efi.h b/xen/include/xen/efi.h index 94a7e547f97b..8c14f7f18718 100644 --- a/xen/include/xen/efi.h +++ b/xen/include/xen/efi.h @@ -30,6 +30,7 @@ union compat_pf_efi_info; struct xenpf_efi_runtime_call; struct compat_pf_efi_runtime_call; +extern bool efi_no_cet_ibt; bool efi_enabled(unsigned int feature); void efi_init_memory(void);
At least one TigerLake NUC has UEFI firmware which isn't CET-IBT compatible. Read under a function pointer to see whether an endbr64 instruction is present, and use this as a heuristic. 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> This was disappointing to discover. I've pestered some folk and maybe something will improve in due course, but it remains an open question how best to discover that Runtime Services are CET-IBT compatible. v2: * Switch to endbr helpers. --- xen/arch/x86/efi/stub.c | 2 ++ xen/common/efi/boot.c | 9 +++++++++ xen/common/efi/runtime.c | 17 +++++++++++++++++ xen/include/xen/efi.h | 1 + 4 files changed, 29 insertions(+)