Re: [PATCH v38 14/24] x86/sgx: Add SGX_IOC_ENCLAVE_INIT

[Sean Christopherson]

On Mon, Sep 21, 2020 at 07:35:14PM +0200, Borislav Petkov wrote:
> On Tue, Sep 15, 2020 at 02:28:32PM +0300, Jarkko Sakkinen wrote:
> > +static int sgx_einit(struct sgx_sigstruct *sigstruct, void *token,
> > +		     struct sgx_epc_page *secs, u64 *lepubkeyhash)
> > +{
> > +	int ret;
> > +
> > +	preempt_disable();
> > +	sgx_update_lepubkeyhash_msrs(lepubkeyhash, false);
> 
> So this will update the cached copies *and* the MSRs itself if what's
> cached is stale...
> 
> > +	ret = __einit(sigstruct, token, sgx_get_epc_addr(secs));
> > +	if (ret == SGX_INVALID_EINITTOKEN) {
> 
> ... so why would it return this error here?
> 
> Definition of this error says:
> 
>  * %SGX_INVALID_EINITTOKEN:     EINITTOKEN is invalid and enclave signer's
>  *                              public key does not match IA32_SGXLEPUBKEYHASH.
> 
> when you just updated them?!
> 
> > +		sgx_update_lepubkeyhash_msrs(lepubkeyhash, true);
> 
> So why force a second time?

The LE pubkey hash MSRs are special snowflakes.  They get reset to Intel's
default key on any loss of EPC, e.g. if the system does a suspend/resume
cycle.  The approach we took (obviously) is to assume the kernel's cache can
be stale at any given time.  The alternative would be to try and track loss
of EPC conditions and emulate the reset, but that's a bit dicey on bare
metal as any missed case would hose SGX, and in a VM it's theoretically
impossible to handle as a particularly unhelpful VMM could emulate loss of
EPC at will.

Yes, this need a big fat comment.

> > +		ret = __einit(sigstruct, token, sgx_get_epc_addr(secs));
> > +	}
> > +	preempt_enable();
> > +	return ret;
> > +}