linux-kernel - Re: [PATCH v22 10/24] x86/sgx: Add sgx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 8 Oct 2019 19:30:35 +0200
From:   Borislav Petkov <bp@...en8.de>
To:     Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org,
        linux-sgx@...r.kernel.org, akpm@...ux-foundation.org,
        dave.hansen@...el.com, sean.j.christopherson@...el.com,
        nhorman@...hat.com, npmccallum@...hat.com, serge.ayoun@...el.com,
        shay.katz-zamir@...el.com, haitao.huang@...el.com,
        andriy.shevchenko@...ux.intel.com, tglx@...utronix.de,
        kai.svahn@...el.com, josh@...htriplett.org, luto@...nel.org,
        kai.huang@...el.com, rientjes@...gle.com, cedric.xing@...el.com
Subject: Re: [PATCH v22 10/24] x86/sgx: Add sgx_einit() for wrapping
 ENCLS[EINIT]

On Tue, Sep 03, 2019 at 05:26:41PM +0300, Jarkko Sakkinen wrote:
> From: Sean Christopherson <sean.j.christopherson@...el.com>
> 
> 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@...el.com>
> Co-developed-by: Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
> ---
>  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