lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <a3a14562-db72-4c19-9f40-7778f14fc516@intel.com> Date: Wed, 6 Dec 2023 09:03:50 +0800 From: "Yang, Weijiang" <weijiang.yang@...el.com> To: Maxim Levitsky <mlevitsk@...hat.com> CC: <linux-kernel@...r.kernel.org>, <kvm@...r.kernel.org>, <dave.hansen@...el.com>, <pbonzini@...hat.com>, <seanjc@...gle.com>, <peterz@...radead.org>, <chao.gao@...el.com>, <rick.p.edgecombe@...el.com>, <john.allen@....com> Subject: Re: [PATCH v7 02/26] x86/fpu/xstate: Refine CET user xstate bit enabling On 12/5/2023 5:53 PM, Maxim Levitsky wrote: > On Fri, 2023-12-01 at 14:51 +0800, Yang, Weijiang wrote: >> On 12/1/2023 1:26 AM, Maxim Levitsky wrote: >>> On Fri, 2023-11-24 at 00:53 -0500, Yang Weijiang wrote: >>>> Remove XFEATURE_CET_USER entry from dependency array as the entry doesn't >>>> reflect true dependency between CET features and the user xstate bit. >>>> Enable the bit in fpu_kernel_cfg.max_features when either SHSTK or IBT is >>>> available. >>>> >>>> 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. >>>> >>>> Note, the issue, i.e., CPUID only enumerates IBT but no SHSTK is resulted >>>> from CET KVM series which synthesizes guest CPUIDs based on userspace >>>> settings,in real world the case is rare. In other words, the exitings >>>> dependency check is correct when only user mode SHSTK is available. >>>> >>>> Signed-off-by: Yang Weijiang <weijiang.yang@...el.com> >>>> Reviewed-by: Rick Edgecombe <rick.p.edgecombe@...el.com> >>>> Tested-by: Rick Edgecombe <rick.p.edgecombe@...el.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 73f6bc00d178..6e50a4251e2b 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); >>>> } >>>> >>>> + /* >>>> + * CET user mode xstate bit has been cleared by above sanity check. >>>> + * Now pick it up if either SHSTK or IBT is available. Either feature >>>> + * depends on the xstate bit to save/restore user mode states. >>>> + */ >>>> + 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; >>>> >>> I am curious: >>> >>> Any reason why my review feedback was not applied even though you did agree >>> that it is reasonable? >> My apology! I changed the patch per you feedback but found XFEATURE_CET_USER didn't >> work before sending out v7 version, after a close look at the existing code: >> >> for (i = 0; i < ARRAY_SIZE(xsave_cpuid_features); i++) { >> unsigned short cid = xsave_cpuid_features[i]; >> >> /* Careful: X86_FEATURE_FPU is 0! */ >> if ((i != XFEATURE_FP && !cid) || !boot_cpu_has(cid)) >> fpu_kernel_cfg.max_features &= ~BIT_ULL(i); >> } >> >> With removal of XFEATURE_CET_USER entry from xsave_cpuid_features, actually >> above check will clear the bit from fpu_kernel_cfg.max_features. > Are you sure about this? If we remove the XFEATURE_CET_USER from the xsave_cpuid_features, > then the above loop will not touch it - it loops only over the items in the xsave_cpuid_features > array. No, the code is a bit tricky, the actual array size is XFEATURE_XTILE_DATA( ie, 18) + 1, those xfeature bits not listed in init code leave a blank entry with xsave_cpuid_features[i] == 0, so for the blank elements, the loop hits (i != XFEATURE_FP && !cid) then the relevant xfeature bit for i is cleared in fpu_kernel_cfg.max_features. I had the same illusion at first when I replied your comments in v6, and modified the code as you suggested but found the issue during tests. Please double check it. > What I suggested was that we remove the XFEATURE_CET_USER from the xsave_cpuid_features > and instead do this after the above loop. > > if (!boot_cpu_has(X86_FEATURE_SHSTK) && !boot_cpu_has(X86_FEATURE_IBT)) > fpu_kernel_cfg.max_features &= ~BIT_ULL(XFEATURE_CET_USER); > > Which is pretty much just a manual iteration of the loop, just instead of checking > for absence of single feature, it checks that both features are absent. > > Best regards, > Maxim Levitsky > > >> So now I need >> to add it back conditionally. >> Your sample code is more consistent with existing code in style, but I don't want to >> hack into the loop and handle XFEATURE_CET_USER specifically. Just keep the handling >> and rewording the comments which is also straightforward. >> >>> https://lore.kernel.org/lkml/c72dfaac-1622-94cf-a81d-9d7ed81b2f55@intel.com/ >>> >>> Best regard >>> Maxim Levitsky >>> > > >
Powered by blists - more mailing lists