Re: [PATCH 08/10] kvm: vmx: add guest's IA32_SGXLEPUBKEYHASHn runtime switch support

["Huang, Kai" <kai.huang@linux.intel.com>]

Hi Paolo/Radim,

I'd like to start a discussion regarding to IA32_SGXLEPUBKEYHASHn 
handling here. I also copied SGX driver mailing list (which looks like I 
should do when sending out this series, sorry) and Sean, Haim and Haitao 
from Intel to have a better discussion.

Basically IA32_SGXLEPUBKEYHASHn (or more generally speaking, SGX Launch 
Control) allows us to run different Launch Enclave (LE) signed with 
different RSA keys. Only when the value of IA32_SGXLEPUBKEYHASHn matches 
the key used to sign the LE, the LE can be initialized, specifically, by 
EINIT, successfully. So before calling EINIT for LE, we have to make 
sure IA32_SGXLEPUBKEYHASHn contain the matched value. One fact is only 
EINIT uses IA32_SGXLEPUBKEYHASHn, and after EINIT, other ENCLS/ENCLU 
(ex, EGETKEY) runs correctly even the MSRs are changed to other values.

To support KVM guests to run their own LEs inside guests, KVM traps 
IA32_SGXLEPUBKEYHASHn MSR write and keep the value to vcpu internally, 
and KVM needs to write the cached value to real MSRs before guest runs 
EINIT. The problem at host side, we also run LE, probably multiple LEs 
(it seems currently SGX driver plans to run single in-kernel LE but I am 
not familiar with details, and IMO we should not assume host will only 
run one LE), therefore if KVM changes the physical MSRs for guest,
host may not be able to run LE as it may not re-write the right MSRs 
back. There are two approaches to make host and KVM guests work together:

1. Anyone who wants to run LE is responsible for writing the correct 
value to IA32_SGXLEPUBKEYHASHn.

My current patch is based on this assumption. For KVM guest, naturally, 
we will write the cached value to real MSRs when vcpu is scheduled in. 
For host, SGX driver should write its own value to MSRs when it performs 
EINIT for LE.

One argument against this approach is KVM guest should never have impact 
on host side, meaning host should not be aware of such MSR change, in 
which case, if host do some performance optimization thing that won't 
update MSRs actively, when host run EINIT, the physical MSRs may contain 
incorrect value. Instead, KVM should be responsible for restoring the 
original MSRs, which brings us to approach 2 below.

2. KVM should restore MSRs after changing for guest.

To do this, the simplest way for KVM is: 1) to save the original 
physical MSRs and update to guest's MSRs before VMENTRY; 2) in VMEXIT 
rewrite the original value to physical MSRs.

To me this approach is also arguable, as KVM guest is actually just a 
normal process (OK, maybe not that normal), and KVM guest should be 
treated as the same as other processes which runs LE, which means 
approach 1 is also reasonable.

And approach 2 will have more performance impact than approach 1 for 
KVM, as it read/write IA32_SGXLEPUBKEYHASHn during each VMEXIT/VMENTRY, 
while approach 1 only write MSRs when vcpu is scheduled in, which is 
less frequent.

I'd like to hear all your comments and hopefully we can have some 
agreement on this.

Another thing is, not quite related to selecting which approach above, 
and either we choose approach 1 or approach 2, KVM still suffers the 
performance loss of writing (and/or reading) to IA32_SGXLEPUBKEYHASHn 
MSRs, either when vcpu scheduled in or during each VMEXIT/VMENTRY. Given 
the fact that the IA32_SGXLEPUBKEYHASHn will only be used by EINIT, We 
can actually do some optimization by trapping EINIT from guest and only 
update MSRs in EINIT VMEXIT. This works for approach 1, but for approach 
2 we have to do some tricky thing during VMEXIT/VMENTRY
to check whether MSRs have been changed by EINIT VMEXIT, and only 
restore the original value if EINIT VMEXIT has happened. Guest's LE 
continues to run even physical MSRs are changed back to original.

But trapping ENCLS requires either 1) KVM to run ENCLS on hebalf of 
guest, in which case we have to reconstruct and remap guest's ENCLS 
parameters and skip the ENCLS for guest; 2) using MTF to let guest to 
run ENCLS again, while still trapping ENCLS. Either case would introduce 
more complicated code and potentially be more buggy, and I don't think 
we should do this to save some time of writing MSRs. If we need to turn 
on ENCLS VMEXIT anyway we can optimize this.

Thank you in advance.

Thanks,
-Kai

