Re: [PATCH v6 01/25] x86/fpu/xstate: Manually check and add XFEATURE_CET_USER xstate bit

[Yang, Weijiang]

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@xxxxxxxxx>
> > ---
> >   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