[RFC PATCH v2 00/10] Support microcode updates affecting SGX

[RFC PATCH v2 00/10] Support microcode updates affecting SGX

[Cathy Zhang]

v1:
https://lore.kernel.org/all/1742be9e-c18e-28c9-75c8-144bf1f6a311@intel.com/T/

Changes since v1:
 - Remove the sysfs file svnupdate. (Thomas Gleixner, Dave Hansen)
 - Let late microcode load path call ENCLS[EUPDATESVN] procedure
   directly. (Borislav Petkov)
 - Update cover letter by removing saying that "...triggered by
   administrators via sysfs...".
 - Drop the patch for documentation change.

cover letter:

== General Microcode Background ==

Historically, microcode updates are applied by the BIOS or early in
boot. In recent years, several trends have made these old approaches
less palatable.

First, the cadence of microcode updates has increased to deliver
security mitigations. Second, the value of those updates has increased,
meaning that any delay in applying them is unacceptable. Third, users
have become accustomed to approaches like hot patching their kernels
and have a growing aversion to reboots in general.

Users want microcode updates to behave more like a hot patching a
kernel and less like a BIOS update.

== SGX Attestation Background ==

SGX enclaves have an attestation mechanism. An enclave might, for
instance, need to attest to its state before it is given a special
decryption key. Since SGX must trust the CPU microcode, attestation
incorporates the microcode versions of all processors on the system
and is affected by microcode updates. This allows the entity to which
the enclave is attesting to make deployment decisions based on the
microcode version. For example, an enclave might be denied a decryption
key if it runs on a system that has old microcode without a specific
mitigation.

Unfortunately, this attestation metric (called CPUSVN) is only a
snapshot. When the kernel first uses SGX (successfully executes any
ENCLS instruction), SGX inspects all CPUs in the system and incorporates
a record of their microcode versions into CPUSVN. Today, that value is
locked and is not updated until a reboot.

== Problems ==

This means that, although the microcode may be update, enclaves can
never attest to this fact. Enclaves are stuck attesting to the old
version until a reboot.

Old enclaves created before the microcode update are presumed to be
compromised must not be allowed to attest with the new microcode
version.

== Solution ==

EUPDATESVN is a new SGX instruction which allows enclave attestation
to include information about updated microcode without a reboot.

Whenever a microcode update affects SGX, the SGX attestation
architecture assumes that all running enclaves and cryptographic
assets (like internal SGX encryption keys) have been compromised.
To mitigate the impact of this presumed compromise, EUPDATESVN success
requires that all SGX memory to be marked as "unused" and its contents
destroyed. This requirement ensures that no compromised enclave can
survive the EUPDATESVN procedure and provides an opportunity to
generate new cryptographic assets.

This series implements the infrastructure needed to track and tear
down bare-metal enclaves and then run EUPDATESVN, it will be called
by the late microcode load path after the microcode update.

This is a very slow operation. It is, of course, exceedingly disruptive
to enclaves but should be infrequent as microcode updates are released
on the order of every few months. Also, this is not the first piece of
the SGX architecture which will destroy all enclave contents.

A follow-on series will add Virtual EPC (KVM guest) support.

SGX Seamless should handle most SGX flows while doing SVN update, so, this
RFC series is based on SGX EDMM v2 which introduces SGX2 flows.
https://lore.kernel.org/lkml/cover.1644274683.git.reinette.chatre@intel.com/T/

Here is the spec for your reference:
https://cdrdv2.intel.com/v1/dl/getContent/648682?explicitVersion=true

Cathy Zhang (10):
  x86/sgx: Introduce mechanism to prevent new initializations of EPC
    pages
  x86/sgx: Provide VA page non-NULL owner
  x86/sgx: Save enclave pointer for VA page
  x86/sgx: Keep record for SGX VA and Guest page type
  x86/sgx: Save the size of each EPC section
  x86/sgx: Forced EPC page zapping for EUPDATESVN
  x86/sgx: Define error codes for ENCLS[EUPDATESVN]
  x86/sgx: Implement ENCLS[EUPDATESVN]
  x86/cpu: Call ENCLS[EUPDATESVN] procedure in microcode update
  x86/sgx: Call ENCLS[EUPDATESVN] during SGX initialization

 arch/x86/include/asm/microcode.h |   5 +
 arch/x86/include/asm/sgx.h       |  46 +++-
 arch/x86/kernel/cpu/sgx/encl.h   |   3 +-
 arch/x86/kernel/cpu/sgx/encls.h  |  16 ++
 arch/x86/kernel/cpu/sgx/sgx.h    |  23 +-
 arch/x86/kernel/cpu/common.c     |   9 +
 arch/x86/kernel/cpu/sgx/encl.c   |  46 +++-
 arch/x86/kernel/cpu/sgx/ioctl.c  |  53 +++-
 arch/x86/kernel/cpu/sgx/main.c   | 460 ++++++++++++++++++++++++++++++-
 arch/x86/kernel/cpu/sgx/virt.c   |  22 ++
 10 files changed, 658 insertions(+), 25 deletions(-)

-- 
2.17.1

[RFC PATCH v2 01/10] x86/sgx: Introduce mechanism to prevent new initializations of EPC pages

[Cathy Zhang]

== Background ==

EUPDATESVN is a new SGX instruction which allows enclave attestation
to include information about updated microcode without a reboot.

The SGX hardware maintains metadata for each enclave page to help
enforce its security guarantees. This includes things like a record
of the enclave to which the page belongs and the type of the page:
SGX metadata like "VA" or "SECS" pages, or regular enclave pages
like those that store user data.

Before an EUPDATESVN operation can be successful, all SGX memory (aka.
EPC) must be marked as "unused" in the SGX hardware metadata (aka,
EPCM). The SGX microcode now maintains a reference count of pages
which are unused to aid in determining when all pages reach the
"unused" state.

Both bare-metal and KVM guest EPC must be made unused. To increase
the chance of a successful EUPDATESVN, the kernel prevents existing
enclaves from creating new, valid pages and prevents new enclave
creation (creating an enclave involves initializing a "SECS" page).

The entire EUPDATESVN process is very slow since it potentially
affects gigabytes of enclave memory. It can potentially take seconds
or minutes to complete. Userspace may encounter -EBUSY errors during
the update and is expected to retry.

== Patch contents ==

Introduce mechanism to prevent new initializations of EPC pages.

Use a flag to indicate when SGX EPC pages are "locked", which means
it's not allowed to allocate new EPC page for use. Check it in all
paths that can initialize an EPC page. Use SRCU to ensure that the
flag is visible across the system before proceeding with an update.

Add checks to all sites that call SGX instructions that can transition
pages from unused to initialized to ensure that the SRCU lock is held.

Signed-off-by: Cathy Zhang <cathy.zhang@intel.com>
---
 arch/x86/kernel/cpu/sgx/encls.h | 10 +++++++
 arch/x86/kernel/cpu/sgx/sgx.h   |  3 ++
 arch/x86/kernel/cpu/sgx/encl.c  | 25 +++++++++++++++--
 arch/x86/kernel/cpu/sgx/ioctl.c | 50 ++++++++++++++++++++++++++++++++-
 arch/x86/kernel/cpu/sgx/main.c  | 37 ++++++++++++++++++++++++
 arch/x86/kernel/cpu/sgx/virt.c  | 20 +++++++++++++
 6 files changed, 142 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/encls.h b/arch/x86/kernel/cpu/sgx/encls.h
index 99004b02e2ed..3f1797ec2445 100644
--- a/arch/x86/kernel/cpu/sgx/encls.h
+++ b/arch/x86/kernel/cpu/sgx/encls.h
@@ -139,6 +139,8 @@ static inline bool encls_failed(int ret)
 /* Initialize an EPC page into an SGX Enclave Control Structure (SECS) page. */
 static inline int __ecreate(struct sgx_pageinfo *pginfo, void *secs)
 {
+	lockdep_assert_held(&sgx_lock_epc_srcu);
+
 	return __encls_2(ECREATE, pginfo, secs);
 }
 
@@ -154,6 +156,8 @@ static inline int __eextend(void *secs, void *addr)
  */
 static inline int __eadd(struct sgx_pageinfo *pginfo, void *addr)
 {
+	lockdep_assert_held(&sgx_lock_epc_srcu);
+
 	return __encls_2(EADD, pginfo, addr);
 }
 
@@ -191,6 +195,8 @@ static inline int __etrack(void *addr)
 static inline int __eldu(struct sgx_pageinfo *pginfo, void *addr,
 			 void *va)
 {
+	lockdep_assert_held(&sgx_lock_epc_srcu);
+
 	return __encls_ret_3(ELDU, pginfo, addr, va);
 }
 
@@ -205,6 +211,8 @@ static inline int __epa(void *addr)
 {
 	unsigned long rbx = SGX_PAGE_TYPE_VA;
 
+	lockdep_assert_held(&sgx_lock_epc_srcu);
+
 	return __encls_2(EPA, rbx, addr);
 }
 
@@ -230,6 +238,8 @@ static inline int __emodt(struct sgx_secinfo *secinfo, void *addr)
 /* Zero a page of EPC memory and add it to an initialized enclave. */
 static inline int __eaug(struct sgx_pageinfo *pginfo, void *addr)
 {
+	lockdep_assert_held(&sgx_lock_epc_srcu);
+
 	return __encls_2(EAUG, pginfo, addr);
 }
 
diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
index 85cbf103b0dd..dc8cb58100e3 100644
--- a/arch/x86/kernel/cpu/sgx/sgx.h
+++ b/arch/x86/kernel/cpu/sgx/sgx.h
@@ -104,4 +104,7 @@ static inline int __init sgx_vepc_init(void)
 
 void sgx_update_lepubkeyhash(u64 *lepubkeyhash);
 
+extern struct srcu_struct sgx_lock_epc_srcu;
+bool sgx_epc_is_locked(void);
+
 #endif /* _X86_SGX_H */
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index a0255d41e075..d2b428992910 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -264,6 +264,7 @@ static vm_fault_t sgx_vma_fault(struct vm_fault *vmf)
 	unsigned long phys_addr;
 	struct sgx_encl *encl;
 	vm_fault_t ret;
+	int srcu_idx;
 
 	encl = vma->vm_private_data;
 
@@ -275,6 +276,12 @@ static vm_fault_t sgx_vma_fault(struct vm_fault *vmf)
 	if (unlikely(!encl))
 		return VM_FAULT_SIGBUS;
 
+	srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
+	if (sgx_epc_is_locked()) {
+		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
+		return VM_FAULT_SIGBUS;
+	}
+
 	/*
 	 * The page_array keeps track of all enclave pages, whether they
 	 * are swapped out or not. If there is no entry for this page and
@@ -283,14 +290,18 @@ static vm_fault_t sgx_vma_fault(struct vm_fault *vmf)
 	 * enclave that will be checked for right away.
 	 */
 	if (cpu_feature_enabled(X86_FEATURE_SGX2) &&
-	    (!xa_load(&encl->page_array, PFN_DOWN(addr))))
-		return sgx_encl_eaug_page(vma, encl, addr);
+	    (!xa_load(&encl->page_array, PFN_DOWN(addr)))) {
+		ret = sgx_encl_eaug_page(vma, encl, addr);
+		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
+		return ret;
+	}
 
 	mutex_lock(&encl->lock);
 
 	entry = sgx_encl_load_page(encl, addr);
 	if (IS_ERR(entry)) {
 		mutex_unlock(&encl->lock);
+		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
 
 		if (PTR_ERR(entry) == -EBUSY)
 			return VM_FAULT_NOPAGE;
@@ -315,12 +326,14 @@ static vm_fault_t sgx_vma_fault(struct vm_fault *vmf)
 				  vm_get_page_prot(page_prot_bits));
 	if (ret != VM_FAULT_NOPAGE) {
 		mutex_unlock(&encl->lock);
+		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
 
 		return VM_FAULT_SIGBUS;
 	}
 
 	sgx_encl_test_and_clear_young(vma->vm_mm, entry);
 	mutex_unlock(&encl->lock);
+	srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
 
 	return VM_FAULT_NOPAGE;
 }
