On 23/05/2024 1:59 pm, Alexandre Chartre wrote: > > > On 5/23/24 14:42, Andrew Cooper wrote: >> On 23/05/2024 1:33 pm, Alexandre Chartre wrote: >>> diff --git a/arch/x86/entry/entry_64_compat.S >>> b/arch/x86/entry/entry_64_compat.S >>> index 11c9b8efdc4c..7fa04edc87e9 100644 >>> --- a/arch/x86/entry/entry_64_compat.S >>> +++ b/arch/x86/entry/entry_64_compat.S >>> @@ -91,7 +91,6 @@ >>> SYM_INNER_LABEL(entry_SYSENTER_compat_after_hwframe, SYM_L_GLOBAL) >>> IBRS_ENTER >>> UNTRAIN_RET >>> - CLEAR_BRANCH_HISTORY >>> /* >>> * SYSENTER doesn't filter flags, so we need to clear NT and AC >>> @@ -116,6 +115,12 @@ >>> SYM_INNER_LABEL(entry_SYSENTER_compat_after_hwframe, SYM_L_GLOBAL) >>> jnz .Lsysenter_fix_flags >>> .Lsysenter_flags_fixed: >>> + /* >>> + * CLEAR_BRANCH_HISTORY can call other functions. It should be >>> invoked >>> + * after making sure TF is cleared because single-step is >>> ignored only >>> + * for instructions inside the entry_SYSENTER_compat function. >>> + */ >>> + CLEAR_BRANCH_HISTORY >> >> Exactly the same is true of UNTRAIN_RET, although it will only manifest >> in i386 builds running on AMD hardware (SYSENTER is #UD on AMD hardware >> in Long mode.) >> >> #DB is IST so does handle it's own speculation safety. It should be >> safe to move all the speculation safety logic in the sysenter handler to >> after .Lsysenter_flags_fixed:, I think? >> > > Right, so something like this: > > --- a/arch/x86/entry/entry_64_compat.S > +++ b/arch/x86/entry/entry_64_compat.S > @@ -89,10 +89,6 @@ > SYM_INNER_LABEL(entry_SYSENTER_compat_after_hwframe, SYM_L_GLOBAL) > > cld > > - IBRS_ENTER > - UNTRAIN_RET > - CLEAR_BRANCH_HISTORY > - > /* > * SYSENTER doesn't filter flags, so we need to clear NT and AC > * ourselves. To save a few cycles, we can check whether > @@ -116,6 +112,15 @@ > SYM_INNER_LABEL(entry_SYSENTER_compat_after_hwframe, SYM_L_GLOBAL) > jnz .Lsysenter_fix_flags > .Lsysenter_flags_fixed: > > + /* > + * CPU bugs mitigations mechanisms can call other functions. They > + * should be invoked after making sure TF is cleared because > + * single-step is ignored only for instructions inside the > + * entry_SYSENTER_compat function. > + */ > + IBRS_ENTER > + UNTRAIN_RET > + CLEAR_BRANCH_HISTORY Yeah - this looks rather better. Although I'd suggest a blank line here if you're going to formalise the patch. ~Andrew
