Re: [PATCH v22 10/24] x86/sgx: Add sgx_einit() for wrapping ENCLS[EINIT]

[Borislav Petkov]

On Tue, Sep 03, 2019 at 05:26:41PM +0300, Jarkko Sakkinen wrote:
> From: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> 
> Enclaves are SGX hosted measured and signed software entities. ENCLS[EINIT]

		SGX-hosted, measured, and ...

> leaf function checks that the enclave has a legit signed measurement and
> transforms the enclave to the state ready for execution. The signed
> measurement is provided by the caller in the form of SIGSTRUCT data
> structure [1].
> 
> Wrap ENCLS[EINIT] into sgx_einit(). Set MSR_IA32_SGXLEPUBKEYHASH* MSRs to
> match the public key contained in the SIGSTRUCT [2]. This sets Linux to
> enforce a policy where the provided public key is as long as the signed
> measurement matches the enclave contents in memory.

That subclause needs to be separated, maybe:

  ... the provided public key is - as long as the signed measurement
  matches the the enclave contents - in memory.

Provided you mean that, of course.

> Add a per-cpu cache to avoid unnecessary reads and write to the MSRs

reads and writes?

> as they are expensive operations.
> 
> [1] Intel SDM: 37.1.3 ENCLAVE SIGNATURE STRUCTURE (SIGSTRUCT)
> [2] Intel SDM: 38.1.4 Intel SGX Launch Control Configuration
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> Co-developed-by: Jarkko Sakkinen <jarkko.sakkinen@xxxxxxxxxxxxxxx>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@xxxxxxxxxxxxxxx>
> ---
>  arch/x86/kernel/cpu/sgx/main.c | 51 ++++++++++++++++++++++++++++++++++
>  arch/x86/kernel/cpu/sgx/sgx.h  |  2 ++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index 6b4727df72ca..d3ed742e90fe 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -17,6 +17,9 @@ EXPORT_SYMBOL_GPL(sgx_epc_sections);
>  
>  int sgx_nr_epc_sections;
>  
> +/* A per-cpu cache for the last known values of IA32_SGXLEPUBKEYHASHx MSRs. */
> +static DEFINE_PER_CPU(u64 [4], sgx_lepubkeyhash_cache);
> +
>  static struct sgx_epc_page *sgx_section_get_page(
>  	struct sgx_epc_section *section)
>  {
> @@ -106,6 +109,54 @@ void sgx_free_page(struct sgx_epc_page *page)
>  }
>  EXPORT_SYMBOL_GPL(sgx_free_page);
>  
> +static void sgx_update_lepubkeyhash_msrs(u64 *lepubkeyhash, bool enforce)
> +{
> +	u64 *cache;
> +	int i;
> +
> +	cache = per_cpu(sgx_lepubkeyhash_cache, smp_processor_id());
> +	for (i = 0; i < 4; i++) {
> +		if (enforce || (lepubkeyhash[i] != cache[i])) {
> +			wrmsrl(MSR_IA32_SGXLEPUBKEYHASH0 + i, lepubkeyhash[i]);
> +			cache[i] = lepubkeyhash[i];
> +		}
> +	}
> +}
> +
> +/**
> + * sgx_einit - initialize an enclave
> + * @sigstruct:		a pointer a SIGSTRUCT
> + * @token:		a pointer an EINITTOKEN (optional)
> + * @secs:		a pointer a SECS

That's a strange formulation "a pointer a/an" ? "to" missing?

> + * @lepubkeyhash:	the desired value for IA32_SGXLEPUBKEYHASHx MSRs
> + *
> + * Execute ENCLS[EINIT], writing the IA32_SGXLEPUBKEYHASHx MSRs according
> + * to @lepubkeyhash (if possible and necessary).
> + *
> + * Return:
> + *   0 on success,
> + *   -errno or SGX error on failure
> + */
> +int sgx_einit(struct sgx_sigstruct *sigstruct, struct sgx_einittoken *token,
> +	      struct sgx_epc_page *secs, u64 *lepubkeyhash)
> +{
> +	int ret;
> +
> +	if (!boot_cpu_has(X86_FEATURE_SGX_LC))
> +		return __einit(sigstruct, token, sgx_epc_addr(secs));
> +
> +	preempt_disable();
> +	sgx_update_lepubkeyhash_msrs(lepubkeyhash, false);
> +	ret = __einit(sigstruct, token, sgx_epc_addr(secs));
> +	if (ret == SGX_INVALID_EINITTOKEN) {
> +		sgx_update_lepubkeyhash_msrs(lepubkeyhash, true);
> +		ret = __einit(sigstruct, token, sgx_epc_addr(secs));
> +	}
> +	preempt_enable();
> +	return ret;
> +}
> +EXPORT_SYMBOL(sgx_einit);

I was about to ask why isn't this export _GPL() but it goes away in a
later patch.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette