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

[Sean Christopherson]

On Mon, Mar 18, 2019 at 11:03:44PM +0800, Yang Weijiang 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
                                                  ^^^^
Please spell out "table" in full, here and below.

> 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:

This contradicts the above statement that "VMX saves/restores ...
IA32_S_CET, SSP and IA32_INTR_SSP_TABL_ADDR MSR" unconditionally.

> 
> - HOST_S_CET: Host supervisor mode IA32_S_CET MSR is loaded
>                    from this field.

Maybe provide a brief introduction on the MSRs instead of mixing that
in with the VMCS field definition?  "Host supervisor mode IA32_S_CET"
makes it sound as if the MSR only exists at cpl=0 or something.

> 
> - HOST_SSP : Host SSP is loaded from this field.
> 
> - HOST_INTR_SSP_TABLE : 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_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_TABLE : Guest IA32_INTR_SSP_TABL_ADDR
                                               ^^^^
>                              MSR is loaded from this field.

You can probably drop the per-field descriptions, the intro regarding
VM_EXIT_LOAD_{HOST,GUEST}_CET_STATE should make it obvious to readers
that these fields contain host/guest values, and the names line up with
the MSRs (assuming you add a brief intro on the MSRs).  E.g.:

If VM_EXIT_LOAD_HOST_CET_STATE = 1, the host's CET MSRs are restored
from the following VMCS fields at VM-Exit:

 - HOST_S_CET
 - HOST_SSP
 - HOST_INTR_SSP_TABLE

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

Assuming you both worked on these patches, a Co-developed-by: tag is
needed for Zhang Yi.  The current docs on Co-developed-by: are a bit
sparse, linux-next has a patch with more information on its usage[1].

https://kernel.googlesource.com/pub/scm/linux/kernel/git/next/linux-next/+/24a2bb90741bf86ddcf6558be419e182ff44fc95

> 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..fd70c4577c5a 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_S_CET                     = 0x00006828,
> +	GUEST_SSP                       = 0x0000682a,
> +	GUEST_INTR_SSP_TABLE            = 0x0000682c,
>  	HOST_CR0                        = 0x00006c00,
>  	HOST_CR3                        = 0x00006c02,
>  	HOST_CR4                        = 0x00006c04,
> @@ -337,6 +342,9 @@ enum vmcs_field {
>  	HOST_IA32_SYSENTER_EIP          = 0x00006c12,
>  	HOST_RSP                        = 0x00006c14,
>  	HOST_RIP                        = 0x00006c16,
> +	HOST_S_CET                      = 0x00006c18,
> +	HOST_SSP                        = 0x00006c1a,
> +	HOST_INTR_SSP_TABLE             = 0x00006c1c
>  };
>  
>  /*
> -- 
> 2.17.1
>