Add an ioctl that performs ENCLS[EINIT], which locks down the measurement
and initializes the enclave for entrance. After this, new pages can no
longer be added.
Acked-by: Jethro Beekman <[email protected]>
Tested-by: Jethro Beekman <[email protected]>
Tested-by: Haitao Huang <[email protected]>
Tested-by: Chunyang Hui <[email protected]>
Tested-by: Jordan Hand <[email protected]>
Tested-by: Nathaniel McCallum <[email protected]>
Tested-by: Seth Moore <[email protected]>
Tested-by: Darren Kenny <[email protected]>
Reviewed-by: Darren Kenny <[email protected]>
Co-developed-by: Sean Christopherson <[email protected]>
Signed-off-by: Sean Christopherson <[email protected]>
Co-developed-by: Suresh Siddha <[email protected]>
Signed-off-by: Suresh Siddha <[email protected]>
Signed-off-by: Jarkko Sakkinen <[email protected]>
---
arch/x86/include/uapi/asm/sgx.h | 11 ++
arch/x86/kernel/cpu/sgx/encl.h | 2 +
arch/x86/kernel/cpu/sgx/ioctl.c | 195 ++++++++++++++++++++++++++++++++
3 files changed, 208 insertions(+)
diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
index c42a2ad3ca0b..7729730d8580 100644
--- a/arch/x86/include/uapi/asm/sgx.h
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -23,6 +23,8 @@ enum sgx_page_flags {
_IOW(SGX_MAGIC, 0x00, struct sgx_enclave_create)
#define SGX_IOC_ENCLAVE_ADD_PAGES \
_IOWR(SGX_MAGIC, 0x01, struct sgx_enclave_add_pages)
+#define SGX_IOC_ENCLAVE_INIT \
+ _IOW(SGX_MAGIC, 0x02, struct sgx_enclave_init)
/**
* struct sgx_enclave_create - parameter structure for the
@@ -50,4 +52,13 @@ struct sgx_enclave_add_pages {
__u64 flags;
};
+/**
+ * struct sgx_enclave_init - parameter structure for the
+ * %SGX_IOC_ENCLAVE_INIT ioctl
+ * @sigstruct: address for the SIGSTRUCT data
+ */
+struct sgx_enclave_init {
+ __u64 sigstruct;
+};
+
#endif /* _UAPI_ASM_X86_SGX_H */
diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h
index 8ff445476657..0448d22d3010 100644
--- a/arch/x86/kernel/cpu/sgx/encl.h
+++ b/arch/x86/kernel/cpu/sgx/encl.h
@@ -70,6 +70,8 @@ struct sgx_encl {
struct xarray page_array;
struct sgx_encl_page secs;
cpumask_t cpumask;
+ unsigned long attributes;
+ unsigned long attributes_mask;
};
extern const struct vm_operations_struct sgx_vm_ops;
diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
index 202680a06c17..de2ed4f35ffb 100644
--- a/arch/x86/kernel/cpu/sgx/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -16,6 +16,9 @@
#include "encl.h"
#include "encls.h"
+/* A per-cpu cache for the last known values of IA32_SGXLEPUBKEYHASHx MSRs. */
+static DEFINE_PER_CPU(u64 [4], sgx_lepubkeyhash_cache);
+
static u32 sgx_calc_ssa_frame_size(u32 miscselect, u64 xfrm)
{
u32 size_max = PAGE_SIZE;
@@ -127,6 +130,9 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
encl->base = secs->base;
encl->size = secs->size;
encl->ssaframesize = secs->ssa_frame_size;
+ encl->attributes = secs->attributes;
+ encl->attributes_mask = SGX_ATTR_DEBUG | SGX_ATTR_MODE64BIT |
+ SGX_ATTR_KSS;
/*
* Set SGX_ENCL_CREATED only after the enclave is fully prepped. This
@@ -482,6 +488,192 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg)
return c;
}
+static int __sgx_get_key_hash(struct crypto_shash *tfm, const void *modulus,
+ void *hash)
+{
+ SHASH_DESC_ON_STACK(shash, tfm);
+
+ shash->tfm = tfm;
+
+ return crypto_shash_digest(shash, modulus, SGX_MODULUS_SIZE, hash);
+}
+
+static int sgx_get_key_hash(const void *modulus, void *hash)
+{
+ struct crypto_shash *tfm;
+ int ret;
+
+ tfm = crypto_alloc_shash("sha256", 0, CRYPTO_ALG_ASYNC);
+ if (IS_ERR(tfm))
+ return PTR_ERR(tfm);
+
+ ret = __sgx_get_key_hash(tfm, modulus, hash);
+
+ crypto_free_shash(tfm);
+ return ret;
+}
+
+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];
+ }
+ }
+}
+
+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);
+ ret = __einit(sigstruct, token, sgx_get_epc_addr(secs));
+ if (ret == SGX_INVALID_EINITTOKEN) {
+ sgx_update_lepubkeyhash_msrs(lepubkeyhash, true);
+ 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.
+ */
+ 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
+ * @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)
+{
+ struct sgx_sigstruct *sigstruct;
+ struct sgx_enclave_init einit;
+ struct page *initp_page;
+ void *token;
+ int ret;
+
+ if ((atomic_read(&encl->flags) & SGX_ENCL_INITIALIZED) ||
+ !(atomic_read(&encl->flags) & SGX_ENCL_CREATED))
+ return -EINVAL;
+
+ if (copy_from_user(&einit, arg, sizeof(einit)))
+ return -EFAULT;
+
+ initp_page = alloc_page(GFP_KERNEL);
+ if (!initp_page)
+ return -ENOMEM;
+
+ sigstruct = kmap(initp_page);
+ token = (void *)((unsigned long)sigstruct + PAGE_SIZE / 2);
+ memset(token, 0, SGX_LAUNCH_TOKEN_SIZE);
+
+ if (copy_from_user(sigstruct, (void __user *)einit.sigstruct,
+ sizeof(*sigstruct))) {
+ ret = -EFAULT;
+ goto out;
+ }
+
+ /*
+ * A legacy field used with Intel signed enclaves. These used to mean
+ * regular and architectural enclaves. The CPU only accepts these values
+ * but they do not have any other meaning.
+ *
+ * Thus, reject any other values.
+ */
+ if (sigstruct->header.vendor != 0x0000 &&
+ sigstruct->header.vendor != 0x8086) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ ret = sgx_encl_init(encl, sigstruct, token);
+
+out:
+ kunmap(initp_page);
+ __free_page(initp_page);
+ return ret;
+}
+
+
long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
{
struct sgx_encl *encl = filep->private_data;
@@ -503,6 +695,9 @@ long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
case SGX_IOC_ENCLAVE_ADD_PAGES:
ret = sgx_ioc_enclave_add_pages(encl, (void __user *)arg);
break;
+ case SGX_IOC_ENCLAVE_INIT:
+ ret = sgx_ioc_enclave_init(encl, (void __user *)arg);
+ break;
default:
ret = -ENOIOCTLCMD;
break;
--
2.25.1
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
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;
> > +}
On Mon, Sep 21, 2020 at 11:10:21AM -0700, Sean Christopherson wrote:
> 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.
Lemme try to understand this: the system could suspend/resume right
here:
sgx_update_lepubkeyhash_msrs(lepubkeyhash, false);
<--- suspend/resume
ret = __einit(sigstruct, token, sgx_get_epc_addr(secs));
and thus the MSRs would have the default key so you'd need the second
__einit() call?
But what happens if the system suspends before the second __einit()
call?
Why don't you simply drop that @enforce param and let the caller handle
any retries?
Or is the scenario something different?
Or you could perhaps disable suspend/resume around it, maybe something
like lock_system_sleep() or so, from a quick grep...
> Yes, this need a big fat comment.
Oh yeah.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Mon, Sep 21, 2020 at 11:10:21AM -0700, Sean Christopherson wrote:
> 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.
Thanks, please provide one :-)
/Jarkko
On Mon, Sep 21, 2020 at 12:17:00PM -0700, Sean Christopherson wrote:
> That was effectively my original suggestion as well, check for a stale cache
> and retry indefinitely. I capitulated because it did feel like I was being
> overly paranoid. I'm obviously ok going the retry indefinitely route :-).
>
> https://lkml.kernel.org/r/[email protected]
Right, so if EINIT is so expensive, why does it matter how many cyccles
WRMSR has? I.e., you don't really need to cache - you simply write the 4
MSRs and you're done. Simple.
As to "indefinitely" - caller can increment a counter which counts
how many times it returned SGX_INVALID_EINITTOKEN. I guess when it
reaches some too high number which should not be reached during normal
usage patterns, you can give up and issue a message to say that counter
reached max retries or so but other than that, you should be ok. That
thing is running interruptible in a loop anyway...
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue, Sep 22, 2020 at 10:29:18AM +0200, Borislav Petkov wrote:
> On Mon, Sep 21, 2020 at 12:17:00PM -0700, Sean Christopherson wrote:
> > That was effectively my original suggestion as well, check for a stale cache
> > and retry indefinitely. I capitulated because it did feel like I was being
> > overly paranoid. I'm obviously ok going the retry indefinitely route :-).
> >
> > https://lkml.kernel.org/r/[email protected]
>
> Right, so if EINIT is so expensive, why does it matter how many cyccles
> WRMSR has? I.e., you don't really need to cache - you simply write the 4
> MSRs and you're done. Simple.
>
> As to "indefinitely" - caller can increment a counter which counts
> how many times it returned SGX_INVALID_EINITTOKEN. I guess when it
> reaches some too high number which should not be reached during normal
> usage patterns, you can give up and issue a message to say that counter
> reached max retries or so but other than that, you should be ok. That
> thing is running interruptible in a loop anyway...
The way I see it after reading the thread is:
1. Start with simpler always-update-MSRs in this patch set.
2. If this ever causes a bottleneck, then we will fix it.
> Thx.
>
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
/Jarkko
On 2020-09-22 10:29, Borislav Petkov wrote:
> On Mon, Sep 21, 2020 at 12:17:00PM -0700, Sean Christopherson wrote:
>> That was effectively my original suggestion as well, check for a stale cache
>> and retry indefinitely. I capitulated because it did feel like I was being
>> overly paranoid. I'm obviously ok going the retry indefinitely route :-).
>>
>> https://lkml.kernel.org/r/[email protected]
>
> Right, so if EINIT is so expensive, why does it matter how many cyccles
> WRMSR has? I.e., you don't really need to cache - you simply write the 4
> MSRs and you're done. Simple.
>
> As to "indefinitely" - caller can increment a counter which counts
> how many times it returned SGX_INVALID_EINITTOKEN. I guess when it
> reaches some too high number which should not be reached during normal
> usage patterns, you can give up and issue a message to say that counter
> reached max retries or so but other than that, you should be ok. That
> thing is running interruptible in a loop anyway...
I don't see why you'd need to retry indefinitely. Yes the MSRs may not match the cached value for “reasons”, but if after you've written them once it still doesn't work, clearly either 1) an “unhelpful” VMM is actively messing with the MSRs which I'd say is at best a VMM bug or 2) there was an EPC reset and your enclave is now invalid anyway, so no need to EINIT.
--
Jethro Beekman | Fortanix
On Tue, Sep 22, 2020 at 02:56:19PM +0200, Jethro Beekman wrote:
> I don't see why you'd need to retry indefinitely. Yes the MSRs may not
> match the cached value for “reasons”, but if after you've written
> them once it still doesn't work, clearly either 1) an “unhelpful”
> VMM is actively messing with the MSRs which I'd say is at best a VMM
> bug or 2) there was an EPC reset and your enclave is now invalid
> anyway, so no need to EINIT.
/me likes that even more.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue, Sep 22, 2020 at 02:56:19PM +0200, Jethro Beekman wrote:
> On 2020-09-22 10:29, Borislav Petkov wrote:
> > On Mon, Sep 21, 2020 at 12:17:00PM -0700, Sean Christopherson wrote:
> >> That was effectively my original suggestion as well, check for a stale cache
> >> and retry indefinitely. I capitulated because it did feel like I was being
> >> overly paranoid. I'm obviously ok going the retry indefinitely route :-).
> >>
> >> https://lkml.kernel.org/r/[email protected]
> >
> > Right, so if EINIT is so expensive, why does it matter how many cyccles
> > WRMSR has? I.e., you don't really need to cache - you simply write the 4
> > MSRs and you're done. Simple.
Hmm, true. The 1200+ cycles to write the hash MSRs (they're 3x slower than
other MSRs) seems scary, but compared to the 60k cycles it really doesn't
matter.
> > As to "indefinitely" - caller can increment a counter which counts
> > how many times it returned SGX_INVALID_EINITTOKEN. I guess when it
> > reaches some too high number which should not be reached during normal
> > usage patterns, you can give up and issue a message to say that counter
> > reached max retries or so but other than that, you should be ok. That
> > thing is running interruptible in a loop anyway...
>
> I don't see why you'd need to retry indefinitely. Yes the MSRs may not match
> the cached value for “reasons”, but if after you've written them once it
> still doesn't work, clearly either 1) an “unhelpful” VMM is actively messing
> with the MSRs which I'd say is at best a VMM bug or 2) there was an EPC reset
> and your enclave is now invalid anyway, so no need to EINIT.
Ah, also true, I overlooked that an MSR reset would also kill the enclave.
So yeah, this can be simplified to:
if (SGX_LC) {
for (i = 0; i < 4; i++)
wrmsrl(...);
}
return __einit(...);
On Tue, Sep 22, 2020 at 04:29:09PM +0200, Borislav Petkov wrote:
> On Tue, Sep 22, 2020 at 02:56:19PM +0200, Jethro Beekman wrote:
> > I don't see why you'd need to retry indefinitely. Yes the MSRs may not
> > match the cached value for “reasons”, but if after you've written
> > them once it still doesn't work, clearly either 1) an “unhelpful”
> > VMM is actively messing with the MSRs which I'd say is at best a VMM
> > bug or 2) there was an EPC reset and your enclave is now invalid
> > anyway, so no need to EINIT.
>
> /me likes that even more.
OK, so I did not follow this particular discussion in high detail, so
as a sanity check I'll preview my changes.
I'd refine sgx_update_lepubkeyhash_msrs() to:
static void sgx_update_lepubkeyhash_msrs(u64 *lepubkeyhash)
{
u64 *cache;
int i;
preempt_disable();
cache = per_cpu(sgx_lepubkeyhash_cache, smp_processor_id());
for (i = 0; i < 4; i++) {
if (lepubkeyhash[i] != cache[i]) {
wrmsrl(MSR_IA32_SGXLEPUBKEYHASH0 + i, lepubkeyhash[i]);
cache[i] = lepubkeyhash[i];
}
}
preempt_enable();
}
I'd drop sgx_einit() completely.
Finally, in sgx_encl_init() I would call sgx_update_lepubkeyhash_msrs()
directly:
/* ... */
sgx_update_lepubkeyhash_msrs(mrsigner);
ret = __einit(sigstruct, token, sgx_get_epc_addr(secs));
/* ... */
These staments would replace the call to sgx_einit().
Do I have the correct understanding?
/Jarkko
On Wed, Sep 23, 2020 at 05:47:07PM +0300, Jarkko Sakkinen wrote:
> OK, so I did not follow this particular discussion in high detail,
What do you mean you did not follow it? It is not a huge subthread in
your mbox.
> so as a sanity check I'll preview my changes.
Sorry, but you'll have to read threads properly like everyone else.
In any case, my preference would be make it the simplest possible: no
cache and try EINIT only once.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, Sep 23, 2020 at 05:55:11PM +0200, Borislav Petkov wrote:
> On Wed, Sep 23, 2020 at 05:47:07PM +0300, Jarkko Sakkinen wrote:
> > OK, so I did not follow this particular discussion in high detail,
>
> What do you mean you did not follow it? It is not a huge subthread in
> your mbox.
I focused on other subthreads but I've read it now through. It has been
waiting in the queue. Can't do everything simultaneously. I did skim
each message as they came for this one though.
> > so as a sanity check I'll preview my changes.
>
> Sorry, but you'll have to read threads properly like everyone else.
>
> In any case, my preference would be make it the simplest possible: no
> cache and try EINIT only once.
The main reason skimming was that I was thinking that perhaps Sean might
send a patch for this one and I would then read the thread again in
better detail and check that the patch matches the conclusions, as he
has been more active on this one.
Anyway, now that I've read it through in detail, I did just change the
init simply as:
secs_addr = sgx_get_epc_addr(encl->secs.epc_page);
preempt_disable();
ret = __einit(sigstruct, token, secs_addr);
preempt_enable();
I use a local for address to make the code more readable instead of
calling sgx_get_epc_addr() inside __einit().
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
/Jarkko