| Message ID | 20220214125127.17985-70-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: Support for CET Indirect Branch Tracking | expand |
On 14.02.2022 13:51, Andrew Cooper wrote: > UEFI Runtime services, at the time of writing, aren't CET-IBT compatible. > Work is ongoing to address this. In the meantime, unconditionally disable IBT. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> > --- 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 > }; The latest with the next addition here we will probably want to ... > @@ -113,6 +114,19 @@ struct efi_rs_state efi_rs_enter(void) ... no longer have this be the function's return type. Jan
On 15/02/2022 16:53, Jan Beulich wrote: > On 14.02.2022 13:51, Andrew Cooper wrote: >> UEFI Runtime services, at the time of writing, aren't CET-IBT compatible. >> Work is ongoing to address this. In the meantime, unconditionally disable IBT. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks. > >> --- 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 >> }; > The latest with the next addition here we will probably want to ... > >> @@ -113,6 +114,19 @@ struct efi_rs_state efi_rs_enter(void) > ... no longer have this be the function's return type. So about this. why aren't we using __attribute__((force_align_arg_pointer)) ? It exists in at least GCC 4.1 and Clang 6. We're way way overdue bumping the minimum toolchain versions, and Clang 3.5=>6 is still very obsolete minimum version. This way, we're not depending on some very subtle ABI mechanics to try and keep the stack properly aligned. ~Andrew
On 16.02.2022 00:00, Andrew Cooper wrote: > On 15/02/2022 16:53, Jan Beulich wrote: >> On 14.02.2022 13:51, Andrew Cooper wrote: >>> --- 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 >>> }; >> The latest with the next addition here we will probably want to ... >> >>> @@ -113,6 +114,19 @@ struct efi_rs_state efi_rs_enter(void) >> ... no longer have this be the function's return type. > > So about this. > > why aren't we using __attribute__((force_align_arg_pointer)) ? It > exists in at least GCC 4.1 and Clang 6. Perhaps first and foremost because this is the first time I encounter this attribute, despite it having been around for so long. However, Clang 6 would be a little too high for the main box I have a Clang installed on - that's Clang 5 only (and, afaict, no option to upgrade without also upgrading the distro, while I'd also like to avoid having to also build myself Clang binaries; maybe sooner or later that's going to be unavoidable, though). While from binary searching its libraries it looks to know of that attribute, it still doesn't accept its use. The other issue I see is that using it would be fragile: We cannot afford to forget putting the attribute on any of the relevant functions. Whereas the present model makes it impossible to miss any instance. Finally the attribute's interaction with -mpreferred-stack-boundary= isn't spelled out anywhere. It looks to behave sanely on gcc 11, but who knows whether this has always been the case. Jan > We're way way overdue bumping the minimum toolchain versions, and Clang > 3.5=>6 is still very obsolete minimum version. This way, we're not > depending on some very subtle ABI mechanics to try and keep the stack > properly aligned. > > ~Andrew
diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c index e3ce85d118e4..13b0975866e3 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 }; @@ -113,6 +114,19 @@ struct efi_rs_state efi_rs_enter(void) switch_cr3_cr4(mfn_to_maddr(efi_l4_mfn), read_cr4()); + /* + * At the time of writing (2022), no UEFI firwmare is CET-IBT compatible. + * Work is under way to remedy this. + * + * Stash MSR_S_CET and clobber ENDBR_EN. This is necessary because + * SHSTK_EN isn't configured until very late on the BSP. + */ + if ( cpu_has_xen_ibt ) + { + rdmsrl(MSR_S_CET, state.msr_s_cet); + wrmsrl(MSR_S_CET, state.msr_s_cet & ~CET_ENDBR_EN); + } + return state; } @@ -122,6 +136,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) ) {
UEFI Runtime services, at the time of writing, aren't CET-IBT compatible. Work is ongoing to address this. In the meantime, unconditionally disable IBT. 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> https://bugzilla.tianocore.org/show_bug.cgi?id=3726 is the upstream tracking ticket. v2: * Rewrite to be an unconditional disable. --- xen/common/efi/runtime.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)