On 5/8/2017 5:24 PM, Kai Huang wrote:
> If SGX runtime launch control is enabled on host (IA32_FEATURE_CONTROL[17]
> is set), KVM can support running multiple guests with each running LE signed
> with different RSA pubkey. KVM traps IA32_SGXLEPUBKEYHASHn MSR write from
> and keeps the values to vcpu internally, and when vcpu is scheduled in, KVM
> write those values to real IA32_SGXLEPUBKEYHASHn MSR.
>
> Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
> ---
>  arch/x86/include/asm/msr-index.h |   5 ++
>  arch/x86/kvm/vmx.c               | 123 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 128 insertions(+)
>
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index e3770f570bb9..70482b951b0f 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -417,6 +417,11 @@
>  #define MSR_IA32_TSC_ADJUST             0x0000003b
>  #define MSR_IA32_BNDCFGS		0x00000d90
>
> +#define MSR_IA32_SGXLEPUBKEYHASH0	0x0000008c
> +#define MSR_IA32_SGXLEPUBKEYHASH1	0x0000008d
> +#define MSR_IA32_SGXLEPUBKEYHASH2	0x0000008e
> +#define MSR_IA32_SGXLEPUBKEYHASH3	0x0000008f
> +
>  #define MSR_IA32_XSS			0x00000da0
>
>  #define FEATURE_CONTROL_LOCKED				(1<<0)
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index a16539594a99..c96332b9dd44 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -656,6 +656,9 @@ struct vcpu_vmx {
>  	 */
>  	u64 msr_ia32_feature_control;
>  	u64 msr_ia32_feature_control_valid_bits;
> +
> +	/* SGX Launch Control public key hash */
> +	u64 msr_ia32_sgxlepubkeyhash[4];
>  };
>
>  enum segment_cache_field {
> @@ -2244,6 +2247,70 @@ static void decache_tsc_multiplier(struct vcpu_vmx *vmx)
>  	vmcs_write64(TSC_MULTIPLIER, vmx->current_tsc_ratio);
>  }
>
> +static bool cpu_sgx_lepubkeyhash_writable(void)
> +{
> +	u64 val, sgx_lc_enabled_mask = (FEATURE_CONTROL_LOCKED |
> +			FEATURE_CONTROL_SGX_LAUNCH_CONTROL_ENABLE);
> +
> +	rdmsrl(MSR_IA32_FEATURE_CONTROL, val);
> +
> +	return ((val & sgx_lc_enabled_mask) == sgx_lc_enabled_mask);
> +}
> +
> +static bool vmx_sgx_lc_disabled_in_bios(struct kvm_vcpu *vcpu)
> +{
> +	return (to_vmx(vcpu)->msr_ia32_feature_control & FEATURE_CONTROL_LOCKED)
> +		&& (!(to_vmx(vcpu)->msr_ia32_feature_control &
> +				FEATURE_CONTROL_SGX_LAUNCH_CONTROL_ENABLE));
> +}
> +
> +#define	SGX_INTEL_DEFAULT_LEPUBKEYHASH0		0xa6053e051270b7ac
> +#define	SGX_INTEL_DEFAULT_LEPUBKEYHASH1	        0x6cfbe8ba8b3b413d
> +#define	SGX_INTEL_DEFAULT_LEPUBKEYHASH2		0xc4916d99f2b3735d
> +#define	SGX_INTEL_DEFAULT_LEPUBKEYHASH3		0xd4f8c05909f9bb3b
> +
> +static void vmx_sgx_init_lepubkeyhash(struct kvm_vcpu *vcpu)
> +{
> +	u64 h0, h1, h2, h3;
> +
> +	/*
> +	 * If runtime launch control is enabled (IA32_SGXLEPUBKEYHASHn is
> +	 * writable), we set guest's default value to be Intel's default
> +	 * hash (which is fixed value and can be hard-coded). Otherwise,
> +	 * guest can only use machine's IA32_SGXLEPUBKEYHASHn so set guest's
> +	 * default to that.
> +	 */
> +	if (cpu_sgx_lepubkeyhash_writable()) {
> +		h0 = SGX_INTEL_DEFAULT_LEPUBKEYHASH0;
> +		h1 = SGX_INTEL_DEFAULT_LEPUBKEYHASH1;
> +		h2 = SGX_INTEL_DEFAULT_LEPUBKEYHASH2;
> +		h3 = SGX_INTEL_DEFAULT_LEPUBKEYHASH3;
> +	}
> +	else {
> +		rdmsrl(MSR_IA32_SGXLEPUBKEYHASH0, h0);
> +		rdmsrl(MSR_IA32_SGXLEPUBKEYHASH1, h1);
> +		rdmsrl(MSR_IA32_SGXLEPUBKEYHASH2, h2);
> +		rdmsrl(MSR_IA32_SGXLEPUBKEYHASH3, h3);
> +	}
> +
> +	to_vmx(vcpu)->msr_ia32_sgxlepubkeyhash[0] = h0;
> +	to_vmx(vcpu)->msr_ia32_sgxlepubkeyhash[1] = h1;
> +	to_vmx(vcpu)->msr_ia32_sgxlepubkeyhash[2] = h2;
> +	to_vmx(vcpu)->msr_ia32_sgxlepubkeyhash[3] = h3;
> +}
> +
> +static void vmx_sgx_lepubkeyhash_load(struct kvm_vcpu *vcpu)
> +{
> +	wrmsrl(MSR_IA32_SGXLEPUBKEYHASH0,
> +			to_vmx(vcpu)->msr_ia32_sgxlepubkeyhash[0]);
> +	wrmsrl(MSR_IA32_SGXLEPUBKEYHASH1,
> +			to_vmx(vcpu)->msr_ia32_sgxlepubkeyhash[1]);
> +	wrmsrl(MSR_IA32_SGXLEPUBKEYHASH2,
> +			to_vmx(vcpu)->msr_ia32_sgxlepubkeyhash[2]);
> +	wrmsrl(MSR_IA32_SGXLEPUBKEYHASH3,
> +			to_vmx(vcpu)->msr_ia32_sgxlepubkeyhash[3]);
> +}
> +
>  /*
>   * Switches to specified vcpu, until a matching vcpu_put(), but assumes
>   * vcpu mutex is already taken.
> @@ -2316,6 +2383,14 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>
>  	vmx_vcpu_pi_load(vcpu, cpu);
>  	vmx->host_pkru = read_pkru();
> +
> +	/*
> +	 * Load guset's SGX LE pubkey hash if runtime launch control is
> +	 * enabled.
> +	 */
> +	if (guest_cpuid_has_sgx_launch_control(vcpu) &&
> +			cpu_sgx_lepubkeyhash_writable())
> +		vmx_sgx_lepubkeyhash_load(vcpu);
>  }
>
>  static void vmx_vcpu_pi_put(struct kvm_vcpu *vcpu)
> @@ -3225,6 +3300,19 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  	case MSR_IA32_FEATURE_CONTROL:
>  		msr_info->data = to_vmx(vcpu)->msr_ia32_feature_control;
>  		break;
> +	case MSR_IA32_SGXLEPUBKEYHASH0 ... MSR_IA32_SGXLEPUBKEYHASH3:
> +		/*
> +		 * SDM 35.1 Model-Specific Registers, table 35-2.
> +		 * Read permitted if CPUID.0x12.0:EAX[0] = 1. (We have
> +		 * guaranteed this will be true if guest_cpuid_has_sgx
> +		 * is true.)
> +		 */
> +		if (!guest_cpuid_has_sgx(vcpu))
> +			return 1;
> +		msr_info->data =
> +			to_vmx(vcpu)->msr_ia32_sgxlepubkeyhash[msr_info->index -
> +			MSR_IA32_SGXLEPUBKEYHASH0];
> +		break;
>  	case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMFUNC:
>  		if (!nested_vmx_allowed(vcpu))
>  			return 1;
> @@ -3344,6 +3432,37 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  		 * SGX has been enabled in BIOS before using SGX.
>  		 */
>  		break;
> +	case MSR_IA32_SGXLEPUBKEYHASH0 ... MSR_IA32_SGXLEPUBKEYHASH3:
> +		/*
> +		 * SDM 35.1 Model-Specific Registers, table 35-2.
> +		 * - If CPUID.0x7.0:ECX[30] = 1, FEATURE_CONTROL[17] is
> +		 * available.
> +		 * - Write permitted if CPUID.0x12.0:EAX[0] = 1 &&
> +		 * FEATURE_CONTROL[17] = 1 && FEATURE_CONTROL[0] = 1.
> +		 */
> +		if (!guest_cpuid_has_sgx(vcpu) ||
> +				!guest_cpuid_has_sgx_launch_control(vcpu))
> +			return 1;
> +		/*
> +		 * Don't let userspace set guest's IA32_SGXLEPUBKEYHASHn,
> +		 * if machine's IA32_SGXLEPUBKEYHASHn cannot be changed at
> +		 * runtime. Note to_vmx(vcpu)->msr_ia32_sgxlepubkeyhash are
> +		 * set to default in vmx_create_vcpu therefore guest is able
> +		 * to get the machine's IA32_SGXLEPUBKEYHASHn by rdmsr in
> +		 * guest.
> +		 */
> +		if (!cpu_sgx_lepubkeyhash_writable())
> +			return 1;
> +		/*
> +		 * If guest's FEATURE_CONTROL[17] is not set, guest's
> +		 * IA32_SGXLEPUBKEYHASHn are not writeable from guest.
> +		 */
> +		if (!vmx_sgx_lc_disabled_in_bios(vcpu) &&
> +				!msr_info->host_initiated)
> +			return 1;
> +		to_vmx(vcpu)->msr_ia32_sgxlepubkeyhash[msr_index -
> +			MSR_IA32_SGXLEPUBKEYHASH0] = data;
> +		break;
>  	case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMFUNC:
>  		if (!msr_info->host_initiated)
>  			return 1; /* they are read-only */
> @@ -9305,6 +9424,10 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
>  		vmx->nested.vpid02 = allocate_vpid();
>  	}
>
> +	/* Set vcpu's default IA32_SGXLEPUBKEYHASHn */
> +	if (enable_sgx && boot_cpu_has(X86_FEATURE_SGX_LAUNCH_CONTROL))
> +		vmx_sgx_init_lepubkeyhash(&vmx->vcpu);
> +
>  	vmx->nested.posted_intr_nv = -1;
>  	vmx->nested.current_vmptr = -1ull;
>  	vmx->nested.current_vmcs12 = NULL;
>