@@ -513,6 +526,7 @@ static int sgx_vma_access(struct vm_area_struct *vma, unsigned long addr,
 	struct sgx_encl_page *entry = NULL;
 	char data[sizeof(unsigned long)];
 	unsigned long align;
+	int srcu_idx;
 	int offset;
 	int cnt;
 	int ret = 0;
@@ -529,6 +543,12 @@ static int sgx_vma_access(struct vm_area_struct *vma, unsigned long addr,
 		return -EFAULT;
 
 	for (i = 0; i < len; i += cnt) {
+		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
+		if (sgx_epc_is_locked()) {
+			ret = -EBUSY;
+			goto out;
+		}
+
 		entry = sgx_encl_reserve_page(encl, (addr + i) & PAGE_MASK);
 		if (IS_ERR(entry)) {
 			ret = PTR_ERR(entry);
@@ -555,6 +575,7 @@ static int sgx_vma_access(struct vm_area_struct *vma, unsigned long addr,
 
 out:
 		mutex_unlock(&encl->lock);
+		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
 
 		if (ret)
 			break;
diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
index d8c3c07badb3..da3b569a10bd 100644
--- a/arch/x86/kernel/cpu/sgx/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -147,6 +147,7 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
 static long sgx_ioc_enclave_create(struct sgx_encl *encl, void __user *arg)
 {
 	struct sgx_enclave_create create_arg;
+	int srcu_idx;
 	void *secs;
 	int ret;
 
@@ -162,9 +163,18 @@ static long sgx_ioc_enclave_create(struct sgx_encl *encl, void __user *arg)
 
 	if (copy_from_user(secs, (void __user *)create_arg.src, PAGE_SIZE))
 		ret = -EFAULT;
-	else
+	else {
+		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
+		if (sgx_epc_is_locked()) {
+			srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
+			return -EBUSY;
+		}
+
 		ret = sgx_encl_create(encl, secs);
 
+		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
+	}
+
 	kfree(secs);
 	return ret;
 }
@@ -444,6 +454,7 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg)
 	struct sgx_enclave_add_pages add_arg;
 	struct sgx_secinfo secinfo;
 	unsigned long c;
+	int srcu_idx;
 	int ret;
 
 	if (!test_bit(SGX_ENCL_CREATED, &encl->flags) ||
@@ -477,8 +488,18 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg)
 		if (need_resched())
 			cond_resched();
 
+		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
+		if (sgx_epc_is_locked()) {
+			ret = -EBUSY;
+			srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
+			break;
+		}
+
 		ret = sgx_encl_add_page(encl, add_arg.src + c, add_arg.offset + c,
 					&secinfo, add_arg.flags);
+
+		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
+
 		if (ret)
 			break;
 	}
@@ -952,6 +973,7 @@ static long sgx_enclave_restrict_perm(struct sgx_encl *encl,
 	unsigned long addr;
 	unsigned long c;
 	void *epc_virt;
+	int srcu_idx;
 	int ret;
 
 	memset(&secinfo, 0, sizeof(secinfo));
@@ -960,6 +982,12 @@ static long sgx_enclave_restrict_perm(struct sgx_encl *encl,
 	vm_prot = vm_prot_from_secinfo(secinfo_perm);
 
 	for (c = 0 ; c < modp->length; c += PAGE_SIZE) {
+		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
+		if (sgx_epc_is_locked()) {
+			srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
+			return -EBUSY;
+		}
+
 		addr = encl->base + modp->offset + c;
 
 		sgx_direct_reclaim();
@@ -1049,6 +1077,7 @@ static long sgx_enclave_restrict_perm(struct sgx_encl *encl,
 
 		sgx_mark_page_reclaimable(entry->epc_page);
 		mutex_unlock(&encl->lock);
+		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
 	}
 
 	ret = 0;
@@ -1060,6 +1089,7 @@ static long sgx_enclave_restrict_perm(struct sgx_encl *encl,
 	sgx_mark_page_reclaimable(entry->epc_page);
 out_unlock:
 	mutex_unlock(&encl->lock);
+	srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
 out:
 	modp->count = c;
 
@@ -1143,6 +1173,7 @@ static long sgx_enclave_modt(struct sgx_encl *encl,
 	unsigned long addr;
 	unsigned long c;
 	void *epc_virt;
+	int srcu_idx;
 	int ret;
 
 	/*
@@ -1156,6 +1187,12 @@ static long sgx_enclave_modt(struct sgx_encl *encl,
 	secinfo.flags = page_type << 8;
 
 	for (c = 0 ; c < modt->length; c += PAGE_SIZE) {
+		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
+		if (sgx_epc_is_locked()) {
+			srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
+			return -EBUSY;
+		}
+
 		addr = encl->base + modt->offset + c;
 
 		sgx_direct_reclaim();
@@ -1255,6 +1292,7 @@ static long sgx_enclave_modt(struct sgx_encl *encl,
 		entry->type = page_type;
 
 		mutex_unlock(&encl->lock);
+		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
 	}
 
 	ret = 0;
@@ -1265,6 +1303,7 @@ static long sgx_enclave_modt(struct sgx_encl *encl,
 	entry->vm_run_prot_bits = run_prot_restore;
 out_unlock:
 	mutex_unlock(&encl->lock);
+	srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
 out:
 	modt->count = c;
 
@@ -1350,12 +1389,19 @@ static long sgx_encl_remove_pages(struct sgx_encl *encl,
 	unsigned long addr;
 	unsigned long c;
 	void *epc_virt;
+	int srcu_idx;
 	int ret;
 
 	memset(&secinfo, 0, sizeof(secinfo));
 	secinfo.flags = SGX_SECINFO_R | SGX_SECINFO_W | SGX_SECINFO_X;
 
 	for (c = 0 ; c < params->length; c += PAGE_SIZE) {
+		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
+		if (sgx_epc_is_locked()) {
+			srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
+			return -EBUSY;
+		}
+
 		addr = encl->base + params->offset + c;
 
 		sgx_direct_reclaim();
@@ -1411,6 +1457,7 @@ static long sgx_encl_remove_pages(struct sgx_encl *encl,
 		kfree(entry);
 
 		mutex_unlock(&encl->lock);
+		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
 	}
 
 	ret = 0;
@@ -1418,6 +1465,7 @@ static long sgx_encl_remove_pages(struct sgx_encl *encl,
 
 out_unlock:
 	mutex_unlock(&encl->lock);
+	srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
 out:
 	params->count = c;
 
diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 545da16bb3ea..99c86b77ca8f 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -23,6 +23,17 @@ static int sgx_nr_epc_sections;
 static struct task_struct *ksgxd_tsk;
 static DECLARE_WAIT_QUEUE_HEAD(ksgxd_waitq);
 static DEFINE_XARRAY(sgx_epc_address_space);
+/*
+ * The flag sgx_epc_locked prevents any new SGX flows that
+ * may attempt to allocate a new EPC page.
+ */
+static bool __rcu sgx_epc_locked;
+/*
+ * By synchronizing around sgx_epc_locked SRCU ensures that any executing
+ * SGX flows have completed before proceeding with an SVN update. New SGX flows
+ * will be prevented from starting during an SVN update.
+ */
+DEFINE_SRCU(sgx_lock_epc_srcu);
 
 /*
  * These variables are part of the state of the reclaimer, and must be accessed
@@ -378,6 +389,8 @@ void sgx_direct_reclaim(void)
 
 static int ksgxd(void *p)
 {
+	int srcu_idx;
+
 	set_freezable();
 
 	/*
@@ -398,9 +411,15 @@ static int ksgxd(void *p)
 				     kthread_should_stop() ||
 				     sgx_should_reclaim(SGX_NR_HIGH_PAGES));
 
+		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
+		if (sgx_epc_is_locked())
+			goto maybe_resched;
+
 		if (sgx_should_reclaim(SGX_NR_HIGH_PAGES))
 			sgx_reclaim_pages();
 
+maybe_resched:
+		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
 		cond_resched();
 	}
 
@@ -943,3 +962,21 @@ static int __init sgx_init(void)
 }
 
 device_initcall(sgx_init);
+
+static void sgx_lock_epc(void)
+{
+	sgx_epc_locked = true;
+	synchronize_srcu(&sgx_lock_epc_srcu);
+}
+
+static void sgx_unlock_epc(void)
+{
+	sgx_epc_locked = false;
+	synchronize_srcu(&sgx_lock_epc_srcu);
+}
+
+bool sgx_epc_is_locked(void)
+{
+	lockdep_assert_held(&sgx_lock_epc_srcu);
+	return sgx_epc_locked;
+}
diff --git a/arch/x86/kernel/cpu/sgx/virt.c b/arch/x86/kernel/cpu/sgx/virt.c
index 6a77a14eee38..e953816d7c8b 100644
--- a/arch/x86/kernel/cpu/sgx/virt.c
+++ b/arch/x86/kernel/cpu/sgx/virt.c
@@ -75,10 +75,21 @@ static vm_fault_t sgx_vepc_fault(struct vm_fault *vmf)
 {
 	struct vm_area_struct *vma = vmf->vma;
 	struct sgx_vepc *vepc = vma->vm_private_data;
+	int srcu_idx;
 	int ret;
 
 	mutex_lock(&vepc->lock);
+	srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
+
+	if (sgx_epc_is_locked()) {
+		ret = -EBUSY;
+		goto out_unlock;
+	}
+
 	ret = __sgx_vepc_fault(vepc, vma, vmf->address);
+
+out_unlock:
+	srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
 	mutex_unlock(&vepc->lock);
 
 	if (!ret)
@@ -331,6 +342,7 @@ int __init sgx_vepc_init(void)
 int sgx_virt_ecreate(struct sgx_pageinfo *pageinfo, void __user *secs,
 		     int *trapnr)
 {
+	int srcu_idx;
 	int ret;
 
 	/*
@@ -347,6 +359,12 @@ int sgx_virt_ecreate(struct sgx_pageinfo *pageinfo, void __user *secs,
 	if (WARN_ON_ONCE(!access_ok(secs, PAGE_SIZE)))
 		return -EINVAL;
 
+	srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
+	if (sgx_epc_is_locked()) {
+		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
+		return -EBUSY;
+	}
+
 	__uaccess_begin();
 	ret = __ecreate(pageinfo, (void *)secs);
 	__uaccess_end();
@@ -356,6 +374,8 @@ int sgx_virt_ecreate(struct sgx_pageinfo *pageinfo, void __user *secs,
 		return -EFAULT;
 	}
 
+	srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
+
 	/* ECREATE doesn't return an error code, it faults or succeeds. */
 	WARN_ON_ONCE(ret);
 	return 0;
-- 
2.17.1

[RFC PATCH v2 02/10] x86/sgx: Provide VA page non-NULL owner

[Cathy Zhang]

Provide non-NULL owner for VA page, which tells that the EPC page is
allocated for use.

Signed-off-by: Cathy Zhang <cathy.zhang@intel.com>
---
 arch/x86/kernel/cpu/sgx/encl.h  | 2 +-
 arch/x86/kernel/cpu/sgx/sgx.h   | 2 +-
 arch/x86/kernel/cpu/sgx/encl.c  | 5 +++--
 arch/x86/kernel/cpu/sgx/ioctl.c | 2 +-
 4 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h
index 1b6ce1da7c92..1807a7f55065 100644
--- a/arch/x86/kernel/cpu/sgx/encl.h
+++ b/arch/x86/kernel/cpu/sgx/encl.h
@@ -114,7 +114,7 @@ void sgx_encl_put_backing(struct sgx_backing *backing, bool do_write);
 int sgx_encl_test_and_clear_young(struct mm_struct *mm,
 				  struct sgx_encl_page *page);
 void sgx_zap_enclave_ptes(struct sgx_encl *encl, unsigned long addr);
-struct sgx_epc_page *sgx_alloc_va_page(void);
+struct sgx_epc_page *sgx_alloc_va_page(struct sgx_va_page *va_page);
 unsigned int sgx_alloc_va_slot(struct sgx_va_page *va_page);
 void sgx_free_va_slot(struct sgx_va_page *va_page, unsigned int offset);
 bool sgx_va_page_full(struct sgx_va_page *va_page);
diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
index dc8cb58100e3..e627c42a6eec 100644
--- a/arch/x86/kernel/cpu/sgx/sgx.h
+++ b/arch/x86/kernel/cpu/sgx/sgx.h
@@ -33,7 +33,7 @@ struct sgx_epc_page {
 	unsigned int section;
 	u16 flags;
 	u16 poison;
-	struct sgx_encl_page *owner;
+	void *owner;
 	struct list_head list;
 };
 
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index d2b428992910..e08c59a049d3 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -990,6 +990,7 @@ void sgx_zap_enclave_ptes(struct sgx_encl *encl, unsigned long addr)
 
 /**
  * sgx_alloc_va_page() - Allocate a Version Array (VA) page
+ * @va_page:	struct sgx_va_page connected to this VA page
  *
  * Allocate a free EPC page and convert it to a Version Array (VA) page.
  *
@@ -997,12 +998,12 @@ void sgx_zap_enclave_ptes(struct sgx_encl *encl, unsigned long addr)
  *   a VA page,
  *   -errno otherwise
  */
-struct sgx_epc_page *sgx_alloc_va_page(void)
+struct sgx_epc_page *sgx_alloc_va_page(struct sgx_va_page *va_page)
 {
 	struct sgx_epc_page *epc_page;
 	int ret;
 
-	epc_page = sgx_alloc_epc_page(NULL, true);
+	epc_page = sgx_alloc_epc_page(va_page, true);
 	if (IS_ERR(epc_page))
 		return ERR_CAST(epc_page);
 
diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
index da3b569a10bd..9eb429aeb319 100644
--- a/arch/x86/kernel/cpu/sgx/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -30,7 +30,7 @@ struct sgx_va_page *sgx_encl_grow(struct sgx_encl *encl)
 		if (!va_page)
 			return ERR_PTR(-ENOMEM);
 
-		va_page->epc_page = sgx_alloc_va_page();
+		va_page->epc_page = sgx_alloc_va_page(va_page);
 		if (IS_ERR(va_page->epc_page)) {
 			err = ERR_CAST(va_page->epc_page);
 			kfree(va_page);
-- 
2.17.1

[RFC PATCH v2 03/10] x86/sgx: Save enclave pointer for VA page

[Cathy Zhang]

Tearing down all enclaves is required by SGX SVN update, which
involves running the ENCLS[EREMOVE] instruction on every EPC
page. This (tearing down all enclaves) should be coordinated
with any enclaves that may be in the process of existing and thus
already be running ENCLS[EREMOVE] as part of enclave release.

In support of this coordination, it is required to know which enclave
owns each in-use EPC page. It is already possible to locate the
owning enclave of SECS and regular pages but not for VA pages.

Save the enclave pointer for each VA page to support locating its
owning enclave.

Note: to track 2T EPC memory, this scheme of tracking will use
additional 8M memory.

Signed-off-by: Cathy Zhang <cathy.zhang@intel.com>
---
 arch/x86/kernel/cpu/sgx/encl.h  | 1 +
 arch/x86/kernel/cpu/sgx/ioctl.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h
index 1807a7f55065..e6f14c45ad49 100644
--- a/arch/x86/kernel/cpu/sgx/encl.h
+++ b/arch/x86/kernel/cpu/sgx/encl.h
@@ -77,6 +77,7 @@ struct sgx_va_page {
 	struct sgx_epc_page *epc_page;
 	DECLARE_BITMAP(slots, SGX_VA_SLOT_COUNT);
 	struct list_head list;
+	struct sgx_encl *encl;
 };
 
 struct sgx_backing {
diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
index 9eb429aeb319..0f7b3e24fb94 100644
--- a/arch/x86/kernel/cpu/sgx/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -30,6 +30,7 @@ struct sgx_va_page *sgx_encl_grow(struct sgx_encl *encl)
 		if (!va_page)
 			return ERR_PTR(-ENOMEM);
 
+		va_page->encl = encl;
 		va_page->epc_page = sgx_alloc_va_page(va_page);
 		if (IS_ERR(va_page->epc_page)) {
 			err = ERR_CAST(va_page->epc_page);
-- 
2.17.1

[RFC PATCH v2 04/10] x86/sgx: Keep record for SGX VA and Guest page type

[Cathy Zhang]

Regular enclave EPC pages have sgx_encl_page as their owner, but
SGX VA page and KVM guest EPC page are maintained by different
owner structures.

SGX CPUSVN update requires to know the EPC page owner's status
and then decide how to handle the page.

Keep a record of page type for SGX VA and KVM guest page while
the other EPC pages already have their type tracked, so that
CPUSVN update can get EPC page's owner by type and handle it then.

Signed-off-by: Cathy Zhang <cathy.zhang@intel.com>
---
 arch/x86/kernel/cpu/sgx/sgx.h  | 4 ++++
 arch/x86/kernel/cpu/sgx/encl.c | 2 ++
 arch/x86/kernel/cpu/sgx/virt.c | 2 ++
 3 files changed, 8 insertions(+)

diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
index e627c42a6eec..1dd06899a64c 100644
--- a/arch/x86/kernel/cpu/sgx/sgx.h
+++ b/arch/x86/kernel/cpu/sgx/sgx.h
@@ -28,6 +28,10 @@
 
 /* Pages on free list */
 #define SGX_EPC_PAGE_IS_FREE		BIT(1)
+/* VA page */
+#define SGX_EPC_PAGE_VA			BIT(2)
+/* Pages allocated for KVM guest */
+#define SGX_EPC_PAGE_GUEST		BIT(3)
 
 struct sgx_epc_page {
 	unsigned int section;
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index e08c59a049d3..a78428aa4465 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -1014,6 +1014,8 @@ struct sgx_epc_page *sgx_alloc_va_page(struct sgx_va_page *va_page)
 		return ERR_PTR(-EFAULT);
 	}
 
+	epc_page->flags |= SGX_EPC_PAGE_VA;
+
 	return epc_page;
 }
 
diff --git a/arch/x86/kernel/cpu/sgx/virt.c b/arch/x86/kernel/cpu/sgx/virt.c
index e953816d7c8b..acdf72769a39 100644
--- a/arch/x86/kernel/cpu/sgx/virt.c
+++ b/arch/x86/kernel/cpu/sgx/virt.c
@@ -50,6 +50,8 @@ static int __sgx_vepc_fault(struct sgx_vepc *vepc,
 	if (IS_ERR(epc_page))
 		return PTR_ERR(epc_page);
 
+	epc_page->flags |= SGX_EPC_PAGE_GUEST;
+
 	ret = xa_err(xa_store(&vepc->page_array, index, epc_page, GFP_KERNEL));
 	if (ret)
 		goto err_free;
-- 
2.17.1

[RFC PATCH v2 05/10] x86/sgx: Save the size of each EPC section

[Cathy Zhang]

For SGX CPUSVN update process will check all EPC pages in each
section to ensure they will be marked as unused, it requires to
know the size of each EPC section to end the loop.

Signed-off-by: Cathy Zhang <cathy.zhang@intel.com>
---
 arch/x86/kernel/cpu/sgx/sgx.h  | 1 +
 arch/x86/kernel/cpu/sgx/main.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
index 1dd06899a64c..8fa0f9c64a13 100644
--- a/arch/x86/kernel/cpu/sgx/sgx.h
+++ b/arch/x86/kernel/cpu/sgx/sgx.h
@@ -63,6 +63,7 @@ struct sgx_epc_section {
 	void *virt_addr;
 	struct sgx_epc_page *pages;
 	struct sgx_numa_node *node;
+	u64 size;
 };
 
 extern struct sgx_epc_section sgx_epc_sections[SGX_MAX_EPC_SECTIONS];
diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 99c86b77ca8f..c9331f6ba034 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -636,6 +636,7 @@ static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size,
 	}
 
 	section->phys_addr = phys_addr;
+	section->size = size;
 	xa_store_range(&sgx_epc_address_space, section->phys_addr,
 		       phys_addr + size - 1, section, GFP_KERNEL);
 
-- 
2.17.1

[RFC PATCH v2 06/10] x86/sgx: Forced EPC page zapping for EUPDATESVN

[Cathy Zhang]

Before an EUPDATESVN instruction can be successful, all enclave
pages (EPC) must be marked as unused in the SGX hardware
metadata (EPCM).

A page becomes unused when an issued EREMOVE instruction succeeds.
To prepare for EUPDATESVN, loop over all SGX pages and attempt
to EREMOVE them. This is fatal to running enclaves and destroys
all enclave state and memory contents. This destruction is by
design and mitigates any compromise of enclaves or the SGX
hardware itself which occurred before the microcode update.

An EREMOVE operation on a page may fail for a few reasons. Each
has its own mitigations.

First, EREMOVE will fail if an enclave that uses the page is
executing. Send an IPI to all CPUs that might be running the
enclave to force it out of the enclave long enough to EREMOVE
the page. Other CPUs might enter the enclave in the meantime,
so this is not a rock-solid guarantee.

Second, EREMOVE can fail on special SGX metadata pages, such as
SECS and VA. EREMOVE will work on them only after the normal SGX
pages that depend on them have been EREMOVE'd. Defer handling those
pages and repeat EREMOVE after the dependency has been addressed.

Signed-off-by: Cathy Zhang <cathy.zhang@intel.com>
---
 arch/x86/kernel/cpu/sgx/sgx.h  |  13 ++
 arch/x86/kernel/cpu/sgx/encl.c |  14 +-
 arch/x86/kernel/cpu/sgx/main.c | 347 +++++++++++++++++++++++++++++++++
 3 files changed, 373 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
index 8fa0f9c64a13..192c6a56ecf0 100644
--- a/arch/x86/kernel/cpu/sgx/sgx.h
+++ b/arch/x86/kernel/cpu/sgx/sgx.h
@@ -32,6 +32,17 @@
 #define SGX_EPC_PAGE_VA			BIT(2)
 /* Pages allocated for KVM guest */
 #define SGX_EPC_PAGE_GUEST		BIT(3)
+/*
+ * Pages, failed to be zapped (EREMOVED)
+ * by SGX CPUSVN update process.
+ */
+#define SGX_EPC_PAGE_ZAP_TRACKED	BIT(4)
+/*
+ * Pages, the associated enclave is being
+ * released while SGX CPUSVN update is
+ * running.
+ */
+#define SGX_EPC_PAGE_IN_RELEASE		BIT(5)
 
 struct sgx_epc_page {
 	unsigned int section;
@@ -111,5 +122,7 @@ void sgx_update_lepubkeyhash(u64 *lepubkeyhash);
 
 extern struct srcu_struct sgx_lock_epc_srcu;
 bool sgx_epc_is_locked(void);
+void sgx_zap_wakeup(void);
+void sgx_zap_abort(void);
 
 #endif /* _X86_SGX_H */
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index a78428aa4465..68e8985bcc31 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -651,6 +651,12 @@ void sgx_encl_release(struct kref *ref)
 	WARN_ON_ONCE(encl->secs_child_cnt);
 	WARN_ON_ONCE(encl->secs.epc_page);
 
+	/*
+	 * EPC pages were freed and EREMOVE was executed. Wake
+	 * up any zappers which were waiting for this.
+	 */
+	sgx_zap_wakeup();
+
 	kfree(encl);
 }
 
@@ -1077,8 +1083,14 @@ void sgx_encl_free_epc_page(struct sgx_epc_page *page)
 	WARN_ON_ONCE(page->flags & SGX_EPC_PAGE_RECLAIMER_TRACKED);
 
 	ret = __eremove(sgx_get_epc_virt_addr(page));
-	if (WARN_ONCE(ret, EREMOVE_ERROR_MESSAGE, ret, ret))
+	if (WARN_ONCE(ret, EREMOVE_ERROR_MESSAGE, ret, ret)) {
+		/*
+		 * The EREMOVE failed. If a CPUSVN is in progress,
+		 * it is now expected to fail. Notify it.
+		 */
+		sgx_zap_abort();
 		return;
+	}
 
 	sgx_free_epc_page(page);
 }
diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index c9331f6ba034..f3dd3757ef9f 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -34,6 +34,16 @@ static bool __rcu sgx_epc_locked;
  * will be prevented from starting during an SVN update.
  */
 DEFINE_SRCU(sgx_lock_epc_srcu);
+static DECLARE_WAIT_QUEUE_HEAD(sgx_zap_waitq);
+
+/* The flag means to abort the SGX CPUSVN update process */
+static bool sgx_zap_abort_wait;
+/*
+ * Track the number of SECS and VA pages associated with enclaves
+ * in releasing. SGX CPUSVN update will wait for them EREMOVEd by
+ * enclave exiting process.
+ */
+static atomic_t zap_waiting_count;
 
 /*
  * These variables are part of the state of the reclaimer, and must be accessed
@@ -607,6 +617,24 @@ void sgx_free_epc_page(struct sgx_epc_page *page)
 
 	spin_lock(&node->lock);
 
+	/*
+	 * The page is EREMOVEd, stop tracking it
+	 * as a deferred target for CPUSVN update
+	 * process.
+	 */
+	if ((page->flags & SGX_EPC_PAGE_ZAP_TRACKED) &&
+	    (!list_empty(&page->list)))
+		list_del(&page->list);
+
+	/*
+	 * The page is EREMOVEd, decrease
+	 * "zap_waiting_count" to stop counting it
+	 * as a waiting target for CPUSVN update
+	 * process.
+	 */
+	if (page->flags & SGX_EPC_PAGE_IN_RELEASE)
+		atomic_dec_if_positive(&zap_waiting_count);
+
 	page->owner = NULL;
 	if (page->poison)
 		list_add(&page->list, &node->sgx_poison_page_list);
@@ -981,3 +1009,322 @@ bool sgx_epc_is_locked(void)
 	lockdep_assert_held(&sgx_lock_epc_srcu);
 	return sgx_epc_locked;
 }
+
+/**
+ * sgx_zap_encl_page - unuse one EPC page
+ * @section:		EPC section
+ * @epc_page:		EPC page
+ * @secs_pages_list:	list to trac SECS pages failed to be EREMOVEd
+ *
+ * Zap an EPC page if it's used by an enclave.
+ *
+ * Returns:
+ * 0:			EPC page is unused or EREMOVE succeeds.
+ * -EBUSY:		EREMOVE failed for other threads executing
+ *			in enclave.
+ * -EIO:		Other EREMOVE failures, like EPC leaks.
+ */
+static int sgx_zap_encl_page(struct sgx_epc_section *section,
+			     struct sgx_epc_page *epc_page,
+			     struct list_head *secs_pages_list)
+{
+	struct sgx_encl *encl;
+	struct sgx_encl_page *encl_page;
+	struct sgx_va_page *va_page;
+	int retry_count = 10;
+	int ret;
+
+	/*
+	 * Holding the per-section lock to ensure the
+	 * "owner" field will not be cleared while
+	 * checking.
+	 */
+	spin_lock(&section->node->lock);
+
+	/*
+	 * The "owner" field is NULL, it means the page
+	 * is unused.
+	 */
+	if (!epc_page->owner) {
+		spin_unlock(&section->node->lock);
+		return 0;
+	}
+
+	if (epc_page->flags & SGX_EPC_PAGE_VA) {
+		va_page = epc_page->owner;
+		encl = va_page->encl;
+	} else {
+		encl_page = epc_page->owner;
+		encl = encl_page->encl;
+	}
+
+	if (!encl) {
+		spin_unlock(&section->node->lock);
+		/*
+		 * The page has owner, but without an Enclave
+		 * associated with. This might be caused by
+		 * EPC leaks happen in enclave's release path.
+		 */
+		ret = __eremove(sgx_get_epc_virt_addr(epc_page));
+		if (WARN_ONCE(ret, EREMOVE_ERROR_MESSAGE, ret, ret))
+			ret = -EIO;
+		else
+			sgx_free_epc_page(epc_page);
+		return ret;
+	}
+
+	/*
+	 * Wait for any enclave already being released to complete
+	 * but prevent any additional enclave from starting release
+	 * while we operate on it.
+	 */
+	if (!kref_get_unless_zero(&encl->refcount)) {
+
+		/*
+		 * The enclave is exiting. The EUPDATESVN
+		 * procedure needs to wait for the EREMOVE
+		 * operation which happens as a part of
+		 * the enclave exit operation. Use
+		 * "zap_waiting_count" to indicate to the
+		 * EUPDATESVN code when it needs to wait.
+		 */
+		if (((epc_page->flags & SGX_EPC_PAGE_VA) ||
+		     (encl_page->type == SGX_PAGE_TYPE_SECS)) &&
+		    !(epc_page->flags & SGX_EPC_PAGE_IN_RELEASE)) {
+			atomic_inc(&zap_waiting_count);
+			epc_page->flags |= SGX_EPC_PAGE_IN_RELEASE;
+		}
+
+		spin_unlock(&section->node->lock);
+		return 0;
+	}
+
+	spin_unlock(&section->node->lock);
+
+	/*
+	 * This EREMOVE has two main purposes:
+	 * 1. Getting EPC pages into the "unused" state.
+	 *    Every EPC page must be unused before an
+	 *    EUPDATESVN can be succeed.
+	 * 2. Forcing enclaves to exit more frequently.
+	 *    EREMOVE will not succeed while any thread is
+	 *    running in the enclave. Every successful
+	 *    EREMOVE increases the chance that an enclave
+	 *    will trip over this page, fault, and exit.
+	 *    This, in turn, increases the likelihood of
+	 *    success for every future EREMOVE attempt.
+	 */
+	ret = __eremove(sgx_get_epc_virt_addr(epc_page));
+
+	if (!ret) {
+		/*
+		 * The SECS page is EREMOVEd successfully this time.
+		 * Remove it from the list to stop tracking it.
+		 */
+		if ((epc_page->flags & SGX_EPC_PAGE_ZAP_TRACKED) &&
+		    !list_empty(&epc_page->list)) {
+			list_del_init(&epc_page->list);
+			epc_page->flags &= ~SGX_EPC_PAGE_ZAP_TRACKED;
+		}
+		goto out;
+	}
+
+	if (ret == SGX_CHILD_PRESENT) {
+		/*
+		 * The SECS page is failed to be EREMOVEd due
+		 * to associations. Add it to "secs_pages_list"
+		 * for deferred handling.
+		 */
+		if (!(epc_page->flags & SGX_EPC_PAGE_ZAP_TRACKED) &&
+		    secs_pages_list) {
+			epc_page->flags |= SGX_EPC_PAGE_ZAP_TRACKED;
+			list_add_tail(&epc_page->list, secs_pages_list);
+		}
+		ret = 0;
+		goto out;
+	}
+
+	if (ret) {
+		/*
+		 * EREMOVE will fail on a page if the owning
+		 * enclave is executing. An IPI will cause the
+		 * enclave to exit, providing an opportunity to
+		 * EREMOVE the page, but it does not guarantee
+		 * the page will be EREMOVEd successfully. Retry
+		 * for several times, if it keeps on failing,
+		 * return -EBUSY to notify userspace for retry.
+		 */
+		do {
+			on_each_cpu_mask(sgx_encl_cpumask(encl), sgx_ipi_cb, NULL, true);
+			ret = __eremove(sgx_get_epc_virt_addr(epc_page));
+			if (!ret)
+				break;
+			retry_count--;
+		} while (retry_count);
+
+		if (ret)
+			ret = -EBUSY;
+	}
+
+out:
+	kref_put(&encl->refcount, sgx_encl_release);
+	return ret;
+}
+
+/**
+ * sgx_zap_section_pages - unuse one EPC section's pages
+ * @section:		EPC section
+ * @secs_pages_list:	list to track SECS pages failed to be EREMOVEd
+ *
+ * Iterate through pages in one EPC section, unuse the pages
+ * initialized for enclaves on bare metal.
+ *
+ * TODO: EPC pages for KVM guest will be handled in future.
+ *
+ * Returns:
+ * 0:			EPC page is unused.
+ * -EBUSY:		EREMOVE failed for other threads executing
+ *			in enclave.
+ * -EIO:		Other EREMOVE failures, like EPC leaks.
+ */
+static int sgx_zap_section_pages(struct sgx_epc_section *section,
+				 struct list_head *secs_pages_list)
+{
+	struct sgx_epc_page *epc_page;
+	int i, ret;
+	unsigned long nr_pages = section->size >> PAGE_SHIFT;
+
+	for (i = 0; i < nr_pages; i++) {
+		epc_page = &section->pages[i];
+
+		/*
+		 * EPC page has "NULL" owner, indicating
+		 * it's unused. No action required for
+		 * this case.
+		 *
+		 * No new owner can be assigned when SGX
+		 * is "frozen".
+		 */
+		if (!epc_page->owner)
+			continue;
+
+		/*
+		 * Try to "unuse" all SGX memory used by enclaves
+		 * on bare-metal.
+		 *
+		 * Failures might be caused by the following reasons:
+		 * 1. EREMOVE failure due to other threads executing
+		 *    in enclave. Return -EBUSY to notify userspace
+		 *    for a later retry.
+		 * 2. Other EREMOVE failures. For example, a bug in
+		 *    SGX memory management like a leak that lost
+		 *    track of an SGX EPC page. Upon these failures,
+		 *    do not even attempt EUPDATESVN.
+		 */
+		if (!(epc_page->flags & SGX_EPC_PAGE_GUEST)) {
+			ret = sgx_zap_encl_page(section, epc_page, secs_pages_list);
+			if (ret)
+				return ret;
+		}
+	}
+
+	return ret;
+}
+
+/**
+ * sgx_zap_pages - unuse all EPC sections' pages
+ *
+ * Context: This function is called while microcode_mutex lock
+ *	    is held by the caller, it ensures that the update
+ *	    process will not run concurrently.
+ *
+ * Returns:
+ * 0:			All enclaves have been torn down and
+ *			all EPC pages are unused.
+ * -ERESTARTSYS:	Interrupted by a signal.
+ * -EBUSY:		EREMOVE failed for other threads executing
+ *			in enclave.
+ * -EIO:		Other EREMOVE failures, like EPC leaks.
+ */
+static int sgx_zap_pages(void)
+{
+	struct sgx_epc_page *epc_page, *tmp;
+	struct sgx_epc_section *section;
+	int i, ret;
+
+	LIST_HEAD(secs_pages_list);
+
+	for (i = 0; i < ARRAY_SIZE(sgx_epc_sections); i++) {
+		section = &sgx_epc_sections[i];
+		if (!section->pages)
+			break;
+		/*
+		 * Go through the section's pages and try to EREMOVE
+		 * each one, except the ones associated with enclaves
+		 * in releasing.
+		 */
+		ret = sgx_zap_section_pages(section, &secs_pages_list);
+		if (WARN_ON_ONCE(ret))
+			goto out;
+	}
+
+	/*
+	 * The SECS page should have no associations now, try
+	 * EREMOVE them again.
+	 */
+	list_for_each_entry_safe(epc_page, tmp, &secs_pages_list, list) {
+		section = &sgx_epc_sections[epc_page->section];
+		ret = sgx_zap_encl_page(section, epc_page, NULL);
+		if (ret)
+			goto out;
+	}
+
+	/*
+	 * There might be pages in the process of being freed
+	 * by exiting enclaves. Wait for the exiting process
+	 * to succeed or fail.
+	 */
+	ret = wait_event_interruptible(sgx_zap_waitq,
+				       (!atomic_read(&zap_waiting_count) ||
+					sgx_zap_abort_wait));
+	if (ret == -ERESTARTSYS) {
+		pr_err("CPUSVN update is not finished yet, but killed by userspace\n");
+		goto out;
+	}
+
+	if (sgx_zap_abort_wait) {
+		ret = -EIO;
+		pr_err("exit-side EREMOVE failure. Aborting CPUSVN update\n");
+		goto out;
+	}
+
+out:
+	return ret;
+}
+
+/**
+ * sgx_zap_wakeup - wake up CPUSVN update process
+ *
+ * Whenever enclave is freed, this function will
+ * be called to check if all EPC pages are unused.
+ * Wake up the CPUSVN update process if it's true.
+ */
+void sgx_zap_wakeup(void)
+{
+	if (wq_has_sleeper(&sgx_zap_waitq) &&
+	    !atomic_read(&zap_waiting_count))
+		wake_up(&sgx_zap_waitq);
+}
+
+/**
+ * sgx_zap_abort - abort SGX CPUSVN update process
+ *
+ * When EPC leaks happen in enclave release process,
+ * it will set flag sgx_zap_abort_wait as true to
+ * abort the CPUSVN update process.
+ */
+void sgx_zap_abort(void)
+{
+	sgx_zap_abort_wait = true;
+	wake_up(&sgx_zap_waitq);
+}
-- 
2.17.1

[RFC PATCH v2 07/10] x86/sgx: Define error codes for ENCLS[EUPDATESVN]

[Cathy Zhang]

Add error codes for ENCLS[EUPDATESVN], then SGX CPUSVN update
process can know the execution state of EUPDATESVN and notify
userspace.

Signed-off-by: Cathy Zhang <cathy.zhang@intel.com>
---
 arch/x86/include/asm/sgx.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
index eae20fa52b93..6aa36c1be426 100644
--- a/arch/x86/include/asm/sgx.h
+++ b/arch/x86/include/asm/sgx.h
@@ -73,6 +73,11 @@ enum sgx_encls_function {
  *				public key does not match IA32_SGXLEPUBKEYHASH.
  * %SGX_PAGE_NOT_MODIFIABLE:	The EPC page cannot be modified because it
  *				is in the PENDING or MODIFIED state.
+ * %SGX_INSUFFICIENT_ENTROPY:	Insufficient entropy in RNG.
+ * %SGX_EPC_NOT_READY:		EPC is not ready for SVN update.
+ * %SGX_NO_UPDATE:		EUPDATESVN was successful, but CPUSVN was not
+ *				updated because current SVN was not newer than
+ *				CPUSVN.
  * %SGX_UNMASKED_EVENT:		An unmasked event, e.g. INTR, was received
  */
 enum sgx_return_code {
@@ -81,6 +86,9 @@ enum sgx_return_code {
 	SGX_CHILD_PRESENT		= 13,
 	SGX_INVALID_EINITTOKEN		= 16,
 	SGX_PAGE_NOT_MODIFIABLE		= 20,
+	SGX_INSUFFICIENT_ENTROPY	= 29,
+	SGX_EPC_NOT_READY		= 30,
+	SGX_NO_UPDATE			= 31,
 	SGX_UNMASKED_EVENT		= 128,
 };
 
-- 
2.17.1

[RFC PATCH v2 08/10] x86/sgx: Implement ENCLS[EUPDATESVN]

[Cathy Zhang]

The SGX attestation architecture assumes a compromise of all running
enclaves and cryptographic assets (like internal SGX encryption keys)
whenever a microcode update affects SGX. To mitigate the impact of
this presumed compromise, a new supervisor SGX instruction:
ENCLS[EUPDATESVN], is introduced to update SGX microcode version and
generate new cryptographic assets in runtime after SGX microcode
update.

EUPDATESVN requires that SGX memory to be marked as "unused" before
it will succeed. This ensures that no compromised enclave can survive
the process and provides an opportunity to generate new cryptographic
assets.

Signed-off-by: Cathy Zhang <cathy.zhang@intel.com>

---
Changes since v1:
 - Print message for each return code to notify userspace the
   ENCLS[EUPDATESVN] execution status.
---
 arch/x86/include/asm/sgx.h      | 33 +++++++++++----------
 arch/x86/kernel/cpu/sgx/encls.h |  6 ++++
 arch/x86/kernel/cpu/sgx/main.c  | 52 +++++++++++++++++++++++++++++++++
 3 files changed, 76 insertions(+), 15 deletions(-)

diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
index 6aa36c1be426..d5942d0848ec 100644
--- a/arch/x86/include/asm/sgx.h
+++ b/arch/x86/include/asm/sgx.h
@@ -26,23 +26,26 @@
 #define SGX_CPUID_EPC_SECTION	0x1
 /* The bitmask for the EPC section type. */
 #define SGX_CPUID_EPC_MASK	GENMASK(3, 0)
+/* EUPDATESVN presence indication */
+#define SGX_CPUID_EUPDATESVN	BIT(10)
 
 enum sgx_encls_function {
-	ECREATE	= 0x00,
-	EADD	= 0x01,
-	EINIT	= 0x02,
-	EREMOVE	= 0x03,
-	EDGBRD	= 0x04,
-	EDGBWR	= 0x05,
-	EEXTEND	= 0x06,
-	ELDU	= 0x08,
-	EBLOCK	= 0x09,
-	EPA	= 0x0A,
-	EWB	= 0x0B,
-	ETRACK	= 0x0C,
-	EAUG	= 0x0D,
-	EMODPR	= 0x0E,
-	EMODT	= 0x0F,
+	ECREATE		= 0x00,
+	EADD		= 0x01,
+	EINIT		= 0x02,
+	EREMOVE		= 0x03,
+	EDGBRD		= 0x04,
+	EDGBWR		= 0x05,
+	EEXTEND		= 0x06,
+	ELDU		= 0x08,
+	EBLOCK		= 0x09,
+	EPA		= 0x0A,
+	EWB		= 0x0B,
+	ETRACK		= 0x0C,
+	EAUG		= 0x0D,
+	EMODPR		= 0x0E,
+	EMODT		= 0x0F,
+	EUPDATESVN	= 0x18,
 };
 
 /**
diff --git a/arch/x86/kernel/cpu/sgx/encls.h b/arch/x86/kernel/cpu/sgx/encls.h
index 3f1797ec2445..b0aa110e7560 100644
--- a/arch/x86/kernel/cpu/sgx/encls.h
+++ b/arch/x86/kernel/cpu/sgx/encls.h
@@ -243,4 +243,10 @@ static inline int __eaug(struct sgx_pageinfo *pginfo, void *addr)
 	return __encls_2(EAUG, pginfo, addr);
 }
 
+/* Update CPUSVN at runtime. */
+static inline int __eupdatesvn(void)
+{
+	return __encls_ret_1(EUPDATESVN, "");
+}
+
 #endif /* _X86_ENCLS_H */
diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index f3dd3757ef9f..123818fa2386 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -1328,3 +1328,55 @@ void sgx_zap_abort(void)
 	sgx_zap_abort_wait = true;
 	wake_up(&sgx_zap_waitq);
 }
+
+/**
+ * sgx_updatesvn() - Issue ENCLS[EUPDATESVN]
+ * If EPC is ready, this instruction will update CPUSVN to the currently
+ * loaded microcode update SVN and generate new cryptographic assets.
+ *
+ * Return:
+ * 0:				CPUSVN is update successfully.
+ * %SGX_LOCKFAIL:		An instruction concurrency rule was violated.
+ * %SGX_INSUFFICIENT_ENTROPY:	Insufficient entropy in RNG.
+ * %SGX_EPC_NOT_READY:		EPC is not ready for SVN update.
+ * %SGX_NO_UPDATE:		EUPDATESVN was successful, but CPUSVN was not
+ *				updated because current SVN was not newer than
+ *				CPUSVN.
+ */
+static int sgx_updatesvn(void)
+{
+	int ret;
+	int retry = 10;
+
+	do {
+		ret = __eupdatesvn();
+		if (ret != SGX_INSUFFICIENT_ENTROPY)
+			break;
+
+	} while (--retry);
+
+	switch (ret) {
+	case 0:
+		pr_info("EUPDATESVN was successful!\n");
+		break;
+	case SGX_NO_UPDATE:
+		pr_info("EUPDATESVN was successful, but CPUSVN was not updated, "
+			"because current SVN was not newer than CPUSVN.\n");
+		break;
+	case SGX_EPC_NOT_READY:
+		pr_info("EPC is not ready for SVN update.");
+		break;
+	case SGX_INSUFFICIENT_ENTROPY:
+		pr_info("CPUSVN update is failed due to Insufficient entropy in RNG, "
+			"please try it later.\n");
+		break;
+	case SGX_EPC_PAGE_CONFLICT:
+		pr_info("CPUSVN update is failed due to concurrency violation, please "
+			"stop running any other ENCLS leaf and try it later.\n");
+		break;
+	default:
+		break;
+	}
+
+	return ret;
+}
-- 
2.17.1

[RFC PATCH v2 09/10] x86/cpu: Call ENCLS[EUPDATESVN] procedure in microcode update

[Cathy Zhang]

EUPDATESVN is the SGX instruction which allows enclave attestation
to include information about updated microcode without a reboot.

Microcode updates which affect SGX require two phases:

1. Do the main microcode update
2. Make the new CPUSVN available for enclave attestation via
   EUPDATESVN.

Before a EUPDATESVN can succeed, all enclave pages (EPC) must be
marked as unused in the SGX metadata (EPCM). This operation destroys
all preexisting SGX enclave data and metadata. This is by design and
mitigates the impact of vulnerabilities that may have compromised
enclaves or the SGX hardware itself prior to the update.

Signed-off-by: Cathy Zhang <cathy.zhang@intel.com>

---
Changes since v1:
 - Remove the sysfs file svnupdate. (Thomas Gleixner, Dave Hansen)
 - Let late microcode load path call ENCLS[EUPDATESVN] procedure
   directly. (Borislav Petkov)
 - Redefine update_cpusvn_intel() to return void instead of int.
---
 arch/x86/include/asm/microcode.h |  5 +++++
 arch/x86/include/asm/sgx.h       |  5 +++++
 arch/x86/kernel/cpu/common.c     |  9 +++++++++
 arch/x86/kernel/cpu/sgx/main.c   | 12 ++++++++++++
 4 files changed, 31 insertions(+)

diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index d6bfdfb0f0af..1ba66b9fe198 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -3,6 +3,7 @@
 #define _ASM_X86_MICROCODE_H
 
 #include <asm/cpu.h>
+#include <asm/sgx.h>
 #include <linux/earlycpio.h>
 #include <linux/initrd.h>
 
@@ -137,4 +138,8 @@ static inline void load_ucode_ap(void)				{ }
 static inline void reload_early_microcode(void)			{ }
 #endif
 
+#ifndef update_cpusvn_intel
+static inline void update_cpusvn_intel(void) {}
+#endif
+
 #endif /* _ASM_X86_MICROCODE_H */
diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
index d5942d0848ec..d0f2832a57b3 100644
--- a/arch/x86/include/asm/sgx.h
+++ b/arch/x86/include/asm/sgx.h
@@ -412,4 +412,9 @@ int sgx_virt_einit(void __user *sigstruct, void __user *token,
 int sgx_set_attribute(unsigned long *allowed_attributes,
 		      unsigned int attribute_fd);
 
+#ifdef CONFIG_X86_SGX
+void update_cpusvn_intel(void);
+#define update_cpusvn_intel update_cpusvn_intel
+#endif
+
 #endif /* _ASM_X86_SGX_H */
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 64deb7727d00..514e621f04c3 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -59,6 +59,7 @@
 #include <asm/cpu_device_id.h>
 #include <asm/uv/uv.h>
 #include <asm/sigframe.h>
+#include <asm/sgx.h>
 
 #include "cpu.h"
 
@@ -2165,6 +2166,14 @@ void microcode_check(void)
 
 	perf_check_microcode();
 
+	/*
+	 * SGX related microcode update requires EUPDATESVN to update CPUSVN, which
+	 * will destroy all enclaves to ensure EPC is not in use. If SGX is configured
+	 * and EUPDATESVN is supported, call the EUPDATESVN procecure.
+	 */
+	if (IS_ENABLED(CONFIG_X86_SGX) && (cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN))
+		update_cpusvn_intel();
+
 	/* Reload CPUID max function as it might've changed. */
 	info.cpuid_level = cpuid_eax(0);
 
diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 123818fa2386..d86745d8cc7d 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -1380,3 +1380,15 @@ static int sgx_updatesvn(void)
 
 	return ret;
 }
+
+void update_cpusvn_intel(void)
+{
+	sgx_lock_epc();
+	if (sgx_zap_pages())
+		goto out;
+
+	sgx_updatesvn();
+
+out:
+	sgx_unlock_epc();
+}
-- 
2.17.1

[RFC PATCH v2 10/10] x86/sgx: Call ENCLS[EUPDATESVN] during SGX initialization

[Cathy Zhang]

A snapshot of the processor microcode SVN is taken each boot cycle at
the time when Intel SGX is first used. This results in microcode
updates being loadable at any time, fixing microcode issues. However,
if system boot up through kexec() from error recovery, no hardware
reset happens, any SGX leaf execution during boot up is not assumed
as the first use in such case, and no snapshot of SVN is taken. So,
it's necessary to call ENCLS[EUPDATESVN] to update SVN automatically,
rather than waiting for the admin to do it when he/she is even not
aware of that.

Call ENCLS[EUPDATESVN] after sanitizing pages will increase the chance
of success, for it requires that EPC is empty.

Signed-off-by: Cathy Zhang <cathy.zhang@intel.com>

---
Changes since v1:
 - Update accordingly for update_cpusvn_intel() return *void*.
---
 arch/x86/kernel/cpu/sgx/main.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index d86745d8cc7d..5c1fe860540c 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -397,8 +397,10 @@ void sgx_direct_reclaim(void)
 		sgx_reclaim_pages();
 }
 
+void update_cpusvn_intel(void);
 static int ksgxd(void *p)
 {
+	int ret;
 	int srcu_idx;
 
 	set_freezable();
@@ -411,7 +413,14 @@ static int ksgxd(void *p)
 	__sgx_sanitize_pages(&sgx_dirty_page_list);
 
 	/* sanity check: */
-	WARN_ON(!list_empty(&sgx_dirty_page_list));
+	if (!WARN_ON(!list_empty(&sgx_dirty_page_list))) {
+		/*
+		 * Do SVN update for kexec(). It should complete without error, for
+		 * all EPC pages are unused at this point.
+		 */
+		if (cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN)
+			update_cpusvn_intel();
+	}
 
 	while (!kthread_should_stop()) {
 		if (try_to_freeze())
-- 
2.17.1

Re: [RFC PATCH v2 09/10] x86/cpu: Call ENCLS[EUPDATESVN] procedure in microcode update

[Jethro Beekman]

[-- Attachment #1: Type: text/plain, Size: 4146 bytes --]

On 2022-03-15 02:02, Cathy Zhang wrote:
> EUPDATESVN is the SGX instruction which allows enclave attestation
> to include information about updated microcode without a reboot.
> 
> Microcode updates which affect SGX require two phases:
> 
> 1. Do the main microcode update
> 2. Make the new CPUSVN available for enclave attestation via
>    EUPDATESVN.
> 
> Before a EUPDATESVN can succeed, all enclave pages (EPC) must be
> marked as unused in the SGX metadata (EPCM). This operation destroys
> all preexisting SGX enclave data and metadata. This is by design and
> mitigates the impact of vulnerabilities that may have compromised
> enclaves or the SGX hardware itself prior to the update.
> 
> Signed-off-by: Cathy Zhang <cathy.zhang@intel.com>
> 
> ---
> Changes since v1:
>  - Remove the sysfs file svnupdate. (Thomas Gleixner, Dave Hansen)
>  - Let late microcode load path call ENCLS[EUPDATESVN] procedure
>    directly. (Borislav Petkov)
>  - Redefine update_cpusvn_intel() to return void instead of int.
> ---
>  arch/x86/include/asm/microcode.h |  5 +++++
>  arch/x86/include/asm/sgx.h       |  5 +++++
>  arch/x86/kernel/cpu/common.c     |  9 +++++++++
>  arch/x86/kernel/cpu/sgx/main.c   | 12 ++++++++++++
>  4 files changed, 31 insertions(+)
> 
> diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
> index d6bfdfb0f0af..1ba66b9fe198 100644
> --- a/arch/x86/include/asm/microcode.h
> +++ b/arch/x86/include/asm/microcode.h
> @@ -3,6 +3,7 @@
>  #define _ASM_X86_MICROCODE_H
>  
>  #include <asm/cpu.h>
> +#include <asm/sgx.h>
>  #include <linux/earlycpio.h>
>  #include <linux/initrd.h>
>  
> @@ -137,4 +138,8 @@ static inline void load_ucode_ap(void)				{ }
>  static inline void reload_early_microcode(void)			{ }
>  #endif
>  
> +#ifndef update_cpusvn_intel
> +static inline void update_cpusvn_intel(void) {}
> +#endif
> +
>  #endif /* _ASM_X86_MICROCODE_H */
> diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
> index d5942d0848ec..d0f2832a57b3 100644
> --- a/arch/x86/include/asm/sgx.h
> +++ b/arch/x86/include/asm/sgx.h
> @@ -412,4 +412,9 @@ int sgx_virt_einit(void __user *sigstruct, void __user *token,
>  int sgx_set_attribute(unsigned long *allowed_attributes,
>  		      unsigned int attribute_fd);
>  
> +#ifdef CONFIG_X86_SGX
> +void update_cpusvn_intel(void);
> +#define update_cpusvn_intel update_cpusvn_intel
> +#endif
> +
>  #endif /* _ASM_X86_SGX_H */
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 64deb7727d00..514e621f04c3 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -59,6 +59,7 @@
>  #include <asm/cpu_device_id.h>
>  #include <asm/uv/uv.h>
>  #include <asm/sigframe.h>
> +#include <asm/sgx.h>
>  
>  #include "cpu.h"
>  
> @@ -2165,6 +2166,14 @@ void microcode_check(void)
>  
>  	perf_check_microcode();
>  
> +	/*
> +	 * SGX related microcode update requires EUPDATESVN to update CPUSVN, which
> +	 * will destroy all enclaves to ensure EPC is not in use. If SGX is configured
> +	 * and EUPDATESVN is supported, call the EUPDATESVN procecure.
> +	 */
> +	if (IS_ENABLED(CONFIG_X86_SGX) && (cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN))
> +		update_cpusvn_intel();
> +
>  	/* Reload CPUID max function as it might've changed. */
>  	info.cpuid_level = cpuid_eax(0);
>  
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index 123818fa2386..d86745d8cc7d 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -1380,3 +1380,15 @@ static int sgx_updatesvn(void)
>  
>  	return ret;
>  }
> +
> +void update_cpusvn_intel(void)
> +{
> +	sgx_lock_epc();
> +	if (sgx_zap_pages())

Doing this automatically and unconditionally during a microcode update seems undesirable. This requires the userland tooling that is coordinating the microcode update to be aware of any SGX enclaves that are running and possibly coordinate sequencing with the processes containing those enclaves. This coupling does not exist today.

--
Jethro Beekman | Fortanix

> +		goto out;
> +
> +	sgx_updatesvn();
> +
> +out:
> +	sgx_unlock_epc();
> +}

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4490 bytes --]

Re: [RFC PATCH v2 09/10] x86/cpu: Call ENCLS[EUPDATESVN] procedure in microcode update

[Jethro Beekman]

[-- Attachment #1: Type: text/plain, Size: 4428 bytes --]

On 2022-03-16 10:46, Jethro Beekman wrote:
> On 2022-03-15 02:02, Cathy Zhang wrote:
>> EUPDATESVN is the SGX instruction which allows enclave attestation
>> to include information about updated microcode without a reboot.
>>
>> Microcode updates which affect SGX require two phases:
>>
>> 1. Do the main microcode update
>> 2. Make the new CPUSVN available for enclave attestation via
>>    EUPDATESVN.
>>
>> Before a EUPDATESVN can succeed, all enclave pages (EPC) must be
>> marked as unused in the SGX metadata (EPCM). This operation destroys
>> all preexisting SGX enclave data and metadata. This is by design and
>> mitigates the impact of vulnerabilities that may have compromised
>> enclaves or the SGX hardware itself prior to the update.
>>
>> Signed-off-by: Cathy Zhang <cathy.zhang@intel.com>
>>
>> ---
>> Changes since v1:
>>  - Remove the sysfs file svnupdate. (Thomas Gleixner, Dave Hansen)
>>  - Let late microcode load path call ENCLS[EUPDATESVN] procedure
>>    directly. (Borislav Petkov)
>>  - Redefine update_cpusvn_intel() to return void instead of int.
>> ---
>>  arch/x86/include/asm/microcode.h |  5 +++++
>>  arch/x86/include/asm/sgx.h       |  5 +++++
>>  arch/x86/kernel/cpu/common.c     |  9 +++++++++
>>  arch/x86/kernel/cpu/sgx/main.c   | 12 ++++++++++++
>>  4 files changed, 31 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
>> index d6bfdfb0f0af..1ba66b9fe198 100644
>> --- a/arch/x86/include/asm/microcode.h
>> +++ b/arch/x86/include/asm/microcode.h
>> @@ -3,6 +3,7 @@
>>  #define _ASM_X86_MICROCODE_H
>>  
>>  #include <asm/cpu.h>
>> +#include <asm/sgx.h>
>>  #include <linux/earlycpio.h>
>>  #include <linux/initrd.h>
>>  
>> @@ -137,4 +138,8 @@ static inline void load_ucode_ap(void)				{ }
>>  static inline void reload_early_microcode(void)			{ }
>>  #endif
>>  
>> +#ifndef update_cpusvn_intel
>> +static inline void update_cpusvn_intel(void) {}
>> +#endif
>> +
>>  #endif /* _ASM_X86_MICROCODE_H */
>> diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
>> index d5942d0848ec..d0f2832a57b3 100644
>> --- a/arch/x86/include/asm/sgx.h
>> +++ b/arch/x86/include/asm/sgx.h
>> @@ -412,4 +412,9 @@ int sgx_virt_einit(void __user *sigstruct, void __user *token,
>>  int sgx_set_attribute(unsigned long *allowed_attributes,
>>  		      unsigned int attribute_fd);
>>  
>> +#ifdef CONFIG_X86_SGX
>> +void update_cpusvn_intel(void);
>> +#define update_cpusvn_intel update_cpusvn_intel
>> +#endif
>> +
>>  #endif /* _ASM_X86_SGX_H */
>> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
>> index 64deb7727d00..514e621f04c3 100644
>> --- a/arch/x86/kernel/cpu/common.c
>> +++ b/arch/x86/kernel/cpu/common.c
>> @@ -59,6 +59,7 @@
>>  #include <asm/cpu_device_id.h>
>>  #include <asm/uv/uv.h>
>>  #include <asm/sigframe.h>
>> +#include <asm/sgx.h>
>>  
>>  #include "cpu.h"
>>  
>> @@ -2165,6 +2166,14 @@ void microcode_check(void)
>>  
>>  	perf_check_microcode();
>>  
>> +	/*
>> +	 * SGX related microcode update requires EUPDATESVN to update CPUSVN, which
>> +	 * will destroy all enclaves to ensure EPC is not in use. If SGX is configured
>> +	 * and EUPDATESVN is supported, call the EUPDATESVN procecure.
>> +	 */
>> +	if (IS_ENABLED(CONFIG_X86_SGX) && (cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN))
>> +		update_cpusvn_intel();
>> +
>>  	/* Reload CPUID max function as it might've changed. */
>>  	info.cpuid_level = cpuid_eax(0);
>>  
>> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
>> index 123818fa2386..d86745d8cc7d 100644
>> --- a/arch/x86/kernel/cpu/sgx/main.c
>> +++ b/arch/x86/kernel/cpu/sgx/main.c
>> @@ -1380,3 +1380,15 @@ static int sgx_updatesvn(void)
>>  
>>  	return ret;
>>  }
>> +
>> +void update_cpusvn_intel(void)
>> +{
>> +	sgx_lock_epc();
>> +	if (sgx_zap_pages())
> 
> Doing this automatically and unconditionally during a microcode update seems undesirable. This requires the userland tooling that is coordinating the microcode update to be aware of any SGX enclaves that are running and possibly coordinate sequencing with the processes containing those enclaves. This coupling does not exist today.

Also, a microcode update may not affect SGX security at all and doing the EUPDATESVN procedure may not be required for this particular update. This case is called out specifically in the EUPDATESVN documentation.

--
Jethro Beekman | Fortanix

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4490 bytes --]

Re: [RFC PATCH v2 09/10] x86/cpu: Call ENCLS[EUPDATESVN] procedure in microcode update

[Dave Hansen]

On 3/16/22 02:46, Jethro Beekman wrote:
>> +void update_cpusvn_intel(void) +{ +	sgx_lock_epc(); +	if
>> (sgx_zap_pages())
> Doing this automatically and unconditionally during a microcode
> update seems undesirable. This requires the userland tooling that is
> coordinating the microcode update to be aware of any SGX enclaves
> that are running and possibly coordinate sequencing with the
> processes containing those enclaves. This coupling does not exist
> today.

"Today" in what?

If a microcode update changes SGX behavior and bumps CPUSVN, it's
fundamentally incompatible with letting enclaves continue to run.  They
might as well be killed.

But, seriously, if you can't handle enclaves being killed every few
months, don't use SGX.  The architecture doesn't allow data to be
persistent like normal x86.  It's fundamental to the architecture.  You
can also think of this as a shiny new SGX *testing* feature: one that
keeps enclave owners from becoming complacent and forgetting about what
the SGX architecture provides.

Re: [RFC PATCH v2 09/10] x86/cpu: Call ENCLS[EUPDATESVN] procedure in microcode update

[Dave Hansen]

On 3/16/22 03:24, Jethro Beekman wrote:
>> Doing this automatically and unconditionally during a microcode
>> update seems undesirable. This requires the userland tooling that
>> is coordinating the microcode update to be aware of any SGX
>> enclaves that are running and possibly coordinate sequencing with
>> the processes containing those enclaves. This coupling does not
>> exist today.
> Also, a microcode update may not affect SGX security at all and doing
> the EUPDATESVN procedure may not be required for this particular
> update. This case is called out specifically in the EUPDATESVN
> documentation.

I don't think Intel provides the metadata for the kernel to tell if an
update requires an EUPDATESVN procedure or not.  If this is inconvenient
for you, I'd suggest reporting this to the folks at Intel who can fix
it.  It doesn't seem like something which they are motivated to fix.

Re: [RFC PATCH v2 01/10] x86/sgx: Introduce mechanism to prevent new initializations of EPC pages

[Jarkko Sakkinen]

On Tue, Mar 15, 2022 at 09:02:51AM +0800, Cathy Zhang wrote:
> == Background ==
> 
> EUPDATESVN is a new SGX instruction which allows enclave attestation
> to include information about updated microcode without a reboot.
> 
> The SGX hardware maintains metadata for each enclave page to help
> enforce its security guarantees. This includes things like a record
> of the enclave to which the page belongs and the type of the page:
> SGX metadata like "VA" or "SECS" pages, or regular enclave pages
> like those that store user data.
> 
> Before an EUPDATESVN operation can be successful, all SGX memory (aka.
> EPC) must be marked as "unused" in the SGX hardware metadata (aka,
> EPCM). The SGX microcode now maintains a reference count of pages
> which are unused to aid in determining when all pages reach the
> "unused" state.
> 
> Both bare-metal and KVM guest EPC must be made unused. To increase
> the chance of a successful EUPDATESVN, the kernel prevents existing
> enclaves from creating new, valid pages and prevents new enclave
> creation (creating an enclave involves initializing a "SECS" page).
> 
> The entire EUPDATESVN process is very slow since it potentially
> affects gigabytes of enclave memory. It can potentially take seconds
> or minutes to complete. Userspace may encounter -EBUSY errors during
> the update and is expected to retry.
> 
> == Patch contents ==
> 
> Introduce mechanism to prevent new initializations of EPC pages.
> 
> Use a flag to indicate when SGX EPC pages are "locked", which means
> it's not allowed to allocate new EPC page for use. Check it in all
> paths that can initialize an EPC page. Use SRCU to ensure that the
> flag is visible across the system before proceeding with an update.
> 
> Add checks to all sites that call SGX instructions that can transition
> pages from unused to initialized to ensure that the SRCU lock is held.
> 
> Signed-off-by: Cathy Zhang <cathy.zhang@intel.com>
> ---
>  arch/x86/kernel/cpu/sgx/encls.h | 10 +++++++
>  arch/x86/kernel/cpu/sgx/sgx.h   |  3 ++
>  arch/x86/kernel/cpu/sgx/encl.c  | 25 +++++++++++++++--
>  arch/x86/kernel/cpu/sgx/ioctl.c | 50 ++++++++++++++++++++++++++++++++-
>  arch/x86/kernel/cpu/sgx/main.c  | 37 ++++++++++++++++++++++++
>  arch/x86/kernel/cpu/sgx/virt.c  | 20 +++++++++++++
>  6 files changed, 142 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/encls.h b/arch/x86/kernel/cpu/sgx/encls.h
> index 99004b02e2ed..3f1797ec2445 100644
> --- a/arch/x86/kernel/cpu/sgx/encls.h
> +++ b/arch/x86/kernel/cpu/sgx/encls.h
> @@ -139,6 +139,8 @@ static inline bool encls_failed(int ret)
>  /* Initialize an EPC page into an SGX Enclave Control Structure (SECS) page. */
>  static inline int __ecreate(struct sgx_pageinfo *pginfo, void *secs)
>  {
> +	lockdep_assert_held(&sgx_lock_epc_srcu);
> +
>  	return __encls_2(ECREATE, pginfo, secs);
>  }
>  
> @@ -154,6 +156,8 @@ static inline int __eextend(void *secs, void *addr)
>   */
>  static inline int __eadd(struct sgx_pageinfo *pginfo, void *addr)
>  {
> +	lockdep_assert_held(&sgx_lock_epc_srcu);
> +
>  	return __encls_2(EADD, pginfo, addr);
>  }
>  
> @@ -191,6 +195,8 @@ static inline int __etrack(void *addr)
>  static inline int __eldu(struct sgx_pageinfo *pginfo, void *addr,
>  			 void *va)
>  {
> +	lockdep_assert_held(&sgx_lock_epc_srcu);
> +
>  	return __encls_ret_3(ELDU, pginfo, addr, va);
>  }
>  
> @@ -205,6 +211,8 @@ static inline int __epa(void *addr)
>  {
>  	unsigned long rbx = SGX_PAGE_TYPE_VA;
>  
> +	lockdep_assert_held(&sgx_lock_epc_srcu);
> +
>  	return __encls_2(EPA, rbx, addr);
>  }
>  
> @@ -230,6 +238,8 @@ static inline int __emodt(struct sgx_secinfo *secinfo, void *addr)
>  /* Zero a page of EPC memory and add it to an initialized enclave. */
>  static inline int __eaug(struct sgx_pageinfo *pginfo, void *addr)
>  {
> +	lockdep_assert_held(&sgx_lock_epc_srcu);
> +
>  	return __encls_2(EAUG, pginfo, addr);
>  }
>  
> diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
> index 85cbf103b0dd..dc8cb58100e3 100644
> --- a/arch/x86/kernel/cpu/sgx/sgx.h
> +++ b/arch/x86/kernel/cpu/sgx/sgx.h
> @@ -104,4 +104,7 @@ static inline int __init sgx_vepc_init(void)
>  
>  void sgx_update_lepubkeyhash(u64 *lepubkeyhash);
>  
> +extern struct srcu_struct sgx_lock_epc_srcu;
> +bool sgx_epc_is_locked(void);
> +
>  #endif /* _X86_SGX_H */
> diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> index a0255d41e075..d2b428992910 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.c
> +++ b/arch/x86/kernel/cpu/sgx/encl.c
> @@ -264,6 +264,7 @@ static vm_fault_t sgx_vma_fault(struct vm_fault *vmf)
>  	unsigned long phys_addr;
>  	struct sgx_encl *encl;
>  	vm_fault_t ret;
> +	int srcu_idx;
>  
>  	encl = vma->vm_private_data;
>  
> @@ -275,6 +276,12 @@ static vm_fault_t sgx_vma_fault(struct vm_fault *vmf)
>  	if (unlikely(!encl))
>  		return VM_FAULT_SIGBUS;
>  
> +	srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> +	if (sgx_epc_is_locked()) {
> +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> +		return VM_FAULT_SIGBUS;
> +	}
> +
>  	/*
>  	 * The page_array keeps track of all enclave pages, whether they
>  	 * are swapped out or not. If there is no entry for this page and
> @@ -283,14 +290,18 @@ static vm_fault_t sgx_vma_fault(struct vm_fault *vmf)
>  	 * enclave that will be checked for right away.
>  	 */
>  	if (cpu_feature_enabled(X86_FEATURE_SGX2) &&
> -	    (!xa_load(&encl->page_array, PFN_DOWN(addr))))
> -		return sgx_encl_eaug_page(vma, encl, addr);
> +	    (!xa_load(&encl->page_array, PFN_DOWN(addr)))) {
> +		ret = sgx_encl_eaug_page(vma, encl, addr);
> +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> +		return ret;
> +	}
>  
>  	mutex_lock(&encl->lock);
>  
>  	entry = sgx_encl_load_page(encl, addr);
>  	if (IS_ERR(entry)) {
>  		mutex_unlock(&encl->lock);
> +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
>  
>  		if (PTR_ERR(entry) == -EBUSY)
>  			return VM_FAULT_NOPAGE;
> @@ -315,12 +326,14 @@ static vm_fault_t sgx_vma_fault(struct vm_fault *vmf)
>  				  vm_get_page_prot(page_prot_bits));
>  	if (ret != VM_FAULT_NOPAGE) {
>  		mutex_unlock(&encl->lock);
> +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
>  
>  		return VM_FAULT_SIGBUS;
>  	}
>  
>  	sgx_encl_test_and_clear_young(vma->vm_mm, entry);
>  	mutex_unlock(&encl->lock);
> +	srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
>  
>  	return VM_FAULT_NOPAGE;
>  }
> @@ -513,6 +526,7 @@ static int sgx_vma_access(struct vm_area_struct *vma, unsigned long addr,
>  	struct sgx_encl_page *entry = NULL;
>  	char data[sizeof(unsigned long)];
>  	unsigned long align;
> +	int srcu_idx;
>  	int offset;
>  	int cnt;
>  	int ret = 0;
> @@ -529,6 +543,12 @@ static int sgx_vma_access(struct vm_area_struct *vma, unsigned long addr,
>  		return -EFAULT;
>  
>  	for (i = 0; i < len; i += cnt) {
> +		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> +		if (sgx_epc_is_locked()) {
> +			ret = -EBUSY;
> +			goto out;
> +		}
> +
>  		entry = sgx_encl_reserve_page(encl, (addr + i) & PAGE_MASK);
>  		if (IS_ERR(entry)) {
>  			ret = PTR_ERR(entry);
> @@ -555,6 +575,7 @@ static int sgx_vma_access(struct vm_area_struct *vma, unsigned long addr,
>  
>  out:
>  		mutex_unlock(&encl->lock);
> +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
>  
>  		if (ret)
>  			break;
> diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
> index d8c3c07badb3..da3b569a10bd 100644
> --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> @@ -147,6 +147,7 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
>  static long sgx_ioc_enclave_create(struct sgx_encl *encl, void __user *arg)
>  {
>  	struct sgx_enclave_create create_arg;
> +	int srcu_idx;
>  	void *secs;
>  	int ret;
>  
> @@ -162,9 +163,18 @@ static long sgx_ioc_enclave_create(struct sgx_encl *encl, void __user *arg)
>  
>  	if (copy_from_user(secs, (void __user *)create_arg.src, PAGE_SIZE))
>  		ret = -EFAULT;
> -	else
> +	else {
> +		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> +		if (sgx_epc_is_locked()) {
> +			srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> +			return -EBUSY;
> +		}
> +
>  		ret = sgx_encl_create(encl, secs);
>  
> +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> +	}
> +
>  	kfree(secs);
>  	return ret;
>  }
> @@ -444,6 +454,7 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg)
>  	struct sgx_enclave_add_pages add_arg;
>  	struct sgx_secinfo secinfo;
>  	unsigned long c;
> +	int srcu_idx;
>  	int ret;
>  
>  	if (!test_bit(SGX_ENCL_CREATED, &encl->flags) ||
> @@ -477,8 +488,18 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg)
>  		if (need_resched())
>  			cond_resched();
>  
> +		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> +		if (sgx_epc_is_locked()) {
> +			ret = -EBUSY;
> +			srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> +			break;
> +		}
> +
>  		ret = sgx_encl_add_page(encl, add_arg.src + c, add_arg.offset + c,
>  					&secinfo, add_arg.flags);
> +
> +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> +
>  		if (ret)
>  			break;
>  	}
> @@ -952,6 +973,7 @@ static long sgx_enclave_restrict_perm(struct sgx_encl *encl,
>  	unsigned long addr;
>  	unsigned long c;
>  	void *epc_virt;
> +	int srcu_idx;
>  	int ret;
>  
>  	memset(&secinfo, 0, sizeof(secinfo));
> @@ -960,6 +982,12 @@ static long sgx_enclave_restrict_perm(struct sgx_encl *encl,
>  	vm_prot = vm_prot_from_secinfo(secinfo_perm);
>  
>  	for (c = 0 ; c < modp->length; c += PAGE_SIZE) {
> +		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> +		if (sgx_epc_is_locked()) {
> +			srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> +			return -EBUSY;
> +		}
> +
>  		addr = encl->base + modp->offset + c;
>  
>  		sgx_direct_reclaim();
> @@ -1049,6 +1077,7 @@ static long sgx_enclave_restrict_perm(struct sgx_encl *encl,
>  
>  		sgx_mark_page_reclaimable(entry->epc_page);
>  		mutex_unlock(&encl->lock);
> +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
>  	}
>  
>  	ret = 0;
> @@ -1060,6 +1089,7 @@ static long sgx_enclave_restrict_perm(struct sgx_encl *encl,
>  	sgx_mark_page_reclaimable(entry->epc_page);
>  out_unlock:
>  	mutex_unlock(&encl->lock);
> +	srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
>  out:
>  	modp->count = c;
>  
> @@ -1143,6 +1173,7 @@ static long sgx_enclave_modt(struct sgx_encl *encl,
>  	unsigned long addr;
>  	unsigned long c;
>  	void *epc_virt;
> +	int srcu_idx;
>  	int ret;
>  
>  	/*
> @@ -1156,6 +1187,12 @@ static long sgx_enclave_modt(struct sgx_encl *encl,
>  	secinfo.flags = page_type << 8;
>  
>  	for (c = 0 ; c < modt->length; c += PAGE_SIZE) {
> +		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> +		if (sgx_epc_is_locked()) {
> +			srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> +			return -EBUSY;
> +		}
> +
>  		addr = encl->base + modt->offset + c;
>  
>  		sgx_direct_reclaim();
> @@ -1255,6 +1292,7 @@ static long sgx_enclave_modt(struct sgx_encl *encl,
>  		entry->type = page_type;
>  
>  		mutex_unlock(&encl->lock);
> +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
>  	}
>  
>  	ret = 0;
> @@ -1265,6 +1303,7 @@ static long sgx_enclave_modt(struct sgx_encl *encl,
>  	entry->vm_run_prot_bits = run_prot_restore;
>  out_unlock:
>  	mutex_unlock(&encl->lock);
> +	srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
>  out:
>  	modt->count = c;
>  
> @@ -1350,12 +1389,19 @@ static long sgx_encl_remove_pages(struct sgx_encl *encl,
>  	unsigned long addr;
>  	unsigned long c;
>  	void *epc_virt;
> +	int srcu_idx;
>  	int ret;
>  
>  	memset(&secinfo, 0, sizeof(secinfo));
>  	secinfo.flags = SGX_SECINFO_R | SGX_SECINFO_W | SGX_SECINFO_X;
>  
>  	for (c = 0 ; c < params->length; c += PAGE_SIZE) {
> +		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> +		if (sgx_epc_is_locked()) {
> +			srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> +			return -EBUSY;
> +		}
> +
>  		addr = encl->base + params->offset + c;
>  
>  		sgx_direct_reclaim();
> @@ -1411,6 +1457,7 @@ static long sgx_encl_remove_pages(struct sgx_encl *encl,
>  		kfree(entry);
>  
>  		mutex_unlock(&encl->lock);
> +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
>  	}
>  
>  	ret = 0;
> @@ -1418,6 +1465,7 @@ static long sgx_encl_remove_pages(struct sgx_encl *encl,
>  
>  out_unlock:
>  	mutex_unlock(&encl->lock);
> +	srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
>  out:
>  	params->count = c;
>  
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index 545da16bb3ea..99c86b77ca8f 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -23,6 +23,17 @@ static int sgx_nr_epc_sections;
>  static struct task_struct *ksgxd_tsk;
>  static DECLARE_WAIT_QUEUE_HEAD(ksgxd_waitq);
>  static DEFINE_XARRAY(sgx_epc_address_space);
> +/*
> + * The flag sgx_epc_locked prevents any new SGX flows that
> + * may attempt to allocate a new EPC page.
> + */
> +static bool __rcu sgx_epc_locked;
> +/*
> + * By synchronizing around sgx_epc_locked SRCU ensures that any executing
> + * SGX flows have completed before proceeding with an SVN update. New SGX flows
> + * will be prevented from starting during an SVN update.
> + */
> +DEFINE_SRCU(sgx_lock_epc_srcu);
>  
>  /*
>   * These variables are part of the state of the reclaimer, and must be accessed
> @@ -378,6 +389,8 @@ void sgx_direct_reclaim(void)
>  
>  static int ksgxd(void *p)
>  {
> +	int srcu_idx;
> +
>  	set_freezable();
>  
>  	/*
> @@ -398,9 +411,15 @@ static int ksgxd(void *p)
>  				     kthread_should_stop() ||
>  				     sgx_should_reclaim(SGX_NR_HIGH_PAGES));
>  
> +		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> +		if (sgx_epc_is_locked())
> +			goto maybe_resched;
> +
>  		if (sgx_should_reclaim(SGX_NR_HIGH_PAGES))
>  			sgx_reclaim_pages();
>  
> +maybe_resched:
> +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
>  		cond_resched();
>  	}
>  
> @@ -943,3 +962,21 @@ static int __init sgx_init(void)
>  }
>  
>  device_initcall(sgx_init);
> +
> +static void sgx_lock_epc(void)
> +{
> +	sgx_epc_locked = true;
> +	synchronize_srcu(&sgx_lock_epc_srcu);
> +}
> +
> +static void sgx_unlock_epc(void)
> +{
> +	sgx_epc_locked = false;
> +	synchronize_srcu(&sgx_lock_epc_srcu);
> +}
> +
> +bool sgx_epc_is_locked(void)
> +{
> +	lockdep_assert_held(&sgx_lock_epc_srcu);
> +	return sgx_epc_locked;
> +}
> diff --git a/arch/x86/kernel/cpu/sgx/virt.c b/arch/x86/kernel/cpu/sgx/virt.c
> index 6a77a14eee38..e953816d7c8b 100644
> --- a/arch/x86/kernel/cpu/sgx/virt.c
> +++ b/arch/x86/kernel/cpu/sgx/virt.c
> @@ -75,10 +75,21 @@ static vm_fault_t sgx_vepc_fault(struct vm_fault *vmf)
>  {
>  	struct vm_area_struct *vma = vmf->vma;
>  	struct sgx_vepc *vepc = vma->vm_private_data;
> +	int srcu_idx;
>  	int ret;
>  
>  	mutex_lock(&vepc->lock);
> +	srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> +
> +	if (sgx_epc_is_locked()) {
> +		ret = -EBUSY;
> +		goto out_unlock;
> +	}
> +
>  	ret = __sgx_vepc_fault(vepc, vma, vmf->address);
> +
> +out_unlock:
> +	srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
>  	mutex_unlock(&vepc->lock);
>  
>  	if (!ret)
> @@ -331,6 +342,7 @@ int __init sgx_vepc_init(void)
>  int sgx_virt_ecreate(struct sgx_pageinfo *pageinfo, void __user *secs,
>  		     int *trapnr)
>  {
> +	int srcu_idx;
>  	int ret;
>  
>  	/*
> @@ -347,6 +359,12 @@ int sgx_virt_ecreate(struct sgx_pageinfo *pageinfo, void __user *secs,
>  	if (WARN_ON_ONCE(!access_ok(secs, PAGE_SIZE)))
>  		return -EINVAL;
>  
> +	srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> +	if (sgx_epc_is_locked()) {
> +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> +		return -EBUSY;
> +	}
> +
>  	__uaccess_begin();
>  	ret = __ecreate(pageinfo, (void *)secs);
>  	__uaccess_end();
> @@ -356,6 +374,8 @@ int sgx_virt_ecreate(struct sgx_pageinfo *pageinfo, void __user *secs,
>  		return -EFAULT;
>  	}
>  
> +	srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> +
>  	/* ECREATE doesn't return an error code, it faults or succeeds. */
>  	WARN_ON_ONCE(ret);
>  	return 0;
> -- 
> 2.17.1
> 

This series is broken. It does not apply on top of tip/x86/sgx. Also you
forgot CC to me.

BR, Jarkko

RE: [RFC PATCH v2 01/10] x86/sgx: Introduce mechanism to prevent new initializations of EPC pages

[Zhang, Cathy]

Hi Jarkko,

> -----Original Message-----
> From: Jarkko Sakkinen <jarkko@kernel.org>
> Sent: Thursday, March 17, 2022 12:13 PM
> To: Zhang, Cathy <cathy.zhang@intel.com>
> Cc: linux-sgx@vger.kernel.org; linux-kernel@vger.kernel.org; Hansen, Dave
> <dave.hansen@intel.com>; Raj, Ashok <ashok.raj@intel.com>
> Subject: Re: [RFC PATCH v2 01/10] x86/sgx: Introduce mechanism to prevent
> new initializations of EPC pages
> 
> On Tue, Mar 15, 2022 at 09:02:51AM +0800, Cathy Zhang wrote:
> > == Background ==
> >
> > EUPDATESVN is a new SGX instruction which allows enclave attestation
> > to include information about updated microcode without a reboot.
> >
> > The SGX hardware maintains metadata for each enclave page to help
> > enforce its security guarantees. This includes things like a record of
> > the enclave to which the page belongs and the type of the page:
> > SGX metadata like "VA" or "SECS" pages, or regular enclave pages like
> > those that store user data.
> >
> > Before an EUPDATESVN operation can be successful, all SGX memory (aka.
> > EPC) must be marked as "unused" in the SGX hardware metadata (aka,
> > EPCM). The SGX microcode now maintains a reference count of pages
> > which are unused to aid in determining when all pages reach the
> > "unused" state.
> >
> > Both bare-metal and KVM guest EPC must be made unused. To increase the
> > chance of a successful EUPDATESVN, the kernel prevents existing
> > enclaves from creating new, valid pages and prevents new enclave
> > creation (creating an enclave involves initializing a "SECS" page).
> >
> > The entire EUPDATESVN process is very slow since it potentially
> > affects gigabytes of enclave memory. It can potentially take seconds
> > or minutes to complete. Userspace may encounter -EBUSY errors during
> > the update and is expected to retry.
> >
> > == Patch contents ==
> >
> > Introduce mechanism to prevent new initializations of EPC pages.
> >
> > Use a flag to indicate when SGX EPC pages are "locked", which means
> > it's not allowed to allocate new EPC page for use. Check it in all
> > paths that can initialize an EPC page. Use SRCU to ensure that the
> > flag is visible across the system before proceeding with an update.
> >
> > Add checks to all sites that call SGX instructions that can transition
> > pages from unused to initialized to ensure that the SRCU lock is held.
> >
> > Signed-off-by: Cathy Zhang <cathy.zhang@intel.com>
> > ---
> >  arch/x86/kernel/cpu/sgx/encls.h | 10 +++++++
> >  arch/x86/kernel/cpu/sgx/sgx.h   |  3 ++
> >  arch/x86/kernel/cpu/sgx/encl.c  | 25 +++++++++++++++--
> > arch/x86/kernel/cpu/sgx/ioctl.c | 50
> ++++++++++++++++++++++++++++++++-
> > arch/x86/kernel/cpu/sgx/main.c  | 37 ++++++++++++++++++++++++
> > arch/x86/kernel/cpu/sgx/virt.c  | 20 +++++++++++++
> >  6 files changed, 142 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/kernel/cpu/sgx/encls.h
> > b/arch/x86/kernel/cpu/sgx/encls.h index 99004b02e2ed..3f1797ec2445
> > 100644
> > --- a/arch/x86/kernel/cpu/sgx/encls.h
> > +++ b/arch/x86/kernel/cpu/sgx/encls.h
> > @@ -139,6 +139,8 @@ static inline bool encls_failed(int ret)
> >  /* Initialize an EPC page into an SGX Enclave Control Structure
> > (SECS) page. */  static inline int __ecreate(struct sgx_pageinfo
> > *pginfo, void *secs)  {
> > +	lockdep_assert_held(&sgx_lock_epc_srcu);
> > +
> >  	return __encls_2(ECREATE, pginfo, secs);  }
> >
> > @@ -154,6 +156,8 @@ static inline int __eextend(void *secs, void *addr)
> >   */
> >  static inline int __eadd(struct sgx_pageinfo *pginfo, void *addr)  {
> > +	lockdep_assert_held(&sgx_lock_epc_srcu);
> > +
> >  	return __encls_2(EADD, pginfo, addr);  }
> >
> > @@ -191,6 +195,8 @@ static inline int __etrack(void *addr)  static
> > inline int __eldu(struct sgx_pageinfo *pginfo, void *addr,
> >  			 void *va)
> >  {
> > +	lockdep_assert_held(&sgx_lock_epc_srcu);
> > +
> >  	return __encls_ret_3(ELDU, pginfo, addr, va);  }
> >
> > @@ -205,6 +211,8 @@ static inline int __epa(void *addr)  {
> >  	unsigned long rbx = SGX_PAGE_TYPE_VA;
> >
> > +	lockdep_assert_held(&sgx_lock_epc_srcu);
> > +
> >  	return __encls_2(EPA, rbx, addr);
> >  }
> >
> > @@ -230,6 +238,8 @@ static inline int __emodt(struct sgx_secinfo
> > *secinfo, void *addr)
> >  /* Zero a page of EPC memory and add it to an initialized enclave. */
> > static inline int __eaug(struct sgx_pageinfo *pginfo, void *addr)  {
> > +	lockdep_assert_held(&sgx_lock_epc_srcu);
> > +
> >  	return __encls_2(EAUG, pginfo, addr);  }
> >
> > diff --git a/arch/x86/kernel/cpu/sgx/sgx.h
> > b/arch/x86/kernel/cpu/sgx/sgx.h index 85cbf103b0dd..dc8cb58100e3
> > 100644
> > --- a/arch/x86/kernel/cpu/sgx/sgx.h
> > +++ b/arch/x86/kernel/cpu/sgx/sgx.h
> > @@ -104,4 +104,7 @@ static inline int __init sgx_vepc_init(void)
> >
> >  void sgx_update_lepubkeyhash(u64 *lepubkeyhash);
> >
> > +extern struct srcu_struct sgx_lock_epc_srcu; bool
> > +sgx_epc_is_locked(void);
> > +
> >  #endif /* _X86_SGX_H */
> > diff --git a/arch/x86/kernel/cpu/sgx/encl.c
> > b/arch/x86/kernel/cpu/sgx/encl.c index a0255d41e075..d2b428992910
> > 100644
> > --- a/arch/x86/kernel/cpu/sgx/encl.c
> > +++ b/arch/x86/kernel/cpu/sgx/encl.c
> > @@ -264,6 +264,7 @@ static vm_fault_t sgx_vma_fault(struct vm_fault
> *vmf)
> >  	unsigned long phys_addr;
> >  	struct sgx_encl *encl;
> >  	vm_fault_t ret;
> > +	int srcu_idx;
> >
> >  	encl = vma->vm_private_data;
> >
> > @@ -275,6 +276,12 @@ static vm_fault_t sgx_vma_fault(struct vm_fault
> *vmf)
> >  	if (unlikely(!encl))
> >  		return VM_FAULT_SIGBUS;
> >
> > +	srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> > +	if (sgx_epc_is_locked()) {
> > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > +		return VM_FAULT_SIGBUS;
> > +	}
> > +
> >  	/*
> >  	 * The page_array keeps track of all enclave pages, whether they
> >  	 * are swapped out or not. If there is no entry for this page and @@
> > -283,14 +290,18 @@ static vm_fault_t sgx_vma_fault(struct vm_fault *vmf)
> >  	 * enclave that will be checked for right away.
> >  	 */
> >  	if (cpu_feature_enabled(X86_FEATURE_SGX2) &&
> > -	    (!xa_load(&encl->page_array, PFN_DOWN(addr))))
> > -		return sgx_encl_eaug_page(vma, encl, addr);
> > +	    (!xa_load(&encl->page_array, PFN_DOWN(addr)))) {
> > +		ret = sgx_encl_eaug_page(vma, encl, addr);
> > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > +		return ret;
> > +	}
> >
> >  	mutex_lock(&encl->lock);
> >
> >  	entry = sgx_encl_load_page(encl, addr);
> >  	if (IS_ERR(entry)) {
> >  		mutex_unlock(&encl->lock);
> > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> >
> >  		if (PTR_ERR(entry) == -EBUSY)
> >  			return VM_FAULT_NOPAGE;
> > @@ -315,12 +326,14 @@ static vm_fault_t sgx_vma_fault(struct vm_fault
> *vmf)
> >  				  vm_get_page_prot(page_prot_bits));
> >  	if (ret != VM_FAULT_NOPAGE) {
> >  		mutex_unlock(&encl->lock);
> > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> >
> >  		return VM_FAULT_SIGBUS;
> >  	}
> >
> >  	sgx_encl_test_and_clear_young(vma->vm_mm, entry);
> >  	mutex_unlock(&encl->lock);
> > +	srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> >
> >  	return VM_FAULT_NOPAGE;
> >  }
> > @@ -513,6 +526,7 @@ static int sgx_vma_access(struct vm_area_struct
> *vma, unsigned long addr,
> >  	struct sgx_encl_page *entry = NULL;
> >  	char data[sizeof(unsigned long)];
> >  	unsigned long align;
> > +	int srcu_idx;
> >  	int offset;
> >  	int cnt;
> >  	int ret = 0;
> > @@ -529,6 +543,12 @@ static int sgx_vma_access(struct vm_area_struct
> *vma, unsigned long addr,
> >  		return -EFAULT;
> >
> >  	for (i = 0; i < len; i += cnt) {
> > +		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> > +		if (sgx_epc_is_locked()) {
> > +			ret = -EBUSY;
> > +			goto out;
> > +		}
> > +
> >  		entry = sgx_encl_reserve_page(encl, (addr + i) &
> PAGE_MASK);
> >  		if (IS_ERR(entry)) {
> >  			ret = PTR_ERR(entry);
> > @@ -555,6 +575,7 @@ static int sgx_vma_access(struct vm_area_struct
> > *vma, unsigned long addr,
> >
> >  out:
> >  		mutex_unlock(&encl->lock);
> > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> >
> >  		if (ret)
> >  			break;
> > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c
> > b/arch/x86/kernel/cpu/sgx/ioctl.c index d8c3c07badb3..da3b569a10bd
> > 100644
> > --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> > @@ -147,6 +147,7 @@ static int sgx_encl_create(struct sgx_encl *encl,
> > struct sgx_secs *secs)  static long sgx_ioc_enclave_create(struct
> > sgx_encl *encl, void __user *arg)  {
> >  	struct sgx_enclave_create create_arg;
> > +	int srcu_idx;
> >  	void *secs;
> >  	int ret;
> >
> > @@ -162,9 +163,18 @@ static long sgx_ioc_enclave_create(struct
> > sgx_encl *encl, void __user *arg)
> >
> >  	if (copy_from_user(secs, (void __user *)create_arg.src, PAGE_SIZE))
> >  		ret = -EFAULT;
> > -	else
> > +	else {
> > +		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> > +		if (sgx_epc_is_locked()) {
> > +			srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > +			return -EBUSY;
> > +		}
> > +
> >  		ret = sgx_encl_create(encl, secs);
> >
> > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > +	}
> > +
> >  	kfree(secs);
> >  	return ret;
> >  }
> > @@ -444,6 +454,7 @@ static long sgx_ioc_enclave_add_pages(struct
> sgx_encl *encl, void __user *arg)
> >  	struct sgx_enclave_add_pages add_arg;
> >  	struct sgx_secinfo secinfo;
> >  	unsigned long c;
> > +	int srcu_idx;
> >  	int ret;
> >
> >  	if (!test_bit(SGX_ENCL_CREATED, &encl->flags) || @@ -477,8 +488,18
> > @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void
> __user *arg)
> >  		if (need_resched())
> >  			cond_resched();
> >
> > +		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> > +		if (sgx_epc_is_locked()) {
> > +			ret = -EBUSY;
> > +			srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > +			break;
> > +		}
> > +
> >  		ret = sgx_encl_add_page(encl, add_arg.src + c,
> add_arg.offset + c,
> >  					&secinfo, add_arg.flags);
> > +
> > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > +
> >  		if (ret)
> >  			break;
> >  	}
> > @@ -952,6 +973,7 @@ static long sgx_enclave_restrict_perm(struct
> sgx_encl *encl,
> >  	unsigned long addr;
> >  	unsigned long c;
> >  	void *epc_virt;
> > +	int srcu_idx;
> >  	int ret;
> >
> >  	memset(&secinfo, 0, sizeof(secinfo)); @@ -960,6 +982,12 @@ static
> > long sgx_enclave_restrict_perm(struct sgx_encl *encl,
> >  	vm_prot = vm_prot_from_secinfo(secinfo_perm);
> >
> >  	for (c = 0 ; c < modp->length; c += PAGE_SIZE) {
> > +		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> > +		if (sgx_epc_is_locked()) {
> > +			srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > +			return -EBUSY;
> > +		}
> > +
> >  		addr = encl->base + modp->offset + c;
> >
> >  		sgx_direct_reclaim();
> > @@ -1049,6 +1077,7 @@ static long sgx_enclave_restrict_perm(struct
> > sgx_encl *encl,
> >
> >  		sgx_mark_page_reclaimable(entry->epc_page);
> >  		mutex_unlock(&encl->lock);
> > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> >  	}
> >
> >  	ret = 0;
> > @@ -1060,6 +1089,7 @@ static long sgx_enclave_restrict_perm(struct
> sgx_encl *encl,
> >  	sgx_mark_page_reclaimable(entry->epc_page);
> >  out_unlock:
> >  	mutex_unlock(&encl->lock);
> > +	srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> >  out:
> >  	modp->count = c;
> >
> > @@ -1143,6 +1173,7 @@ static long sgx_enclave_modt(struct sgx_encl
> *encl,
> >  	unsigned long addr;
> >  	unsigned long c;
> >  	void *epc_virt;
> > +	int srcu_idx;
> >  	int ret;
> >
> >  	/*
> > @@ -1156,6 +1187,12 @@ static long sgx_enclave_modt(struct sgx_encl
> *encl,
> >  	secinfo.flags = page_type << 8;
> >
> >  	for (c = 0 ; c < modt->length; c += PAGE_SIZE) {
> > +		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> > +		if (sgx_epc_is_locked()) {
> > +			srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > +			return -EBUSY;
> > +		}
> > +
> >  		addr = encl->base + modt->offset + c;
> >
> >  		sgx_direct_reclaim();
> > @@ -1255,6 +1292,7 @@ static long sgx_enclave_modt(struct sgx_encl
> *encl,
> >  		entry->type = page_type;
> >
> >  		mutex_unlock(&encl->lock);
> > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> >  	}
> >
> >  	ret = 0;
> > @@ -1265,6 +1303,7 @@ static long sgx_enclave_modt(struct sgx_encl
> *encl,
> >  	entry->vm_run_prot_bits = run_prot_restore;
> >  out_unlock:
> >  	mutex_unlock(&encl->lock);
> > +	srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> >  out:
> >  	modt->count = c;
> >
> > @@ -1350,12 +1389,19 @@ static long sgx_encl_remove_pages(struct
> sgx_encl *encl,
> >  	unsigned long addr;
> >  	unsigned long c;
> >  	void *epc_virt;
> > +	int srcu_idx;
> >  	int ret;
> >
> >  	memset(&secinfo, 0, sizeof(secinfo));
> >  	secinfo.flags = SGX_SECINFO_R | SGX_SECINFO_W | SGX_SECINFO_X;
> >
> >  	for (c = 0 ; c < params->length; c += PAGE_SIZE) {
> > +		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> > +		if (sgx_epc_is_locked()) {
> > +			srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > +			return -EBUSY;
> > +		}
> > +
> >  		addr = encl->base + params->offset + c;
> >
> >  		sgx_direct_reclaim();
> > @@ -1411,6 +1457,7 @@ static long sgx_encl_remove_pages(struct
> sgx_encl *encl,
> >  		kfree(entry);
> >
> >  		mutex_unlock(&encl->lock);
> > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> >  	}
> >
> >  	ret = 0;
> > @@ -1418,6 +1465,7 @@ static long sgx_encl_remove_pages(struct
> > sgx_encl *encl,
> >
> >  out_unlock:
> >  	mutex_unlock(&encl->lock);
> > +	srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> >  out:
> >  	params->count = c;
> >
> > diff --git a/arch/x86/kernel/cpu/sgx/main.c
> > b/arch/x86/kernel/cpu/sgx/main.c index 545da16bb3ea..99c86b77ca8f
> > 100644
> > --- a/arch/x86/kernel/cpu/sgx/main.c
> > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > @@ -23,6 +23,17 @@ static int sgx_nr_epc_sections;  static struct
> > task_struct *ksgxd_tsk;  static DECLARE_WAIT_QUEUE_HEAD(ksgxd_waitq);
> >  static DEFINE_XARRAY(sgx_epc_address_space);
> > +/*
> > + * The flag sgx_epc_locked prevents any new SGX flows that
> > + * may attempt to allocate a new EPC page.
> > + */
> > +static bool __rcu sgx_epc_locked;
> > +/*
> > + * By synchronizing around sgx_epc_locked SRCU ensures that any
> > +executing
> > + * SGX flows have completed before proceeding with an SVN update. New
> > +SGX flows
> > + * will be prevented from starting during an SVN update.
> > + */
> > +DEFINE_SRCU(sgx_lock_epc_srcu);
> >
> >  /*
> >   * These variables are part of the state of the reclaimer, and must
> > be accessed @@ -378,6 +389,8 @@ void sgx_direct_reclaim(void)
> >
> >  static int ksgxd(void *p)
> >  {
> > +	int srcu_idx;
> > +
> >  	set_freezable();
> >
> >  	/*
> > @@ -398,9 +411,15 @@ static int ksgxd(void *p)
> >  				     kthread_should_stop() ||
> >
> sgx_should_reclaim(SGX_NR_HIGH_PAGES));
> >
> > +		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> > +		if (sgx_epc_is_locked())
> > +			goto maybe_resched;
> > +
> >  		if (sgx_should_reclaim(SGX_NR_HIGH_PAGES))
> >  			sgx_reclaim_pages();
> >
> > +maybe_resched:
> > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> >  		cond_resched();
> >  	}
> >
> > @@ -943,3 +962,21 @@ static int __init sgx_init(void)  }
> >
> >  device_initcall(sgx_init);
> > +
> > +static void sgx_lock_epc(void)
> > +{
> > +	sgx_epc_locked = true;
> > +	synchronize_srcu(&sgx_lock_epc_srcu);
> > +}
> > +
> > +static void sgx_unlock_epc(void)
> > +{
> > +	sgx_epc_locked = false;
> > +	synchronize_srcu(&sgx_lock_epc_srcu);
> > +}
> > +
> > +bool sgx_epc_is_locked(void)
> > +{
> > +	lockdep_assert_held(&sgx_lock_epc_srcu);
> > +	return sgx_epc_locked;
> > +}
> > diff --git a/arch/x86/kernel/cpu/sgx/virt.c
> > b/arch/x86/kernel/cpu/sgx/virt.c index 6a77a14eee38..e953816d7c8b
> > 100644
> > --- a/arch/x86/kernel/cpu/sgx/virt.c
> > +++ b/arch/x86/kernel/cpu/sgx/virt.c
> > @@ -75,10 +75,21 @@ static vm_fault_t sgx_vepc_fault(struct vm_fault
> > *vmf)  {
> >  	struct vm_area_struct *vma = vmf->vma;
> >  	struct sgx_vepc *vepc = vma->vm_private_data;
> > +	int srcu_idx;
> >  	int ret;
> >
> >  	mutex_lock(&vepc->lock);
> > +	srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> > +
> > +	if (sgx_epc_is_locked()) {
> > +		ret = -EBUSY;
> > +		goto out_unlock;
> > +	}
> > +
> >  	ret = __sgx_vepc_fault(vepc, vma, vmf->address);
> > +
> > +out_unlock:
> > +	srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> >  	mutex_unlock(&vepc->lock);
> >
> >  	if (!ret)
> > @@ -331,6 +342,7 @@ int __init sgx_vepc_init(void)  int
> > sgx_virt_ecreate(struct sgx_pageinfo *pageinfo, void __user *secs,
> >  		     int *trapnr)
> >  {
> > +	int srcu_idx;
> >  	int ret;
> >
> >  	/*
> > @@ -347,6 +359,12 @@ int sgx_virt_ecreate(struct sgx_pageinfo *pageinfo,
> void __user *secs,
> >  	if (WARN_ON_ONCE(!access_ok(secs, PAGE_SIZE)))
> >  		return -EINVAL;
> >
> > +	srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> > +	if (sgx_epc_is_locked()) {
> > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > +		return -EBUSY;
> > +	}
> > +
> >  	__uaccess_begin();
> >  	ret = __ecreate(pageinfo, (void *)secs);
> >  	__uaccess_end();
> > @@ -356,6 +374,8 @@ int sgx_virt_ecreate(struct sgx_pageinfo *pageinfo,
> void __user *secs,
> >  		return -EFAULT;
> >  	}
> >
> > +	srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > +
> >  	/* ECREATE doesn't return an error code, it faults or succeeds. */
> >  	WARN_ON_ONCE(ret);
> >  	return 0;
> > --
> > 2.17.1
> >
> 
> This series is broken. It does not apply on top of tip/x86/sgx. Also you
> forgot CC to me.

I'm sincerely sorry for not cc to you, I'm aware of that after sending out :- )

Yes, it requires to apply the following SGX2 v2 patches to tip/x86/sgx, prior to
this series. As I mentioned in cover letter, SGX2 introduces new SGX instructions
and flows, which should be handled during SVN update.

The v2 of SGX2 support patch set, with 32 patches in total, refer to the cover letter:
https://lore.kernel.org/lkml/cover.1644274683.git.reinette.chatre@intel.com/
The two additional patches SGX2 support requires:
"x86/sgx: Add poison handling to reclaimer"
 https://lore.kernel.org/linux-sgx/dcc95eb2aaefb042527ac50d0a50738c7c160dac.1643830353.git.reinette.chatre@intel.com/
"x86/sgx: Silence softlockup detection when releasing large enclaves"
 https://lore.kernel.org/linux-sgx/b5e9f218064aa76e3026f778e1ad0a1d823e3db8.1643133224.git.reinette.chatre@intel.com/

> 
> BR, Jarkko

Re: [RFC PATCH v2 01/10] x86/sgx: Introduce mechanism to prevent new initializations of EPC pages

[Jarkko Sakkinen]

On Thu, Mar 17, 2022 at 02:08:21PM +0000, Zhang, Cathy wrote:
> Hi Jarkko,
> 
> > -----Original Message-----
> > From: Jarkko Sakkinen <jarkko@kernel.org>
> > Sent: Thursday, March 17, 2022 12:13 PM
> > To: Zhang, Cathy <cathy.zhang@intel.com>
> > Cc: linux-sgx@vger.kernel.org; linux-kernel@vger.kernel.org; Hansen, Dave
> > <dave.hansen@intel.com>; Raj, Ashok <ashok.raj@intel.com>
> > Subject: Re: [RFC PATCH v2 01/10] x86/sgx: Introduce mechanism to prevent
> > new initializations of EPC pages
> > 
> > On Tue, Mar 15, 2022 at 09:02:51AM +0800, Cathy Zhang wrote:
> > > == Background ==
> > >
> > > EUPDATESVN is a new SGX instruction which allows enclave attestation
> > > to include information about updated microcode without a reboot.
> > >
> > > The SGX hardware maintains metadata for each enclave page to help
> > > enforce its security guarantees. This includes things like a record of
> > > the enclave to which the page belongs and the type of the page:
> > > SGX metadata like "VA" or "SECS" pages, or regular enclave pages like
> > > those that store user data.
> > >
> > > Before an EUPDATESVN operation can be successful, all SGX memory (aka.
> > > EPC) must be marked as "unused" in the SGX hardware metadata (aka,
> > > EPCM). The SGX microcode now maintains a reference count of pages
> > > which are unused to aid in determining when all pages reach the
> > > "unused" state.
> > >
> > > Both bare-metal and KVM guest EPC must be made unused. To increase the
> > > chance of a successful EUPDATESVN, the kernel prevents existing
> > > enclaves from creating new, valid pages and prevents new enclave
> > > creation (creating an enclave involves initializing a "SECS" page).
> > >
> > > The entire EUPDATESVN process is very slow since it potentially
> > > affects gigabytes of enclave memory. It can potentially take seconds
> > > or minutes to complete. Userspace may encounter -EBUSY errors during
> > > the update and is expected to retry.
> > >
> > > == Patch contents ==
> > >
> > > Introduce mechanism to prevent new initializations of EPC pages.
> > >
> > > Use a flag to indicate when SGX EPC pages are "locked", which means
> > > it's not allowed to allocate new EPC page for use. Check it in all
> > > paths that can initialize an EPC page. Use SRCU to ensure that the
> > > flag is visible across the system before proceeding with an update.
> > >
> > > Add checks to all sites that call SGX instructions that can transition
> > > pages from unused to initialized to ensure that the SRCU lock is held.
> > >
> > > Signed-off-by: Cathy Zhang <cathy.zhang@intel.com>
> > > ---
> > >  arch/x86/kernel/cpu/sgx/encls.h | 10 +++++++
> > >  arch/x86/kernel/cpu/sgx/sgx.h   |  3 ++
> > >  arch/x86/kernel/cpu/sgx/encl.c  | 25 +++++++++++++++--
> > > arch/x86/kernel/cpu/sgx/ioctl.c | 50
> > ++++++++++++++++++++++++++++++++-
> > > arch/x86/kernel/cpu/sgx/main.c  | 37 ++++++++++++++++++++++++
> > > arch/x86/kernel/cpu/sgx/virt.c  | 20 +++++++++++++
> > >  6 files changed, 142 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/arch/x86/kernel/cpu/sgx/encls.h
> > > b/arch/x86/kernel/cpu/sgx/encls.h index 99004b02e2ed..3f1797ec2445
> > > 100644
> > > --- a/arch/x86/kernel/cpu/sgx/encls.h
> > > +++ b/arch/x86/kernel/cpu/sgx/encls.h
> > > @@ -139,6 +139,8 @@ static inline bool encls_failed(int ret)
> > >  /* Initialize an EPC page into an SGX Enclave Control Structure
> > > (SECS) page. */  static inline int __ecreate(struct sgx_pageinfo
> > > *pginfo, void *secs)  {
> > > +	lockdep_assert_held(&sgx_lock_epc_srcu);
> > > +
> > >  	return __encls_2(ECREATE, pginfo, secs);  }
> > >
> > > @@ -154,6 +156,8 @@ static inline int __eextend(void *secs, void *addr)
> > >   */
> > >  static inline int __eadd(struct sgx_pageinfo *pginfo, void *addr)  {
> > > +	lockdep_assert_held(&sgx_lock_epc_srcu);
> > > +
> > >  	return __encls_2(EADD, pginfo, addr);  }
> > >
> > > @@ -191,6 +195,8 @@ static inline int __etrack(void *addr)  static
> > > inline int __eldu(struct sgx_pageinfo *pginfo, void *addr,
> > >  			 void *va)
> > >  {
> > > +	lockdep_assert_held(&sgx_lock_epc_srcu);
> > > +
> > >  	return __encls_ret_3(ELDU, pginfo, addr, va);  }
> > >
> > > @@ -205,6 +211,8 @@ static inline int __epa(void *addr)  {
> > >  	unsigned long rbx = SGX_PAGE_TYPE_VA;
> > >
> > > +	lockdep_assert_held(&sgx_lock_epc_srcu);
> > > +
> > >  	return __encls_2(EPA, rbx, addr);
> > >  }
> > >
> > > @@ -230,6 +238,8 @@ static inline int __emodt(struct sgx_secinfo
> > > *secinfo, void *addr)
> > >  /* Zero a page of EPC memory and add it to an initialized enclave. */
> > > static inline int __eaug(struct sgx_pageinfo *pginfo, void *addr)  {
> > > +	lockdep_assert_held(&sgx_lock_epc_srcu);
> > > +
> > >  	return __encls_2(EAUG, pginfo, addr);  }
> > >
> > > diff --git a/arch/x86/kernel/cpu/sgx/sgx.h
> > > b/arch/x86/kernel/cpu/sgx/sgx.h index 85cbf103b0dd..dc8cb58100e3
> > > 100644
> > > --- a/arch/x86/kernel/cpu/sgx/sgx.h
> > > +++ b/arch/x86/kernel/cpu/sgx/sgx.h
> > > @@ -104,4 +104,7 @@ static inline int __init sgx_vepc_init(void)
> > >
> > >  void sgx_update_lepubkeyhash(u64 *lepubkeyhash);
> > >
> > > +extern struct srcu_struct sgx_lock_epc_srcu; bool
> > > +sgx_epc_is_locked(void);
> > > +
> > >  #endif /* _X86_SGX_H */
> > > diff --git a/arch/x86/kernel/cpu/sgx/encl.c
> > > b/arch/x86/kernel/cpu/sgx/encl.c index a0255d41e075..d2b428992910
> > > 100644
> > > --- a/arch/x86/kernel/cpu/sgx/encl.c
> > > +++ b/arch/x86/kernel/cpu/sgx/encl.c
> > > @@ -264,6 +264,7 @@ static vm_fault_t sgx_vma_fault(struct vm_fault
> > *vmf)
> > >  	unsigned long phys_addr;
> > >  	struct sgx_encl *encl;
> > >  	vm_fault_t ret;
> > > +	int srcu_idx;
> > >
> > >  	encl = vma->vm_private_data;
> > >
> > > @@ -275,6 +276,12 @@ static vm_fault_t sgx_vma_fault(struct vm_fault
> > *vmf)
> > >  	if (unlikely(!encl))
> > >  		return VM_FAULT_SIGBUS;
> > >
> > > +	srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> > > +	if (sgx_epc_is_locked()) {
> > > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > > +		return VM_FAULT_SIGBUS;
> > > +	}
> > > +
> > >  	/*
> > >  	 * The page_array keeps track of all enclave pages, whether they
> > >  	 * are swapped out or not. If there is no entry for this page and @@
> > > -283,14 +290,18 @@ static vm_fault_t sgx_vma_fault(struct vm_fault *vmf)
> > >  	 * enclave that will be checked for right away.
> > >  	 */
> > >  	if (cpu_feature_enabled(X86_FEATURE_SGX2) &&
> > > -	    (!xa_load(&encl->page_array, PFN_DOWN(addr))))
> > > -		return sgx_encl_eaug_page(vma, encl, addr);
> > > +	    (!xa_load(&encl->page_array, PFN_DOWN(addr)))) {
> > > +		ret = sgx_encl_eaug_page(vma, encl, addr);
> > > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > > +		return ret;
> > > +	}
> > >
> > >  	mutex_lock(&encl->lock);
> > >
> > >  	entry = sgx_encl_load_page(encl, addr);
> > >  	if (IS_ERR(entry)) {
> > >  		mutex_unlock(&encl->lock);
> > > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > >
> > >  		if (PTR_ERR(entry) == -EBUSY)
> > >  			return VM_FAULT_NOPAGE;
> > > @@ -315,12 +326,14 @@ static vm_fault_t sgx_vma_fault(struct vm_fault
> > *vmf)
> > >  				  vm_get_page_prot(page_prot_bits));
> > >  	if (ret != VM_FAULT_NOPAGE) {
> > >  		mutex_unlock(&encl->lock);
> > > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > >
> > >  		return VM_FAULT_SIGBUS;
> > >  	}
> > >
> > >  	sgx_encl_test_and_clear_young(vma->vm_mm, entry);
> > >  	mutex_unlock(&encl->lock);
> > > +	srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > >
> > >  	return VM_FAULT_NOPAGE;
> > >  }
> > > @@ -513,6 +526,7 @@ static int sgx_vma_access(struct vm_area_struct
> > *vma, unsigned long addr,
> > >  	struct sgx_encl_page *entry = NULL;
> > >  	char data[sizeof(unsigned long)];
> > >  	unsigned long align;
> > > +	int srcu_idx;
> > >  	int offset;
> > >  	int cnt;
> > >  	int ret = 0;
> > > @@ -529,6 +543,12 @@ static int sgx_vma_access(struct vm_area_struct
> > *vma, unsigned long addr,
> > >  		return -EFAULT;
> > >
> > >  	for (i = 0; i < len; i += cnt) {
> > > +		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> > > +		if (sgx_epc_is_locked()) {
> > > +			ret = -EBUSY;
> > > +			goto out;
> > > +		}
> > > +
> > >  		entry = sgx_encl_reserve_page(encl, (addr + i) &
> > PAGE_MASK);
> > >  		if (IS_ERR(entry)) {
> > >  			ret = PTR_ERR(entry);
> > > @@ -555,6 +575,7 @@ static int sgx_vma_access(struct vm_area_struct
> > > *vma, unsigned long addr,
> > >
> > >  out:
> > >  		mutex_unlock(&encl->lock);
> > > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > >
> > >  		if (ret)
> > >  			break;
> > > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c
> > > b/arch/x86/kernel/cpu/sgx/ioctl.c index d8c3c07badb3..da3b569a10bd
> > > 100644
> > > --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> > > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> > > @@ -147,6 +147,7 @@ static int sgx_encl_create(struct sgx_encl *encl,
> > > struct sgx_secs *secs)  static long sgx_ioc_enclave_create(struct
> > > sgx_encl *encl, void __user *arg)  {
> > >  	struct sgx_enclave_create create_arg;
> > > +	int srcu_idx;
> > >  	void *secs;
> > >  	int ret;
> > >
> > > @@ -162,9 +163,18 @@ static long sgx_ioc_enclave_create(struct
> > > sgx_encl *encl, void __user *arg)
> > >
> > >  	if (copy_from_user(secs, (void __user *)create_arg.src, PAGE_SIZE))
> > >  		ret = -EFAULT;
> > > -	else
> > > +	else {
> > > +		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> > > +		if (sgx_epc_is_locked()) {
> > > +			srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > > +			return -EBUSY;
> > > +		}
> > > +
> > >  		ret = sgx_encl_create(encl, secs);
> > >
> > > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > > +	}
> > > +
> > >  	kfree(secs);
> > >  	return ret;
> > >  }
> > > @@ -444,6 +454,7 @@ static long sgx_ioc_enclave_add_pages(struct
> > sgx_encl *encl, void __user *arg)
> > >  	struct sgx_enclave_add_pages add_arg;
> > >  	struct sgx_secinfo secinfo;
> > >  	unsigned long c;
> > > +	int srcu_idx;
> > >  	int ret;
> > >
> > >  	if (!test_bit(SGX_ENCL_CREATED, &encl->flags) || @@ -477,8 +488,18
> > > @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void
> > __user *arg)
> > >  		if (need_resched())
> > >  			cond_resched();
> > >
> > > +		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> > > +		if (sgx_epc_is_locked()) {
> > > +			ret = -EBUSY;
> > > +			srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > > +			break;
> > > +		}
> > > +
> > >  		ret = sgx_encl_add_page(encl, add_arg.src + c,
> > add_arg.offset + c,
> > >  					&secinfo, add_arg.flags);
> > > +
> > > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > > +
> > >  		if (ret)
> > >  			break;
> > >  	}
> > > @@ -952,6 +973,7 @@ static long sgx_enclave_restrict_perm(struct
> > sgx_encl *encl,
> > >  	unsigned long addr;
> > >  	unsigned long c;
> > >  	void *epc_virt;
> > > +	int srcu_idx;
> > >  	int ret;
> > >
> > >  	memset(&secinfo, 0, sizeof(secinfo)); @@ -960,6 +982,12 @@ static
> > > long sgx_enclave_restrict_perm(struct sgx_encl *encl,
> > >  	vm_prot = vm_prot_from_secinfo(secinfo_perm);
> > >
> > >  	for (c = 0 ; c < modp->length; c += PAGE_SIZE) {
> > > +		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> > > +		if (sgx_epc_is_locked()) {
> > > +			srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > > +			return -EBUSY;
> > > +		}
> > > +
> > >  		addr = encl->base + modp->offset + c;
> > >
> > >  		sgx_direct_reclaim();
> > > @@ -1049,6 +1077,7 @@ static long sgx_enclave_restrict_perm(struct
> > > sgx_encl *encl,
> > >
> > >  		sgx_mark_page_reclaimable(entry->epc_page);
> > >  		mutex_unlock(&encl->lock);
> > > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > >  	}
> > >
> > >  	ret = 0;
> > > @@ -1060,6 +1089,7 @@ static long sgx_enclave_restrict_perm(struct
> > sgx_encl *encl,
> > >  	sgx_mark_page_reclaimable(entry->epc_page);
> > >  out_unlock:
> > >  	mutex_unlock(&encl->lock);
> > > +	srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > >  out:
> > >  	modp->count = c;
> > >
> > > @@ -1143,6 +1173,7 @@ static long sgx_enclave_modt(struct sgx_encl
> > *encl,
> > >  	unsigned long addr;
> > >  	unsigned long c;
> > >  	void *epc_virt;
> > > +	int srcu_idx;
> > >  	int ret;
> > >
> > >  	/*
> > > @@ -1156,6 +1187,12 @@ static long sgx_enclave_modt(struct sgx_encl
> > *encl,
> > >  	secinfo.flags = page_type << 8;
> > >
> > >  	for (c = 0 ; c < modt->length; c += PAGE_SIZE) {
> > > +		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> > > +		if (sgx_epc_is_locked()) {
> > > +			srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > > +			return -EBUSY;
> > > +		}
> > > +
> > >  		addr = encl->base + modt->offset + c;
> > >
> > >  		sgx_direct_reclaim();
> > > @@ -1255,6 +1292,7 @@ static long sgx_enclave_modt(struct sgx_encl
> > *encl,
> > >  		entry->type = page_type;
> > >
> > >  		mutex_unlock(&encl->lock);
> > > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > >  	}
> > >
> > >  	ret = 0;
> > > @@ -1265,6 +1303,7 @@ static long sgx_enclave_modt(struct sgx_encl
> > *encl,
> > >  	entry->vm_run_prot_bits = run_prot_restore;
> > >  out_unlock:
> > >  	mutex_unlock(&encl->lock);
> > > +	srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > >  out:
> > >  	modt->count = c;
> > >
> > > @@ -1350,12 +1389,19 @@ static long sgx_encl_remove_pages(struct
> > sgx_encl *encl,
> > >  	unsigned long addr;
> > >  	unsigned long c;
> > >  	void *epc_virt;
> > > +	int srcu_idx;
> > >  	int ret;
> > >
> > >  	memset(&secinfo, 0, sizeof(secinfo));
> > >  	secinfo.flags = SGX_SECINFO_R | SGX_SECINFO_W | SGX_SECINFO_X;
> > >
> > >  	for (c = 0 ; c < params->length; c += PAGE_SIZE) {
> > > +		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> > > +		if (sgx_epc_is_locked()) {
> > > +			srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > > +			return -EBUSY;
> > > +		}
> > > +
> > >  		addr = encl->base + params->offset + c;
> > >
> > >  		sgx_direct_reclaim();
> > > @@ -1411,6 +1457,7 @@ static long sgx_encl_remove_pages(struct
> > sgx_encl *encl,
> > >  		kfree(entry);
> > >
> > >  		mutex_unlock(&encl->lock);
> > > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > >  	}
> > >
> > >  	ret = 0;
> > > @@ -1418,6 +1465,7 @@ static long sgx_encl_remove_pages(struct
> > > sgx_encl *encl,
> > >
> > >  out_unlock:
> > >  	mutex_unlock(&encl->lock);
> > > +	srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > >  out:
> > >  	params->count = c;
> > >
> > > diff --git a/arch/x86/kernel/cpu/sgx/main.c
> > > b/arch/x86/kernel/cpu/sgx/main.c index 545da16bb3ea..99c86b77ca8f
> > > 100644
> > > --- a/arch/x86/kernel/cpu/sgx/main.c
> > > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > > @@ -23,6 +23,17 @@ static int sgx_nr_epc_sections;  static struct
> > > task_struct *ksgxd_tsk;  static DECLARE_WAIT_QUEUE_HEAD(ksgxd_waitq);
> > >  static DEFINE_XARRAY(sgx_epc_address_space);
> > > +/*
> > > + * The flag sgx_epc_locked prevents any new SGX flows that
> > > + * may attempt to allocate a new EPC page.
> > > + */
> > > +static bool __rcu sgx_epc_locked;
> > > +/*
> > > + * By synchronizing around sgx_epc_locked SRCU ensures that any
> > > +executing
> > > + * SGX flows have completed before proceeding with an SVN update. New
> > > +SGX flows
> > > + * will be prevented from starting during an SVN update.
> > > + */
> > > +DEFINE_SRCU(sgx_lock_epc_srcu);
> > >
> > >  /*
> > >   * These variables are part of the state of the reclaimer, and must
> > > be accessed @@ -378,6 +389,8 @@ void sgx_direct_reclaim(void)
> > >
> > >  static int ksgxd(void *p)
> > >  {
> > > +	int srcu_idx;
> > > +
> > >  	set_freezable();
> > >
> > >  	/*
> > > @@ -398,9 +411,15 @@ static int ksgxd(void *p)
> > >  				     kthread_should_stop() ||
> > >
> > sgx_should_reclaim(SGX_NR_HIGH_PAGES));
> > >
> > > +		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> > > +		if (sgx_epc_is_locked())
> > > +			goto maybe_resched;
> > > +
> > >  		if (sgx_should_reclaim(SGX_NR_HIGH_PAGES))
> > >  			sgx_reclaim_pages();
> > >
> > > +maybe_resched:
> > > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > >  		cond_resched();
> > >  	}
> > >
> > > @@ -943,3 +962,21 @@ static int __init sgx_init(void)  }
> > >
> > >  device_initcall(sgx_init);
> > > +
> > > +static void sgx_lock_epc(void)
> > > +{
> > > +	sgx_epc_locked = true;
> > > +	synchronize_srcu(&sgx_lock_epc_srcu);
> > > +}
> > > +
> > > +static void sgx_unlock_epc(void)
> > > +{
> > > +	sgx_epc_locked = false;
> > > +	synchronize_srcu(&sgx_lock_epc_srcu);
> > > +}
> > > +
> > > +bool sgx_epc_is_locked(void)
> > > +{
> > > +	lockdep_assert_held(&sgx_lock_epc_srcu);
> > > +	return sgx_epc_locked;
> > > +}
> > > diff --git a/arch/x86/kernel/cpu/sgx/virt.c
> > > b/arch/x86/kernel/cpu/sgx/virt.c index 6a77a14eee38..e953816d7c8b
> > > 100644
> > > --- a/arch/x86/kernel/cpu/sgx/virt.c
> > > +++ b/arch/x86/kernel/cpu/sgx/virt.c
> > > @@ -75,10 +75,21 @@ static vm_fault_t sgx_vepc_fault(struct vm_fault
> > > *vmf)  {
> > >  	struct vm_area_struct *vma = vmf->vma;
> > >  	struct sgx_vepc *vepc = vma->vm_private_data;
> > > +	int srcu_idx;
> > >  	int ret;
> > >
> > >  	mutex_lock(&vepc->lock);
> > > +	srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> > > +
> > > +	if (sgx_epc_is_locked()) {
> > > +		ret = -EBUSY;
> > > +		goto out_unlock;
> > > +	}
> > > +
> > >  	ret = __sgx_vepc_fault(vepc, vma, vmf->address);
> > > +
> > > +out_unlock:
> > > +	srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > >  	mutex_unlock(&vepc->lock);
> > >
> > >  	if (!ret)
> > > @@ -331,6 +342,7 @@ int __init sgx_vepc_init(void)  int
> > > sgx_virt_ecreate(struct sgx_pageinfo *pageinfo, void __user *secs,
> > >  		     int *trapnr)
> > >  {
> > > +	int srcu_idx;
> > >  	int ret;
> > >
> > >  	/*
> > > @@ -347,6 +359,12 @@ int sgx_virt_ecreate(struct sgx_pageinfo *pageinfo,
> > void __user *secs,
> > >  	if (WARN_ON_ONCE(!access_ok(secs, PAGE_SIZE)))
> > >  		return -EINVAL;
> > >
> > > +	srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> > > +	if (sgx_epc_is_locked()) {
> > > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > > +		return -EBUSY;
> > > +	}
> > > +
> > >  	__uaccess_begin();
> > >  	ret = __ecreate(pageinfo, (void *)secs);
> > >  	__uaccess_end();
> > > @@ -356,6 +374,8 @@ int sgx_virt_ecreate(struct sgx_pageinfo *pageinfo,
> > void __user *secs,
> > >  		return -EFAULT;
> > >  	}
> > >
> > > +	srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > > +
> > >  	/* ECREATE doesn't return an error code, it faults or succeeds. */
> > >  	WARN_ON_ONCE(ret);
> > >  	return 0;
> > > --
> > > 2.17.1
> > >
> > 
> > This series is broken. It does not apply on top of tip/x86/sgx. Also you
> > forgot CC to me.
> 
> I'm sincerely sorry for not cc to you, I'm aware of that after sending out :- )

Apologies, I did not mean it like that. Just mentioning that I can look
into this :-)

> Yes, it requires to apply the following SGX2 v2 patches to tip/x86/sgx, prior to
> this series. As I mentioned in cover letter, SGX2 introduces new SGX instructions
> and flows, which should be handled during SVN update.
> 
> The v2 of SGX2 support patch set, with 32 patches in total, refer to the cover letter:
> https://lore.kernel.org/lkml/cover.1644274683.git.reinette.chatre@intel.com/
> The two additional patches SGX2 support requires:
> "x86/sgx: Add poison handling to reclaimer"
>  https://lore.kernel.org/linux-sgx/dcc95eb2aaefb042527ac50d0a50738c7c160dac.1643830353.git.reinette.chatre@intel.com/
> "x86/sgx: Silence softlockup detection when releasing large enclaves"
>  https://lore.kernel.org/linux-sgx/b5e9f218064aa76e3026f778e1ad0a1d823e3db8.1643133224.git.reinette.chatre@intel.com/

Why don't you pick the dependent patches from that patch set and put them
to this one? It does not matter if two patch sets contain same patches. If
this gets merged first, then Reinette's job will be easier, and vice versa.
Tags make sure that everyone gets what they deserve from their
contribution.

Otherwise for instance giving a tested-by is impossible. You have to root
this to the mainline, or at least to x86 tip.git.

BR, Jarkko

RE: [RFC PATCH v2 01/10] x86/sgx: Introduce mechanism to prevent new initializations of EPC pages

[Zhang, Cathy]

Hi Jarkko,

> -----Original Message-----
> From: Jarkko Sakkinen <jarkko@kernel.org>
> Sent: Friday, March 18, 2022 2:53 AM
> To: Zhang, Cathy <cathy.zhang@intel.com>
> Cc: linux-sgx@vger.kernel.org; linux-kernel@vger.kernel.org; Hansen, Dave
> <dave.hansen@intel.com>; Raj, Ashok <ashok.raj@intel.com>
> Subject: Re: [RFC PATCH v2 01/10] x86/sgx: Introduce mechanism to prevent
> new initializations of EPC pages
> 
> On Thu, Mar 17, 2022 at 02:08:21PM +0000, Zhang, Cathy wrote:
> > Hi Jarkko,
> >
> > > -----Original Message-----
> > > From: Jarkko Sakkinen <jarkko@kernel.org>
> > > Sent: Thursday, March 17, 2022 12:13 PM
> > > To: Zhang, Cathy <cathy.zhang@intel.com>
> > > Cc: linux-sgx@vger.kernel.org; linux-kernel@vger.kernel.org; Hansen,
> > > Dave <dave.hansen@intel.com>; Raj, Ashok <ashok.raj@intel.com>
> > > Subject: Re: [RFC PATCH v2 01/10] x86/sgx: Introduce mechanism to
> > > prevent new initializations of EPC pages
> > >
> > > On Tue, Mar 15, 2022 at 09:02:51AM +0800, Cathy Zhang wrote:
> > > > == Background ==
> > > >
> > > > EUPDATESVN is a new SGX instruction which allows enclave
> > > > attestation to include information about updated microcode without a
> reboot.
> > > >
> > > > The SGX hardware maintains metadata for each enclave page to help
> > > > enforce its security guarantees. This includes things like a
> > > > record of the enclave to which the page belongs and the type of the
> page:
> > > > SGX metadata like "VA" or "SECS" pages, or regular enclave pages
> > > > like those that store user data.
> > > >
> > > > Before an EUPDATESVN operation can be successful, all SGX memory
> (aka.
> > > > EPC) must be marked as "unused" in the SGX hardware metadata (aka,
> > > > EPCM). The SGX microcode now maintains a reference count of pages
> > > > which are unused to aid in determining when all pages reach the
> > > > "unused" state.
> > > >
> > > > Both bare-metal and KVM guest EPC must be made unused. To increase
> > > > the chance of a successful EUPDATESVN, the kernel prevents
> > > > existing enclaves from creating new, valid pages and prevents new
> > > > enclave creation (creating an enclave involves initializing a "SECS" page).
> > > >
> > > > The entire EUPDATESVN process is very slow since it potentially
> > > > affects gigabytes of enclave memory. It can potentially take
> > > > seconds or minutes to complete. Userspace may encounter -EBUSY
> > > > errors during the update and is expected to retry.
> > > >
> > > > == Patch contents ==
> > > >
> > > > Introduce mechanism to prevent new initializations of EPC pages.
> > > >
> > > > Use a flag to indicate when SGX EPC pages are "locked", which
> > > > means it's not allowed to allocate new EPC page for use. Check it
> > > > in all paths that can initialize an EPC page. Use SRCU to ensure
> > > > that the flag is visible across the system before proceeding with an
> update.
> > > >
> > > > Add checks to all sites that call SGX instructions that can
> > > > transition pages from unused to initialized to ensure that the SRCU lock
> is held.
> > > >
> > > > Signed-off-by: Cathy Zhang <cathy.zhang@intel.com>
> > > > ---
> > > >  arch/x86/kernel/cpu/sgx/encls.h | 10 +++++++
> > > >  arch/x86/kernel/cpu/sgx/sgx.h   |  3 ++
> > > >  arch/x86/kernel/cpu/sgx/encl.c  | 25 +++++++++++++++--
> > > > arch/x86/kernel/cpu/sgx/ioctl.c | 50
> > > ++++++++++++++++++++++++++++++++-
> > > > arch/x86/kernel/cpu/sgx/main.c  | 37 ++++++++++++++++++++++++
> > > > arch/x86/kernel/cpu/sgx/virt.c  | 20 +++++++++++++
> > > >  6 files changed, 142 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/arch/x86/kernel/cpu/sgx/encls.h
> > > > b/arch/x86/kernel/cpu/sgx/encls.h index 99004b02e2ed..3f1797ec2445
> > > > 100644
> > > > --- a/arch/x86/kernel/cpu/sgx/encls.h
> > > > +++ b/arch/x86/kernel/cpu/sgx/encls.h
> > > > @@ -139,6 +139,8 @@ static inline bool encls_failed(int ret)
> > > >  /* Initialize an EPC page into an SGX Enclave Control Structure
> > > > (SECS) page. */  static inline int __ecreate(struct sgx_pageinfo
> > > > *pginfo, void *secs)  {
> > > > +	lockdep_assert_held(&sgx_lock_epc_srcu);
> > > > +
> > > >  	return __encls_2(ECREATE, pginfo, secs);  }
> > > >
> > > > @@ -154,6 +156,8 @@ static inline int __eextend(void *secs, void
> *addr)
> > > >   */
> > > >  static inline int __eadd(struct sgx_pageinfo *pginfo, void *addr)
> > > > {
> > > > +	lockdep_assert_held(&sgx_lock_epc_srcu);
> > > > +
> > > >  	return __encls_2(EADD, pginfo, addr);  }
> > > >
> > > > @@ -191,6 +195,8 @@ static inline int __etrack(void *addr)  static
> > > > inline int __eldu(struct sgx_pageinfo *pginfo, void *addr,
> > > >  			 void *va)
> > > >  {
> > > > +	lockdep_assert_held(&sgx_lock_epc_srcu);
> > > > +
> > > >  	return __encls_ret_3(ELDU, pginfo, addr, va);  }
> > > >
> > > > @@ -205,6 +211,8 @@ static inline int __epa(void *addr)  {
> > > >  	unsigned long rbx = SGX_PAGE_TYPE_VA;
> > > >
> > > > +	lockdep_assert_held(&sgx_lock_epc_srcu);
> > > > +
> > > >  	return __encls_2(EPA, rbx, addr);  }
> > > >
> > > > @@ -230,6 +238,8 @@ static inline int __emodt(struct sgx_secinfo
> > > > *secinfo, void *addr)
> > > >  /* Zero a page of EPC memory and add it to an initialized
> > > > enclave. */ static inline int __eaug(struct sgx_pageinfo *pginfo,
> > > > void *addr)  {
> > > > +	lockdep_assert_held(&sgx_lock_epc_srcu);
> > > > +
> > > >  	return __encls_2(EAUG, pginfo, addr);  }
> > > >
> > > > diff --git a/arch/x86/kernel/cpu/sgx/sgx.h
> > > > b/arch/x86/kernel/cpu/sgx/sgx.h index 85cbf103b0dd..dc8cb58100e3
> > > > 100644
> > > > --- a/arch/x86/kernel/cpu/sgx/sgx.h
> > > > +++ b/arch/x86/kernel/cpu/sgx/sgx.h
> > > > @@ -104,4 +104,7 @@ static inline int __init sgx_vepc_init(void)
> > > >
> > > >  void sgx_update_lepubkeyhash(u64 *lepubkeyhash);
> > > >
> > > > +extern struct srcu_struct sgx_lock_epc_srcu; bool
> > > > +sgx_epc_is_locked(void);
> > > > +
> > > >  #endif /* _X86_SGX_H */
> > > > diff --git a/arch/x86/kernel/cpu/sgx/encl.c
> > > > b/arch/x86/kernel/cpu/sgx/encl.c index a0255d41e075..d2b428992910
> > > > 100644
> > > > --- a/arch/x86/kernel/cpu/sgx/encl.c
> > > > +++ b/arch/x86/kernel/cpu/sgx/encl.c
> > > > @@ -264,6 +264,7 @@ static vm_fault_t sgx_vma_fault(struct
> > > > vm_fault
> > > *vmf)
> > > >  	unsigned long phys_addr;
> > > >  	struct sgx_encl *encl;
> > > >  	vm_fault_t ret;
> > > > +	int srcu_idx;
> > > >
> > > >  	encl = vma->vm_private_data;
> > > >
> > > > @@ -275,6 +276,12 @@ static vm_fault_t sgx_vma_fault(struct
> > > > vm_fault
> > > *vmf)
> > > >  	if (unlikely(!encl))
> > > >  		return VM_FAULT_SIGBUS;
> > > >
> > > > +	srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> > > > +	if (sgx_epc_is_locked()) {
> > > > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > > > +		return VM_FAULT_SIGBUS;
> > > > +	}
> > > > +
> > > >  	/*
> > > >  	 * The page_array keeps track of all enclave pages, whether they
> > > >  	 * are swapped out or not. If there is no entry for this page
> > > > and @@
> > > > -283,14 +290,18 @@ static vm_fault_t sgx_vma_fault(struct vm_fault
> *vmf)
> > > >  	 * enclave that will be checked for right away.
> > > >  	 */
> > > >  	if (cpu_feature_enabled(X86_FEATURE_SGX2) &&
> > > > -	    (!xa_load(&encl->page_array, PFN_DOWN(addr))))
> > > > -		return sgx_encl_eaug_page(vma, encl, addr);
> > > > +	    (!xa_load(&encl->page_array, PFN_DOWN(addr)))) {
> > > > +		ret = sgx_encl_eaug_page(vma, encl, addr);
> > > > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > > > +		return ret;
> > > > +	}
> > > >
> > > >  	mutex_lock(&encl->lock);
> > > >
> > > >  	entry = sgx_encl_load_page(encl, addr);
> > > >  	if (IS_ERR(entry)) {
> > > >  		mutex_unlock(&encl->lock);
> > > > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > > >
> > > >  		if (PTR_ERR(entry) == -EBUSY)
> > > >  			return VM_FAULT_NOPAGE;
> > > > @@ -315,12 +326,14 @@ static vm_fault_t sgx_vma_fault(struct
> > > > vm_fault
> > > *vmf)
> > > >  				  vm_get_page_prot(page_prot_bits));
> > > >  	if (ret != VM_FAULT_NOPAGE) {
> > > >  		mutex_unlock(&encl->lock);
> > > > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > > >
> > > >  		return VM_FAULT_SIGBUS;
> > > >  	}
> > > >
> > > >  	sgx_encl_test_and_clear_young(vma->vm_mm, entry);
> > > >  	mutex_unlock(&encl->lock);
> > > > +	srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > > >
> > > >  	return VM_FAULT_NOPAGE;
> > > >  }
> > > > @@ -513,6 +526,7 @@ static int sgx_vma_access(struct
> > > > vm_area_struct
> > > *vma, unsigned long addr,
> > > >  	struct sgx_encl_page *entry = NULL;
> > > >  	char data[sizeof(unsigned long)];
> > > >  	unsigned long align;
> > > > +	int srcu_idx;
> > > >  	int offset;
> > > >  	int cnt;
> > > >  	int ret = 0;
> > > > @@ -529,6 +543,12 @@ static int sgx_vma_access(struct
> > > > vm_area_struct
> > > *vma, unsigned long addr,
> > > >  		return -EFAULT;
> > > >
> > > >  	for (i = 0; i < len; i += cnt) {
> > > > +		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> > > > +		if (sgx_epc_is_locked()) {
> > > > +			ret = -EBUSY;
> > > > +			goto out;
> > > > +		}
> > > > +
> > > >  		entry = sgx_encl_reserve_page(encl, (addr + i) &
> > > PAGE_MASK);
> > > >  		if (IS_ERR(entry)) {
> > > >  			ret = PTR_ERR(entry);
> > > > @@ -555,6 +575,7 @@ static int sgx_vma_access(struct
> > > > vm_area_struct *vma, unsigned long addr,
> > > >
> > > >  out:
> > > >  		mutex_unlock(&encl->lock);
> > > > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > > >
> > > >  		if (ret)
> > > >  			break;
> > > > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c
> > > > b/arch/x86/kernel/cpu/sgx/ioctl.c index d8c3c07badb3..da3b569a10bd
> > > > 100644
> > > > --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> > > > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> > > > @@ -147,6 +147,7 @@ static int sgx_encl_create(struct sgx_encl
> > > > *encl, struct sgx_secs *secs)  static long
> > > > sgx_ioc_enclave_create(struct sgx_encl *encl, void __user *arg)  {
> > > >  	struct sgx_enclave_create create_arg;
> > > > +	int srcu_idx;
> > > >  	void *secs;
> > > >  	int ret;
> > > >
> > > > @@ -162,9 +163,18 @@ static long sgx_ioc_enclave_create(struct
> > > > sgx_encl *encl, void __user *arg)
> > > >
> > > >  	if (copy_from_user(secs, (void __user *)create_arg.src, PAGE_SIZE))
> > > >  		ret = -EFAULT;
> > > > -	else
> > > > +	else {
> > > > +		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> > > > +		if (sgx_epc_is_locked()) {
> > > > +			srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > > > +			return -EBUSY;
> > > > +		}
> > > > +
> > > >  		ret = sgx_encl_create(encl, secs);
> > > >
> > > > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > > > +	}
> > > > +
> > > >  	kfree(secs);
> > > >  	return ret;
> > > >  }
> > > > @@ -444,6 +454,7 @@ static long sgx_ioc_enclave_add_pages(struct
> > > sgx_encl *encl, void __user *arg)
> > > >  	struct sgx_enclave_add_pages add_arg;
> > > >  	struct sgx_secinfo secinfo;
> > > >  	unsigned long c;
> > > > +	int srcu_idx;
> > > >  	int ret;
> > > >
> > > >  	if (!test_bit(SGX_ENCL_CREATED, &encl->flags) || @@ -477,8
> > > > +488,18 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl
> > > > *encl, void
> > > __user *arg)
> > > >  		if (need_resched())
> > > >  			cond_resched();
> > > >
> > > > +		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> > > > +		if (sgx_epc_is_locked()) {
> > > > +			ret = -EBUSY;
> > > > +			srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > > > +			break;
> > > > +		}
> > > > +
> > > >  		ret = sgx_encl_add_page(encl, add_arg.src + c,
> > > add_arg.offset + c,
> > > >  					&secinfo, add_arg.flags);
> > > > +
> > > > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > > > +
> > > >  		if (ret)
> > > >  			break;
> > > >  	}
> > > > @@ -952,6 +973,7 @@ static long sgx_enclave_restrict_perm(struct
> > > sgx_encl *encl,
> > > >  	unsigned long addr;
> > > >  	unsigned long c;
> > > >  	void *epc_virt;
> > > > +	int srcu_idx;
> > > >  	int ret;
> > > >
> > > >  	memset(&secinfo, 0, sizeof(secinfo)); @@ -960,6 +982,12 @@
> > > > static long sgx_enclave_restrict_perm(struct sgx_encl *encl,
> > > >  	vm_prot = vm_prot_from_secinfo(secinfo_perm);
> > > >
> > > >  	for (c = 0 ; c < modp->length; c += PAGE_SIZE) {
> > > > +		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> > > > +		if (sgx_epc_is_locked()) {
> > > > +			srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > > > +			return -EBUSY;
> > > > +		}
> > > > +
> > > >  		addr = encl->base + modp->offset + c;
> > > >
> > > >  		sgx_direct_reclaim();
> > > > @@ -1049,6 +1077,7 @@ static long sgx_enclave_restrict_perm(struct
> > > > sgx_encl *encl,
> > > >
> > > >  		sgx_mark_page_reclaimable(entry->epc_page);
> > > >  		mutex_unlock(&encl->lock);
> > > > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > > >  	}
> > > >
> > > >  	ret = 0;
> > > > @@ -1060,6 +1089,7 @@ static long sgx_enclave_restrict_perm(struct
> > > sgx_encl *encl,
> > > >  	sgx_mark_page_reclaimable(entry->epc_page);
> > > >  out_unlock:
> > > >  	mutex_unlock(&encl->lock);
> > > > +	srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > > >  out:
> > > >  	modp->count = c;
> > > >
> > > > @@ -1143,6 +1173,7 @@ static long sgx_enclave_modt(struct sgx_encl
> > > *encl,
> > > >  	unsigned long addr;
> > > >  	unsigned long c;
> > > >  	void *epc_virt;
> > > > +	int srcu_idx;
> > > >  	int ret;
> > > >
> > > >  	/*
> > > > @@ -1156,6 +1187,12 @@ static long sgx_enclave_modt(struct
> > > > sgx_encl
> > > *encl,
> > > >  	secinfo.flags = page_type << 8;
> > > >
> > > >  	for (c = 0 ; c < modt->length; c += PAGE_SIZE) {
> > > > +		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> > > > +		if (sgx_epc_is_locked()) {
> > > > +			srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > > > +			return -EBUSY;
> > > > +		}
> > > > +
> > > >  		addr = encl->base + modt->offset + c;
> > > >
> > > >  		sgx_direct_reclaim();
> > > > @@ -1255,6 +1292,7 @@ static long sgx_enclave_modt(struct sgx_encl
> > > *encl,
> > > >  		entry->type = page_type;
> > > >
> > > >  		mutex_unlock(&encl->lock);
> > > > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > > >  	}
> > > >
> > > >  	ret = 0;
> > > > @@ -1265,6 +1303,7 @@ static long sgx_enclave_modt(struct sgx_encl
> > > *encl,
> > > >  	entry->vm_run_prot_bits = run_prot_restore;
> > > >  out_unlock:
> > > >  	mutex_unlock(&encl->lock);
> > > > +	srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > > >  out:
> > > >  	modt->count = c;
> > > >
> > > > @@ -1350,12 +1389,19 @@ static long sgx_encl_remove_pages(struct
> > > sgx_encl *encl,
> > > >  	unsigned long addr;
> > > >  	unsigned long c;
> > > >  	void *epc_virt;
> > > > +	int srcu_idx;
> > > >  	int ret;
> > > >
> > > >  	memset(&secinfo, 0, sizeof(secinfo));
> > > >  	secinfo.flags = SGX_SECINFO_R | SGX_SECINFO_W | SGX_SECINFO_X;
> > > >
> > > >  	for (c = 0 ; c < params->length; c += PAGE_SIZE) {
> > > > +		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> > > > +		if (sgx_epc_is_locked()) {
> > > > +			srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > > > +			return -EBUSY;
> > > > +		}
> > > > +
> > > >  		addr = encl->base + params->offset + c;
> > > >
> > > >  		sgx_direct_reclaim();
> > > > @@ -1411,6 +1457,7 @@ static long sgx_encl_remove_pages(struct
> > > sgx_encl *encl,
> > > >  		kfree(entry);
> > > >
> > > >  		mutex_unlock(&encl->lock);
> > > > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > > >  	}
> > > >
> > > >  	ret = 0;
> > > > @@ -1418,6 +1465,7 @@ static long sgx_encl_remove_pages(struct
> > > > sgx_encl *encl,
> > > >
> > > >  out_unlock:
> > > >  	mutex_unlock(&encl->lock);
> > > > +	srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > > >  out:
> > > >  	params->count = c;
> > > >
> > > > diff --git a/arch/x86/kernel/cpu/sgx/main.c
> > > > b/arch/x86/kernel/cpu/sgx/main.c index 545da16bb3ea..99c86b77ca8f
> > > > 100644
> > > > --- a/arch/x86/kernel/cpu/sgx/main.c
> > > > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > > > @@ -23,6 +23,17 @@ static int sgx_nr_epc_sections;  static struct
> > > > task_struct *ksgxd_tsk;  static
> > > > DECLARE_WAIT_QUEUE_HEAD(ksgxd_waitq);
> > > >  static DEFINE_XARRAY(sgx_epc_address_space);
> > > > +/*
> > > > + * The flag sgx_epc_locked prevents any new SGX flows that
> > > > + * may attempt to allocate a new EPC page.
> > > > + */
> > > > +static bool __rcu sgx_epc_locked;
> > > > +/*
> > > > + * By synchronizing around sgx_epc_locked SRCU ensures that any
> > > > +executing
> > > > + * SGX flows have completed before proceeding with an SVN update.
> > > > +New SGX flows
> > > > + * will be prevented from starting during an SVN update.
> > > > + */
> > > > +DEFINE_SRCU(sgx_lock_epc_srcu);
> > > >
> > > >  /*
> > > >   * These variables are part of the state of the reclaimer, and
> > > > must be accessed @@ -378,6 +389,8 @@ void sgx_direct_reclaim(void)
> > > >
> > > >  static int ksgxd(void *p)
> > > >  {
> > > > +	int srcu_idx;
> > > > +
> > > >  	set_freezable();
> > > >
> > > >  	/*
> > > > @@ -398,9 +411,15 @@ static int ksgxd(void *p)
> > > >  				     kthread_should_stop() ||
> > > >
> > > sgx_should_reclaim(SGX_NR_HIGH_PAGES));
> > > >
> > > > +		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> > > > +		if (sgx_epc_is_locked())
> > > > +			goto maybe_resched;
> > > > +
> > > >  		if (sgx_should_reclaim(SGX_NR_HIGH_PAGES))
> > > >  			sgx_reclaim_pages();
> > > >
> > > > +maybe_resched:
> > > > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > > >  		cond_resched();
> > > >  	}
> > > >
> > > > @@ -943,3 +962,21 @@ static int __init sgx_init(void)  }
> > > >
> > > >  device_initcall(sgx_init);
> > > > +
> > > > +static void sgx_lock_epc(void)
> > > > +{
> > > > +	sgx_epc_locked = true;
> > > > +	synchronize_srcu(&sgx_lock_epc_srcu);
> > > > +}
> > > > +
> > > > +static void sgx_unlock_epc(void)
> > > > +{
> > > > +	sgx_epc_locked = false;
> > > > +	synchronize_srcu(&sgx_lock_epc_srcu);
> > > > +}
> > > > +
> > > > +bool sgx_epc_is_locked(void)
> > > > +{
> > > > +	lockdep_assert_held(&sgx_lock_epc_srcu);
> > > > +	return sgx_epc_locked;
> > > > +}
> > > > diff --git a/arch/x86/kernel/cpu/sgx/virt.c
> > > > b/arch/x86/kernel/cpu/sgx/virt.c index 6a77a14eee38..e953816d7c8b
> > > > 100644
> > > > --- a/arch/x86/kernel/cpu/sgx/virt.c
> > > > +++ b/arch/x86/kernel/cpu/sgx/virt.c
> > > > @@ -75,10 +75,21 @@ static vm_fault_t sgx_vepc_fault(struct
> > > > vm_fault
> > > > *vmf)  {
> > > >  	struct vm_area_struct *vma = vmf->vma;
> > > >  	struct sgx_vepc *vepc = vma->vm_private_data;
> > > > +	int srcu_idx;
> > > >  	int ret;
> > > >
> > > >  	mutex_lock(&vepc->lock);
> > > > +	srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> > > > +
> > > > +	if (sgx_epc_is_locked()) {
> > > > +		ret = -EBUSY;
> > > > +		goto out_unlock;
> > > > +	}
> > > > +
> > > >  	ret = __sgx_vepc_fault(vepc, vma, vmf->address);
> > > > +
> > > > +out_unlock:
> > > > +	srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > > >  	mutex_unlock(&vepc->lock);
> > > >
> > > >  	if (!ret)
> > > > @@ -331,6 +342,7 @@ int __init sgx_vepc_init(void)  int
> > > > sgx_virt_ecreate(struct sgx_pageinfo *pageinfo, void __user *secs,
> > > >  		     int *trapnr)
> > > >  {
> > > > +	int srcu_idx;
> > > >  	int ret;
> > > >
> > > >  	/*
> > > > @@ -347,6 +359,12 @@ int sgx_virt_ecreate(struct sgx_pageinfo
> > > > *pageinfo,
> > > void __user *secs,
> > > >  	if (WARN_ON_ONCE(!access_ok(secs, PAGE_SIZE)))
> > > >  		return -EINVAL;
> > > >
> > > > +	srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> > > > +	if (sgx_epc_is_locked()) {
> > > > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > > > +		return -EBUSY;
> > > > +	}
> > > > +
> > > >  	__uaccess_begin();
> > > >  	ret = __ecreate(pageinfo, (void *)secs);
> > > >  	__uaccess_end();
> > > > @@ -356,6 +374,8 @@ int sgx_virt_ecreate(struct sgx_pageinfo
> > > > *pageinfo,
> > > void __user *secs,
> > > >  		return -EFAULT;
> > > >  	}
> > > >
> > > > +	srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > > > +
> > > >  	/* ECREATE doesn't return an error code, it faults or succeeds. */
> > > >  	WARN_ON_ONCE(ret);
> > > >  	return 0;
> > > > --
> > > > 2.17.1
> > > >
> > >
> > > This series is broken. It does not apply on top of tip/x86/sgx. Also
> > > you forgot CC to me.
> >
> > I'm sincerely sorry for not cc to you, I'm aware of that after sending
> > out :- )
> 
> Apologies, I did not mean it like that. Just mentioning that I can look into
> this :-)

It's very appreciated if you can help review to move this forward, thanks Jarkko!

> 
> > Yes, it requires to apply the following SGX2 v2 patches to
> > tip/x86/sgx, prior to this series. As I mentioned in cover letter,
> > SGX2 introduces new SGX instructions and flows, which should be handled
> during SVN update.
> >
> > The v2 of SGX2 support patch set, with 32 patches in total, refer to the
> cover letter:
> > https://lore.kernel.org/lkml/cover.1644274683.git.reinette.chatre@inte
> > l.com/ The two additional patches SGX2 support requires:
> > "x86/sgx: Add poison handling to reclaimer"
> >
> > https://lore.kernel.org/linux-
> sgx/dcc95eb2aaefb042527ac50d0a50738c7c16
> > 0dac.1643830353.git.reinette.chatre@intel.com/
> > "x86/sgx: Silence softlockup detection when releasing large enclaves"
> >
> > https://lore.kernel.org/linux-
> sgx/b5e9f218064aa76e3026f778e1ad0a1d823e
> > 3db8.1643133224.git.reinette.chatre@intel.com/
> 
> Why don't you pick the dependent patches from that patch set and put them
> to this one? It does not matter if two patch sets contain same patches. If this
> gets merged first, then Reinette's job will be easier, and vice versa.
> Tags make sure that everyone gets what they deserve from their contribution.

For easy to discuss, I split the SGX2 support patches into the following two parts:

1. auxiliary changes:
[V2,09/32] x86/sgx: Export sgx_encl_ewb_cpumask()
https://lore.kernel.org/lkml/cover.1644274683.git.reinette.chatre@intel.com/T/#m44e2b931e82a87a8b2c80058130182eb747fbcf0 
[V2,10/32] x86/sgx: Rename sgx_encl_ewb_cpumask() as sgx_encl_cpumask()
https://lore.kernel.org/lkml/cover.1644274683.git.reinette.chatre@intel.com/T/#mf6268a66b5c48ca9a18a772b6eaea097c315dc1d
[V2,12/32] x86/sgx: Make sgx_ipi_cb() available internally
https://lore.kernel.org/lkml/cover.1644274683.git.reinette.chatre@intel.com/T/#ma3330f8ee8136aa084d0a2b5f110331e37f44c52
[V2,14/32] x86/sgx: Keep record of SGX page type
https://lore.kernel.org/lkml/cover.1644274683.git.reinette.chatre@intel.com/T/#m4ae80fdf67ad330119bfc2abaea845baa24ed14c

2. The other SGX2 support patches.

For #1, we can put them into this series. But regarding #2, it introduces EAUG/EMODPR/EMODT and the related flows, it seems not sensible to pick up only some of the patches, it will be not convenient to run the related test while doing SVN update.

> 
> Otherwise for instance giving a tested-by is impossible. You have to root this
> to the mainline, or at least to x86 tip.git.

Put this series on top of SGX2 support is based on the assumption that SGX2 will get merged first. If it's required to make this series independent of SGX2, we need to pick up the above #1, and make change in patch " x86/sgx: Introduce mechanism to prevent new initializations of EPC pages" to remove the handling on SGX2 flows. Once SGX2 gets merged, provide the changes in following patches.

How do you think, Jarkko? And I also would like to get suggestion from @Chatre, Reinette for this.

> 
> BR, Jarkko

Re: [RFC PATCH v2 09/10] x86/cpu: Call ENCLS[EUPDATESVN] procedure in microcode update

[Jethro Beekman]

[-- Attachment #1: Type: text/plain, Size: 1835 bytes --]

On 2022-03-16 16:42, Dave Hansen wrote:
> On 3/16/22 02:46, Jethro Beekman wrote:
>>> +void update_cpusvn_intel(void) +{ +	sgx_lock_epc(); +	if
>>> (sgx_zap_pages())
>> Doing this automatically and unconditionally during a microcode
>> update seems undesirable. This requires the userland tooling that is
>> coordinating the microcode update to be aware of any SGX enclaves
>> that are running and possibly coordinate sequencing with the
>> processes containing those enclaves. This coupling does not exist
>> today.
> 
> "Today" in what?

In distros.

> If a microcode update changes SGX behavior and bumps CPUSVN, it's
> fundamentally incompatible with letting enclaves continue to run.  They
> might as well be killed.

It's not "fundamentally incompatible". It works fine today and will continue to work fine in CPUs that have EUPDATESVN support. Yes your enclaves are probably vulnerable to some attacks, but people run vulnerable software intentionally all the time.

> But, seriously, if you can't handle enclaves being killed every few
> months, don't use SGX.  The architecture doesn't allow data to be

I don't think anyone is making a claim that there are enclaves that wouldn't be able handle this? Being able to deal with something is not the same as wanting to deal with something. My laptop can handle running out of battery in the middle of writing some new code. That doesn't mean I want my laptop to arbitrarily turn off at uncontrollable times?

> persistent like normal x86.  It's fundamental to the architecture.  You
> can also think of this as a shiny new SGX *testing* feature: one that
> keeps enclave owners from becoming complacent and forgetting about what
> the SGX architecture provides.

Great idea, why not expand on this and just randomly call EREMOVE at timed intervals?

--
Jethro Beekman | Fortanix

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4490 bytes --]

Re: [RFC PATCH v2 09/10] x86/cpu: Call ENCLS[EUPDATESVN] procedure in microcode update

[Jethro Beekman]

[-- Attachment #1: Type: text/plain, Size: 1190 bytes --]

On 2022-03-16 16:47, Dave Hansen wrote:
> On 3/16/22 03:24, Jethro Beekman wrote:
>>> Doing this automatically and unconditionally during a microcode
>>> update seems undesirable. This requires the userland tooling that
>>> is coordinating the microcode update to be aware of any SGX
>>> enclaves that are running and possibly coordinate sequencing with
>>> the processes containing those enclaves. This coupling does not
>>> exist today.
>> Also, a microcode update may not affect SGX security at all and doing
>> the EUPDATESVN procedure may not be required for this particular
>> update. This case is called out specifically in the EUPDATESVN
>> documentation.
> 
> I don't think Intel provides the metadata for the kernel to tell if an
> update requires an EUPDATESVN procedure or not.  If this is inconvenient
> for you, I'd suggest reporting this to the folks at Intel who can fix
> it.  It doesn't seem like something which they are motivated to fix.

Whether or not metadata is currently made available is orthogonal to creating a mechanism by which userspace can indidate that a particular microcode update shouldn't trigger the EUPDATESVN procedure.

--
Jethro Beekman | Fortanix

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4490 bytes --]

Re: [RFC PATCH v2 01/10] x86/sgx: Introduce mechanism to prevent new initializations of EPC pages

[Reinette Chatre]

Hi Cathy,

On 3/17/2022 7:33 PM, Zhang, Cathy wrote:
> Hi Jarkko,
> 
>> -----Original Message-----
>> From: Jarkko Sakkinen <jarkko@kernel.org>
>> Sent: Friday, March 18, 2022 2:53 AM
>> To: Zhang, Cathy <cathy.zhang@intel.com>
>> Cc: linux-sgx@vger.kernel.org; linux-kernel@vger.kernel.org; Hansen, Dave
>> <dave.hansen@intel.com>; Raj, Ashok <ashok.raj@intel.com>
>> Subject: Re: [RFC PATCH v2 01/10] x86/sgx: Introduce mechanism to prevent
>> new initializations of EPC pages
>>
>> On Thu, Mar 17, 2022 at 02:08:21PM +0000, Zhang, Cathy wrote:
>>> Hi Jarkko,
>>>
>>>> -----Original Message-----
>>>> From: Jarkko Sakkinen <jarkko@kernel.org>
>>>> Sent: Thursday, March 17, 2022 12:13 PM
>>>> To: Zhang, Cathy <cathy.zhang@intel.com>
>>>> Cc: linux-sgx@vger.kernel.org; linux-kernel@vger.kernel.org; Hansen,
>>>> Dave <dave.hansen@intel.com>; Raj, Ashok <ashok.raj@intel.com>
>>>> Subject: Re: [RFC PATCH v2 01/10] x86/sgx: Introduce mechanism to
>>>> prevent new initializations of EPC pages
>>>>
>>>> On Tue, Mar 15, 2022 at 09:02:51AM +0800, Cathy Zhang wrote:

...

>>> Yes, it requires to apply the following SGX2 v2 patches to
>>> tip/x86/sgx, prior to this series. As I mentioned in cover letter,
>>> SGX2 introduces new SGX instructions and flows, which should be handled
>> during SVN update.
>>>
>>> The v2 of SGX2 support patch set, with 32 patches in total, refer to the
>> cover letter:
>>> https://lore.kernel.org/lkml/cover.1644274683.git.reinette.chatre@inte
>>> l.com/ The two additional patches SGX2 support requires:
>>> "x86/sgx: Add poison handling to reclaimer"
>>>
>>> https://lore.kernel.org/linux-
>> sgx/dcc95eb2aaefb042527ac50d0a50738c7c16
>>> 0dac.1643830353.git.reinette.chatre@intel.com/
>>> "x86/sgx: Silence softlockup detection when releasing large enclaves"
>>>
>>> https://lore.kernel.org/linux-
>> sgx/b5e9f218064aa76e3026f778e1ad0a1d823e
>>> 3db8.1643133224.git.reinette.chatre@intel.com/
>>
>> Why don't you pick the dependent patches from that patch set and put them
>> to this one? It does not matter if two patch sets contain same patches. If this
>> gets merged first, then Reinette's job will be easier, and vice versa.
>> Tags make sure that everyone gets what they deserve from their contribution.
> 
> For easy to discuss, I split the SGX2 support patches into the following two parts:
> 
> 1. auxiliary changes:
> [V2,09/32] x86/sgx: Export sgx_encl_ewb_cpumask()
> https://lore.kernel.org/lkml/cover.1644274683.git.reinette.chatre@intel.com/T/#m44e2b931e82a87a8b2c80058130182eb747fbcf0
> [V2,10/32] x86/sgx: Rename sgx_encl_ewb_cpumask() as sgx_encl_cpumask()
> https://lore.kernel.org/lkml/cover.1644274683.git.reinette.chatre@intel.com/T/#mf6268a66b5c48ca9a18a772b6eaea097c315dc1d
> [V2,12/32] x86/sgx: Make sgx_ipi_cb() available internally
> https://lore.kernel.org/lkml/cover.1644274683.git.reinette.chatre@intel.com/T/#ma3330f8ee8136aa084d0a2b5f110331e37f44c52
> [V2,14/32] x86/sgx: Keep record of SGX page type
> https://lore.kernel.org/lkml/cover.1644274683.git.reinette.chatre@intel.com/T/#m4ae80fdf67ad330119bfc2abaea845baa24ed14c
> 
> 2. The other SGX2 support patches.
> 
> For #1, we can put them into this series. But regarding #2, it introduces EAUG/EMODPR/EMODT and the related flows, it seems not sensible to pick up only some of the patches, it will be not convenient to run the related test while doing SVN update.
> 
>>
>> Otherwise for instance giving a tested-by is impossible. You have to root this
>> to the mainline, or at least to x86 tip.git.
> 
> Put this series on top of SGX2 support is based on the assumption that SGX2 will get merged first. If it's required to make this series independent of SGX2, we need to pick up the above #1, and make change in patch " x86/sgx: Introduce mechanism to prevent new initializations of EPC pages" to remove the handling on SGX2 flows. Once SGX2 gets merged, provide the changes in following patches.
> 
> How do you think, Jarkko? And I also would like to get suggestion from @Chatre, Reinette for this.

Is it possible to factor out the changes to the SGX2 flows into separate patches?

This should continue to support the testing you mentioned and the patches could
then be dropped from your series and moved to the SGX2 series if the EUPDATESVN
work is merged first. I am not sure about the testing though because I did not
notice a selftest as part of the series. How can I test this work if/when it
moves to the SGX2 series?

Reinette

RE: [RFC PATCH v2 01/10] x86/sgx: Introduce mechanism to prevent new initializations of EPC pages

[Zhang, Cathy]

Hi Reinette,

> -----Original Message-----
> From: Chatre, Reinette <reinette.chatre@intel.com>
> Sent: Saturday, March 19, 2022 12:24 AM
> To: Zhang, Cathy <cathy.zhang@intel.com>; Jarkko Sakkinen
> <jarkko@kernel.org>
> Cc: linux-sgx@vger.kernel.org; linux-kernel@vger.kernel.org; Hansen, Dave
> <dave.hansen@intel.com>; Raj, Ashok <ashok.raj@intel.com>
> Subject: Re: [RFC PATCH v2 01/10] x86/sgx: Introduce mechanism to prevent
> new initializations of EPC pages
> 
> Hi Cathy,
> 
> On 3/17/2022 7:33 PM, Zhang, Cathy wrote:
> > Hi Jarkko,
> >
> >> -----Original Message-----
> >> From: Jarkko Sakkinen <jarkko@kernel.org>
> >> Sent: Friday, March 18, 2022 2:53 AM
> >> To: Zhang, Cathy <cathy.zhang@intel.com>
> >> Cc: linux-sgx@vger.kernel.org; linux-kernel@vger.kernel.org; Hansen,
> >> Dave <dave.hansen@intel.com>; Raj, Ashok <ashok.raj@intel.com>
> >> Subject: Re: [RFC PATCH v2 01/10] x86/sgx: Introduce mechanism to
> >> prevent new initializations of EPC pages
> >>
> >> On Thu, Mar 17, 2022 at 02:08:21PM +0000, Zhang, Cathy wrote:
> >>> Hi Jarkko,
> >>>
> >>>> -----Original Message-----
> >>>> From: Jarkko Sakkinen <jarkko@kernel.org>
> >>>> Sent: Thursday, March 17, 2022 12:13 PM
> >>>> To: Zhang, Cathy <cathy.zhang@intel.com>
> >>>> Cc: linux-sgx@vger.kernel.org; linux-kernel@vger.kernel.org;
> >>>> Hansen, Dave <dave.hansen@intel.com>; Raj, Ashok
> >>>> <ashok.raj@intel.com>
> >>>> Subject: Re: [RFC PATCH v2 01/10] x86/sgx: Introduce mechanism to
> >>>> prevent new initializations of EPC pages
> >>>>
> >>>> On Tue, Mar 15, 2022 at 09:02:51AM +0800, Cathy Zhang wrote:
> 
> ...
> 
> >>> Yes, it requires to apply the following SGX2 v2 patches to
> >>> tip/x86/sgx, prior to this series. As I mentioned in cover letter,
> >>> SGX2 introduces new SGX instructions and flows, which should be
> >>> handled
> >> during SVN update.
> >>>
> >>> The v2 of SGX2 support patch set, with 32 patches in total, refer to
> >>> the
> >> cover letter:
> >>> https://lore.kernel.org/lkml/cover.1644274683.git.reinette.chatre@in
> >>> te l.com/ The two additional patches SGX2 support requires:
> >>> "x86/sgx: Add poison handling to reclaimer"
> >>>
> >>> https://lore.kernel.org/linux-
> >> sgx/dcc95eb2aaefb042527ac50d0a50738c7c16
> >>> 0dac.1643830353.git.reinette.chatre@intel.com/
> >>> "x86/sgx: Silence softlockup detection when releasing large enclaves"
> >>>
> >>> https://lore.kernel.org/linux-
> >> sgx/b5e9f218064aa76e3026f778e1ad0a1d823e
> >>> 3db8.1643133224.git.reinette.chatre@intel.com/
> >>
> >> Why don't you pick the dependent patches from that patch set and put
> >> them to this one? It does not matter if two patch sets contain same
> >> patches. If this gets merged first, then Reinette's job will be easier, and
> vice versa.
> >> Tags make sure that everyone gets what they deserve from their
> contribution.
> >
> > For easy to discuss, I split the SGX2 support patches into the following two
> parts:
> >
> > 1. auxiliary changes:
> > [V2,09/32] x86/sgx: Export sgx_encl_ewb_cpumask()
> > https://lore.kernel.org/lkml/cover.1644274683.git.reinette.chatre@inte
> > l.com/T/#m44e2b931e82a87a8b2c80058130182eb747fbcf0
> > [V2,10/32] x86/sgx: Rename sgx_encl_ewb_cpumask() as
> > sgx_encl_cpumask()
> > https://lore.kernel.org/lkml/cover.1644274683.git.reinette.chatre@inte
> > l.com/T/#mf6268a66b5c48ca9a18a772b6eaea097c315dc1d
> > [V2,12/32] x86/sgx: Make sgx_ipi_cb() available internally
> > https://lore.kernel.org/lkml/cover.1644274683.git.reinette.chatre@inte
> > l.com/T/#ma3330f8ee8136aa084d0a2b5f110331e37f44c52
> > [V2,14/32] x86/sgx: Keep record of SGX page type
> > https://lore.kernel.org/lkml/cover.1644274683.git.reinette.chatre@inte
> > l.com/T/#m4ae80fdf67ad330119bfc2abaea845baa24ed14c
> >
> > 2. The other SGX2 support patches.
> >
> > For #1, we can put them into this series. But regarding #2, it introduces
> EAUG/EMODPR/EMODT and the related flows, it seems not sensible to pick
> up only some of the patches, it will be not convenient to run the related test
> while doing SVN update.
> >
> >>
> >> Otherwise for instance giving a tested-by is impossible. You have to
> >> root this to the mainline, or at least to x86 tip.git.
> >
> > Put this series on top of SGX2 support is based on the assumption that
> SGX2 will get merged first. If it's required to make this series independent of
> SGX2, we need to pick up the above #1, and make change in patch " x86/sgx:
> Introduce mechanism to prevent new initializations of EPC pages" to remove
> the handling on SGX2 flows. Once SGX2 gets merged, provide the changes in
> following patches.
> >
> > How do you think, Jarkko? And I also would like to get suggestion from
> @Chatre, Reinette for this.
> 
> Is it possible to factor out the changes to the SGX2 flows into separate
> patches?

Yes, the changes can be into separate patches. And the separate patches
should be moved out from this series, to make sure this series is applied
to tip branch independent of SGX2 series.

> 
> This should continue to support the testing you mentioned and the patches
> could then be dropped from your series and moved to the SGX2 series if the
> EUPDATESVN work is merged first. I am not sure about the testing though
> because I did not notice a selftest as part of the series. How can I test this
> work if/when it moves to the SGX2 series?

To test it, you will the following command while you run SGX2 stress test.

iucode-tool -K xxxxxx.PDB --overwrite ; echo 1 > /sys/devices/system/cpu/microcode/reload

Here xxxxxx.PDB is an ucode patch.

I'm not sure if it's possible to write a selftest for it for selftest should create a
ucode patch.

Additionally, regarding the "auxiliary changes" I list above, would you like to move
them into this SVN update series, or just let me rebase on them, like you rebased
with some other patches?

> 
> Reinette 

RE: [RFC PATCH v2 09/10] x86/cpu: Call ENCLS[EUPDATESVN] procedure in microcode update

[Zhang, Cathy]

Hi Jethro,

Thanks for helping review!

> -----Original Message-----
> From: Jethro Beekman <jethro@fortanix.com>
> Sent: Friday, March 18, 2022 5:06 PM
> To: Hansen, Dave <dave.hansen@intel.com>; Zhang, Cathy
> <cathy.zhang@intel.com>; linux-sgx@vger.kernel.org; linux-Hi
> kernel@vger.kernel.org
> Cc: Raj, Ashok <ashok.raj@intel.com>
> Subject: Re: [RFC PATCH v2 09/10] x86/cpu: Call ENCLS[EUPDATESVN]
> procedure in microcode update
> 
> On 2022-03-16 16:47, Dave Hansen wrote:
> > On 3/16/22 03:24, Jethro Beekman wrote:
> >>> Doing this automatically and unconditionally during a microcode
> >>> update seems undesirable. This requires the userland tooling that
> >>> is coordinating the microcode update to be aware of any SGX
> >>> enclaves that are running and possibly coordinate sequencing with
> >>> the processes containing those enclaves. This coupling does not
> >>> exist today.
> >> Also, a microcode update may not affect SGX security at all and doing
> >> the EUPDATESVN procedure may not be required for this particular
> >> update. This case is called out specifically in the EUPDATESVN
> >> documentation.
> >
> > I don't think Intel provides the metadata for the kernel to tell if an
> > update requires an EUPDATESVN procedure or not.  If this is inconvenient
> > for you, I'd suggest reporting this to the folks at Intel who can fix
> > it.  It doesn't seem like something which they are motivated to fix.
> 
> Whether or not metadata is currently made available is orthogonal to
> creating a mechanism by which userspace can indidate that a particular
> microcode update shouldn't trigger the EUPDATESVN procedure.

I'm not sure if you have noticed the discussions in v1:
https://lore.kernel.org/all/1742be9e-c18e-28c9-75c8-144bf1f6a311@intel.com/T/#m18e6fecd8c9c517c68cb4d62e53f24909abd50a7

We remove the sysfs which allows userspace to decide if and when to
trigger the EUPDATESVN procedure. Please comment if you have other
suggestion.

> 
> --
> Jethro Beekman | Fortanix