Re: [PATCH v5 05/39] x86/fpu/xstate: Introduce CET MSR and XSAVES supervisor states

[Kees Cook]

On Thu, Jan 19, 2023 at 01:22:43PM -0800, Rick Edgecombe wrote:
> From: Yu-cheng Yu <yu-cheng.yu@xxxxxxxxx>
> 
> Shadow stack register state can be managed with XSAVE. The registers
> can logically be separated into two groups:
>         * Registers controlling user-mode operation
>         * Registers controlling kernel-mode operation
> 
> The architecture has two new XSAVE state components: one for each group
> of those groups of registers. This lets an OS manage them separately if
> it chooses. Future patches for host userspace and KVM guests will only
> utilize the user-mode registers, so only configure XSAVE to save
> user-mode registers. This state will add 16 bytes to the xsave buffer
> size.
> 
> Future patches will use the user-mode XSAVE area to save guest user-mode
> CET state. However, VMCS includes new fields for guest CET supervisor
> states. KVM can use these to save and restore guest supervisor state, so
> host supervisor XSAVE support is not required.
> 
> Adding this exacerbates the already unwieldy if statement in
> check_xstate_against_struct() that handles warning about un-implemented
> xfeatures. So refactor these check's by having XCHECK_SZ() set a bool when
> it actually check's the xfeature. This ends up exceeding 80 chars, but was
> better on balance than other options explored. Pass the bool as pointer to
> make it clear that XCHECK_SZ() can change the variable.
> 
> While configuring user-mode XSAVE, clarify kernel-mode registers are not
> managed by XSAVE by defining the xfeature in
> XFEATURE_MASK_SUPERVISOR_UNSUPPORTED, like is done for XFEATURE_MASK_PT.
> This serves more of a documentation as code purpose, and functionally,
> only enables a few safety checks.
> 
> Both XSAVE state components are supervisor states, even the state
> controlling user-mode operation. This is a departure from earlier features
> like protection keys where the PKRU state is a normal user
> (non-supervisor) state. Having the user state be supervisor-managed
> ensures there is no direct, unprivileged access to it, making it harder
> for an attacker to subvert CET.
> 
> To facilitate this privileged access, define the two user-mode CET MSRs,
> and the bits defined in those MSRs relevant to future shadow stack
> enablement patches.
> 
> Tested-by: Pengfei Xu <pengfei.xu@xxxxxxxxx>
> Tested-by: John Allen <john.allen@xxxxxxx>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@xxxxxxxxx>

Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>

-- 
Kees Cook