| Message ID | 20230914063325.85503-2-weijiang.yang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable CET Virtualization | expand |
On Thu, 2023-09-14 at 02:33 -0400, Yang Weijiang wrote: > Remove XFEATURE_CET_USER entry from dependency array as the entry > doesn't > reflect true dependency between CET features and the xstate bit, > instead > manually check and add the bit back if either SHSTK or IBT is > supported. > > Both user mode shadow stack and indirect branch tracking features > depend > on XFEATURE_CET_USER bit in XSS to automatically save/restore user > mode > xstate registers, i.e., IA32_U_CET and IA32_PL3_SSP whenever > necessary. > > Although in real world a platform with IBT but no SHSTK is rare, but > in > virtualization world it's common, guest SHSTK and IBT can be > controlled > independently via userspace app. Nit, not sure we can assert it's common yet. It's true in general that guests can have CPUID combinations that don't appear in real world of course. Is that what you meant? Also, this doesn't discuss the real main reason for this patch, and that is that KVM will soon use the xfeature for user ibt, and so there will now be a reason to have XFEATURE_CET_USER depend on IBT. > > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> Otherwise: Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com> Tested-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
On 9/15/2023 6:39 AM, Edgecombe, Rick P wrote: > On Thu, 2023-09-14 at 02:33 -0400, Yang Weijiang wrote: >> Remove XFEATURE_CET_USER entry from dependency array as the entry >> doesn't >> reflect true dependency between CET features and the xstate bit, >> instead >> manually check and add the bit back if either SHSTK or IBT is >> supported. >> >> Both user mode shadow stack and indirect branch tracking features >> depend >> on XFEATURE_CET_USER bit in XSS to automatically save/restore user >> mode >> xstate registers, i.e., IA32_U_CET and IA32_PL3_SSP whenever >> necessary. >> >> Although in real world a platform with IBT but no SHSTK is rare, but >> in >> virtualization world it's common, guest SHSTK and IBT can be >> controlled >> independently via userspace app. > Nit, not sure we can assert it's common yet. It's true in general that > guests can have CPUID combinations that don't appear in real world of > course. Is that what you meant? Yes, guest CPUID features can be configured by userspace flexibly. > > Also, this doesn't discuss the real main reason for this patch, and > that is that KVM will soon use the xfeature for user ibt, and so there > will now be a reason to have XFEATURE_CET_USER depend on IBT. This is one justification for Linux OS, another reason is there's non-Linux OS which is using the user IBT feature. I should make the reasons clearer in changelog, thanks for pointing it out! >> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> > Otherwise: > > Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > Tested-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
On Fri, 2023-09-15 at 10:32 +0800, Yang, Weijiang wrote: > > > > Also, this doesn't discuss the real main reason for this patch, and > > that is that KVM will soon use the xfeature for user ibt, and so > > there > > will now be a reason to have XFEATURE_CET_USER depend on IBT. > > This is one justification for Linux OS, another reason is there's > non-Linux > OS which is using the user IBT feature. I should make the reasons > clearer > in changelog, thanks for pointing it out! The point I was trying to make was today (before this series) nothing on the system can use user IBT. Not the host, and not in any guest because KVM doesn't support it. So the added xfeature dependency on IBT was not previously needed. It is being added only for KVM CET support (which, yes, may run on guests with non-standard CPUID).
On 9/16/2023 12:35 AM, Edgecombe, Rick P wrote: > On Fri, 2023-09-15 at 10:32 +0800, Yang, Weijiang wrote: >>> Also, this doesn't discuss the real main reason for this patch, and >>> that is that KVM will soon use the xfeature for user ibt, and so >>> there >>> will now be a reason to have XFEATURE_CET_USER depend on IBT. >> This is one justification for Linux OS, another reason is there's >> non-Linux >> OS which is using the user IBT feature. I should make the reasons >> clearer >> in changelog, thanks for pointing it out! > The point I was trying to make was today (before this series) nothing > on the system can use user IBT. Not the host, and not in any guest > because KVM doesn't support it. So the added xfeature dependency on IBT > was not previously needed. It is being added only for KVM CET support > (which, yes, may run on guests with non-standard CPUID). Agree, I'll highlight this in changelog, thanks!
On Thu, 2023-09-14 at 02:33 -0400, Yang Weijiang wrote: > Remove XFEATURE_CET_USER entry from dependency array as the entry doesn't > reflect true dependency between CET features and the xstate bit, instead > manually check and add the bit back if either SHSTK or IBT is supported. > > Both user mode shadow stack and indirect branch tracking features depend > on XFEATURE_CET_USER bit in XSS to automatically save/restore user mode > xstate registers, i.e., IA32_U_CET and IA32_PL3_SSP whenever necessary. > > Although in real world a platform with IBT but no SHSTK is rare, but in > virtualization world it's common, guest SHSTK and IBT can be controlled > independently via userspace app. > > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> > --- > arch/x86/kernel/fpu/xstate.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c > index cadf68737e6b..12c8cb278346 100644 > --- a/arch/x86/kernel/fpu/xstate.c > +++ b/arch/x86/kernel/fpu/xstate.c > @@ -73,7 +73,6 @@ static unsigned short xsave_cpuid_features[] __initdata = { > [XFEATURE_PT_UNIMPLEMENTED_SO_FAR] = X86_FEATURE_INTEL_PT, > [XFEATURE_PKRU] = X86_FEATURE_OSPKE, > [XFEATURE_PASID] = X86_FEATURE_ENQCMD, > - [XFEATURE_CET_USER] = X86_FEATURE_SHSTK, > [XFEATURE_XTILE_CFG] = X86_FEATURE_AMX_TILE, > [XFEATURE_XTILE_DATA] = X86_FEATURE_AMX_TILE, > }; > @@ -798,6 +797,14 @@ void __init fpu__init_system_xstate(unsigned int legacy_size) > fpu_kernel_cfg.max_features &= ~BIT_ULL(i); > } > > + /* > + * Manually add CET user mode xstate bit if either SHSTK or IBT is > + * available. Both features depend on the xstate bit to save/restore > + * CET user mode state. > + */ > + if (boot_cpu_has(X86_FEATURE_SHSTK) || boot_cpu_has(X86_FEATURE_IBT)) > + fpu_kernel_cfg.max_features |= BIT_ULL(XFEATURE_CET_USER); > + > if (!cpu_feature_enabled(X86_FEATURE_XFD)) > fpu_kernel_cfg.max_features &= ~XFEATURE_MASK_USER_DYNAMIC; > The goal of the xsave_cpuid_features is to disable xfeature state bits which are enabled in CPUID, but their parent feature bit (e.g X86_FEATURE_AVX512) is disabled in CPUID, something that should not happen on real CPU, but can happen if the user explicitly disables the feature on the kernel command line and/or due to virtualization. However the above code does the opposite, it will enable XFEATURE_CET_USER xsaves component, when in fact, it might be disabled in the CPUID (and one can say that in theory such configuration is even useful, since the kernel can still context switch CET msrs manually). So I think that the code should do this instead: if (!boot_cpu_has(X86_FEATURE_SHSTK) && !boot_cpu_has(X86_FEATURE_IBT)) fpu_kernel_cfg.max_features &= ~BIT_ULL(XFEATURE_CET_USER); Best regards, Maxim Levitsky
On 11/1/2023 1:43 AM, Maxim Levitsky wrote: > On Thu, 2023-09-14 at 02:33 -0400, Yang Weijiang wrote: >> Remove XFEATURE_CET_USER entry from dependency array as the entry doesn't >> reflect true dependency between CET features and the xstate bit, instead >> manually check and add the bit back if either SHSTK or IBT is supported. >> >> Both user mode shadow stack and indirect branch tracking features depend >> on XFEATURE_CET_USER bit in XSS to automatically save/restore user mode >> xstate registers, i.e., IA32_U_CET and IA32_PL3_SSP whenever necessary. >> >> Although in real world a platform with IBT but no SHSTK is rare, but in >> virtualization world it's common, guest SHSTK and IBT can be controlled >> independently via userspace app. >> >> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> >> --- >> arch/x86/kernel/fpu/xstate.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c >> index cadf68737e6b..12c8cb278346 100644 >> --- a/arch/x86/kernel/fpu/xstate.c >> +++ b/arch/x86/kernel/fpu/xstate.c >> @@ -73,7 +73,6 @@ static unsigned short xsave_cpuid_features[] __initdata = { >> [XFEATURE_PT_UNIMPLEMENTED_SO_FAR] = X86_FEATURE_INTEL_PT, >> [XFEATURE_PKRU] = X86_FEATURE_OSPKE, >> [XFEATURE_PASID] = X86_FEATURE_ENQCMD, >> - [XFEATURE_CET_USER] = X86_FEATURE_SHSTK, >> [XFEATURE_XTILE_CFG] = X86_FEATURE_AMX_TILE, >> [XFEATURE_XTILE_DATA] = X86_FEATURE_AMX_TILE, >> }; >> @@ -798,6 +797,14 @@ void __init fpu__init_system_xstate(unsigned int legacy_size) >> fpu_kernel_cfg.max_features &= ~BIT_ULL(i); >> } >> >> + /* >> + * Manually add CET user mode xstate bit if either SHSTK or IBT is >> + * available. Both features depend on the xstate bit to save/restore >> + * CET user mode state. >> + */ >> + if (boot_cpu_has(X86_FEATURE_SHSTK) || boot_cpu_has(X86_FEATURE_IBT)) >> + fpu_kernel_cfg.max_features |= BIT_ULL(XFEATURE_CET_USER); >> + >> if (!cpu_feature_enabled(X86_FEATURE_XFD)) >> fpu_kernel_cfg.max_features &= ~XFEATURE_MASK_USER_DYNAMIC; >> > > The goal of the xsave_cpuid_features is to disable xfeature state bits which are enabled > in CPUID, but their parent feature bit (e.g X86_FEATURE_AVX512) is disabled in CPUID, > something that should not happen on real CPU, but can happen if the user explicitly > disables the feature on the kernel command line and/or due to virtualization. > > However the above code does the opposite, it will enable XFEATURE_CET_USER xsaves component, > when in fact, it might be disabled in the CPUID (and one can say that in theory such > configuration is even useful, since the kernel can still context switch CET msrs manually). > > > So I think that the code should do this instead: > > if (!boot_cpu_has(X86_FEATURE_SHSTK) && !boot_cpu_has(X86_FEATURE_IBT)) > fpu_kernel_cfg.max_features &= ~BIT_ULL(XFEATURE_CET_USER); Hi, Maxim, Thanks a lot for the comments on the series! I'll will check and reply them after finish an urgent task at hand. Yeah, it looks good to me and makes the handling logic more consistent! > Best regards, > Maxim Levitsky > > > >
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c index cadf68737e6b..12c8cb278346 100644 --- a/arch/x86/kernel/fpu/xstate.c +++ b/arch/x86/kernel/fpu/xstate.c @@ -73,7 +73,6 @@ static unsigned short xsave_cpuid_features[] __initdata = { [XFEATURE_PT_UNIMPLEMENTED_SO_FAR] = X86_FEATURE_INTEL_PT, [XFEATURE_PKRU] = X86_FEATURE_OSPKE, [XFEATURE_PASID] = X86_FEATURE_ENQCMD, - [XFEATURE_CET_USER] = X86_FEATURE_SHSTK, [XFEATURE_XTILE_CFG] = X86_FEATURE_AMX_TILE, [XFEATURE_XTILE_DATA] = X86_FEATURE_AMX_TILE, }; @@ -798,6 +797,14 @@ void __init fpu__init_system_xstate(unsigned int legacy_size) fpu_kernel_cfg.max_features &= ~BIT_ULL(i); } + /* + * Manually add CET user mode xstate bit if either SHSTK or IBT is + * available. Both features depend on the xstate bit to save/restore + * CET user mode state. + */ + if (boot_cpu_has(X86_FEATURE_SHSTK) || boot_cpu_has(X86_FEATURE_IBT)) + fpu_kernel_cfg.max_features |= BIT_ULL(XFEATURE_CET_USER); + if (!cpu_feature_enabled(X86_FEATURE_XFD)) fpu_kernel_cfg.max_features &= ~XFEATURE_MASK_USER_DYNAMIC;
Remove XFEATURE_CET_USER entry from dependency array as the entry doesn't reflect true dependency between CET features and the xstate bit, instead manually check and add the bit back if either SHSTK or IBT is supported. Both user mode shadow stack and indirect branch tracking features depend on XFEATURE_CET_USER bit in XSS to automatically save/restore user mode xstate registers, i.e., IA32_U_CET and IA32_PL3_SSP whenever necessary. Although in real world a platform with IBT but no SHSTK is rare, but in virtualization world it's common, guest SHSTK and IBT can be controlled independently via userspace app. Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> --- arch/x86/kernel/fpu/xstate.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)