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?
> + ret = __einit(sigstruct, token, sgx_get_epc_addr(secs));
> + }
> + preempt_enable();
> + return ret;
> +}
> +
> +static int sgx_encl_init(struct sgx_encl *encl, struct sgx_sigstruct *sigstruct,
> + void *token)
> +{
> + u64 mrsigner[4];
> + int ret;
> + int i;
> + int j;
> +
> + /* Deny initializing enclaves with attributes (namely provisioning)
> + * that have not been explicitly allowed.
> + */
Comments style is with the first line empty:
/*
* A sentence ending with a full-stop.
* Another sentence. ...
* More sentences. ...
*/
> + if (encl->attributes & ~encl->attributes_mask)
> + return -EACCES;
> +
> + ret = sgx_get_key_hash(sigstruct->modulus, mrsigner);
> + if (ret)
> + return ret;
> +
> + mutex_lock(&encl->lock);
> +
> + /*
> + * ENCLS[EINIT] is interruptible because it has such a high latency,
> + * e.g. 50k+ cycles on success. If an IRQ/NMI/SMI becomes pending,
> + * EINIT may fail with SGX_UNMASKED_EVENT so that the event can be
> + * serviced.
> + */
> + for (i = 0; i < SGX_EINIT_SLEEP_COUNT; i++) {
> + for (j = 0; j < SGX_EINIT_SPIN_COUNT; j++) {
> + ret = sgx_einit(sigstruct, token, encl->secs.epc_page,
> + mrsigner);
> + if (ret == SGX_UNMASKED_EVENT)
> + continue;
> + else
> + break;
> + }
> +
> + if (ret != SGX_UNMASKED_EVENT)
> + break;
> +
> + msleep_interruptible(SGX_EINIT_SLEEP_TIME);
> +
> + if (signal_pending(current)) {
> + ret = -ERESTARTSYS;
> + goto err_out;
> + }
> + }
> +
> + if (ret & ENCLS_FAULT_FLAG) {
> + if (encls_failed(ret))
> + ENCLS_WARN(ret, "EINIT");
> +
> + sgx_encl_destroy(encl);
> + ret = -EFAULT;
> + } else if (ret) {
> + pr_debug("EINIT returned %d\n", ret);
> + ret = -EPERM;
> + } else {
> + atomic_or(SGX_ENCL_INITIALIZED, &encl->flags);
> + }
> +
> +err_out:
> + mutex_unlock(&encl->lock);
> + return ret;
> +}
> +
> +/**
> + * sgx_ioc_enclave_init - handler for %SGX_IOC_ENCLAVE_INIT
> + *
> + * @filep: open file to /dev/sgx
Aaand again:
"@encl: pointer to an enclave instance (via ioctl() file pointer)"
this is also from a previous review.
> + * @arg: userspace pointer to a struct sgx_enclave_init instance
> + *
> + * Flush any outstanding enqueued EADD operations and perform EINIT. The
> + * Launch Enclave Public Key Hash MSRs are rewritten as necessary to match
> + * the enclave's MRSIGNER, which is caculated from the provided sigstruct.
> + *
> + * Return:
> + * 0 on success,
> + * SGX error code on EINIT failure,
> + * -errno otherwise
> + */
> +static long sgx_ioc_enclave_init(struct sgx_encl *encl, void __user *arg)
> +{
...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
