Re: [PATCH v3 1/8] KVM:VMX: Define CET VMCS fields and bits

[Sean Christopherson]

On Tue, Feb 26, 2019 at 11:31:11AM -0800, Jim Mattson wrote:
> On Mon, Feb 25, 2019 at 10:32 PM Yang Weijiang <weijiang.yang@xxxxxxxxx> wrote:
> >
> > CET - Control-flow Enforcement Technology, it's used to
> > protect against return/jump oriented programming (ROP)
> > attacks. It provides the following capabilities to defend
> > against ROP/JOP style control-flow subversion attacks:
> > - Shadow Stack (SHSTK):
> >                  A second stack for the program that is
> >                  used exclusively for control transfer
> >                  operations.
> > - Indirect Branch Tracking (IBT):
> >                  Free branch protection to defend against
> >                  Jump/Call Oriented Programming.
> >
> > On processors that support CET, VMX saves/restores
> > the states of IA32_S_CET, SSP and IA32_INTR_SSP_TABL_ADDR MSR
> > to the VMCS area for Guest/Host unconditionally.
> >
> > If VM_EXIT_LOAD_HOST_CET_STATE = 1, the host CET MSRs are
> > restored from VMCS host-state area at VM exit as follows:
> >
> > - HOST_IA32_S_CET: Host supervisor mode IA32_S_CET MSR is loaded
> >                    from this field.
> >
> > - HOST_SSP :       Host SSP is loaded from this field.
> >
> > - HOST_INTR_SSP_TABL_ADDR : Host IA32_INTR_SSP_TABL_ADDR
> >                              MSR is loaded from this field.
> >
> > If VM_ENTRY_LOAD_GUEST_CET_STATE = 1, the guest CET MSRs are loaded
> > from VMCS guest-state area at VM entry as follows:
> >
> > - GUEST_IA32_S_CET : Guest supervisor mode IA32_S_CET MSR is loaded
> >                      from this field.
> >
> > - GUEST_SSP :        Guest SSP is loaded from this field.
> >
> > - GUEST_INTR_SSP_TABL_ADDR : Guest IA32_INTR_SSP_TABL_ADDR
> >                              MSR is loaded from this field.
> >
> > Additionally, to context switch guest and host CET states, the VMM
> > uses xsaves/xrstors instructions to save/restore the guest CET states
> > at VM exit/entry. The CET xsave area is within thread_struct.fpu area.
> > If OS execution flow changes during task switch/interrupt/exception etc.,
> > the OS also relies on xsaves/xrstors to switch CET states accordingly.
> >
> > Signed-off-by: Zhang Yi Z <yi.z.zhang@xxxxxxxxxxxxxxx>
> > Signed-off-by: Yang Weijiang <weijiang.yang@xxxxxxxxx>
> > ---
> >  arch/x86/include/asm/vmx.h | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
> > index ade0f153947d..395c1f7e5938 100644
> > --- a/arch/x86/include/asm/vmx.h
> > +++ b/arch/x86/include/asm/vmx.h
> > @@ -98,6 +98,7 @@
> >  #define VM_EXIT_LOAD_IA32_EFER                  0x00200000
> >  #define VM_EXIT_SAVE_VMX_PREEMPTION_TIMER       0x00400000
> >  #define VM_EXIT_CLEAR_BNDCFGS                   0x00800000
> > +#define VM_EXIT_LOAD_HOST_CET_STATE             0x10000000
> >
> >  #define VM_EXIT_ALWAYSON_WITHOUT_TRUE_MSR      0x00036dff
> >
> > @@ -109,6 +110,7 @@
> >  #define VM_ENTRY_LOAD_IA32_PAT                 0x00004000
> >  #define VM_ENTRY_LOAD_IA32_EFER                 0x00008000
> >  #define VM_ENTRY_LOAD_BNDCFGS                   0x00010000
> > +#define VM_ENTRY_LOAD_GUEST_CET_STATE           0x00100000
> >
> >  #define VM_ENTRY_ALWAYSON_WITHOUT_TRUE_MSR     0x000011ff
> >
> > @@ -325,6 +327,9 @@ enum vmcs_field {
> >         GUEST_PENDING_DBG_EXCEPTIONS    = 0x00006822,
> >         GUEST_SYSENTER_ESP              = 0x00006824,
> >         GUEST_SYSENTER_EIP              = 0x00006826,
> > +       GUEST_IA32_S_CET                = 0x00006828,
> > +       GUEST_SSP                       = 0x0000682a,
> > +       GUEST_INTR_SSP_TABL_ADDR        = 0x0000682c,
> 
> Nit: TABL is an unusual abbreviation. Perhaps TBL here and below?

I don't see any reason to abbreviate TABLE, we don't need the two
characters for anything.

> And why did you drop the 'IA32' here, but not in GUEST_IA32_S_CET above?
> (It is true that there seems to be no rhyme or reason to the mnemonics
> chosen here. For example, EFER keeps its IA32, but SYSENTER_EIP
> doesn't. Sigh.)

My vote is to always drop IA32, IMO it's redundant.