[PATCH v5 0/5] Enable automatic SVN updates for SGX enclaves

[PATCH v5 0/5] Enable automatic SVN updates for SGX enclaves

[Elena Reshetova]

Changes since v4 following reviews by Dave and Jarkko:

 - breakdown the single patch into 4 patches as
 suggested by Dave
 - fix error unwinding in sgx_(vepc_)open as 
 suggested by Jarkko
 - numerous code improvements suggested by Dave
 - numerous additional code comments and commit
 message improvements as suggested by Dave
 - switch to usage of atomic64_t for sgx_usage_count
 to ensure overflows cannot happen as suggested by Dave
 - do not return a error case when failing with
 SGX_INSUFFICIENT_ENTROPY, fail silently as suggested
 by Dave

Changes since v3 following reviews by Kai and Sean:

 - Change the overall approach to the one suggested
  by Sean and do the EUPDATESVN execution during
  sgx_open() and sgx_vepc_open().
  Note, I do not try to do EUPDATESVN during the release()
  flows since it doesnt save any noticable amount
  of time during next open flow per underlying EUPDATESVN
  microcode logic.
 - In sgx_update_svn() remove the check that we are
  running under VMM and expect the VMM to instead
  expose correct CPUID
 - Move the actual CPUID leaf check out of
  sgx_update_svn() into sgx_init()
 - Use existing RDRAND_RETRY_LOOPS define instead of 10
 - Change the sgx_update_svn() to return 0 only in
 success cases (or if unsupported)
 - Various smaller cosmetic fixes

The still to be discussed question is what sgx_update_svn()
should return in case of various failures. The current version
follows suggestion by Kai and would return an error (and block
sgx_(vepc_)open() in all cases, including running out of entropy.
I think this might be the correct approach for SGX_INSUFFICIENT_ENTROPY
since in such cases userspace can retry the open() and also
will get an info about what is actually blocking the EUPDATEVSN
(and can act on this). However, this is a change in existing API
and therefore debatable and I would like to hear people's feedback.

Changes since v2 following review by Jarkko:

 - formatting of comments is fixed
 - change from pr_error to ENCLS_WARN to communicate errors from
 EUPDATESVN
 - In case an unknown error is detected (must not happen per spec),
 make page allocation from EPC fail in order to prevent EPC usage

Changes since v1 following review by Jarkko:

 - first and second patch are squashed together and a better
   explanation of the change is added into the commit message
 - third and fourth patch are also combined for better understanding
   of error code purposes used in 4th patch
 - implementation of sgx_updatesvn adjusted following Jarkko's 
   suggestions
 - minor fixes in both commit messages and code from the review
 - dropping co-developed-by tag since the code now differs enough
   from the original submission. However, the reference where the
   original code came from and credits to original author is kept

Background
----------

In case an SGX vulnerability is discovered and TCB recovery
for SGX is triggered, Intel specifies a process that must be
followed for a given vulnerability. Steps to mitigate can vary
based on vulnerability type, affected components, etc.
In some cases, a vulnerability can be mitigated via a runtime
recovery flow by shutting down all running SGX enclaves,
clearing enclave page cache (EPC), applying a microcode patch
that does not require a reboot (via late microcode loading) and
restarting all SGX enclaves.


Problem statement
-------------------------
Even when the above-described runtime recovery flow to mitigate the
SGX vulnerability is followed, the SGX attestation evidence will
still reflect the security SVN version being equal to the previous
state of security SVN (containing vulnerability) that created
and managed the enclave until the runtime recovery event. This
limitation currently can be only overcome via a platform reboot,
which negates all the benefits from the rebootless late microcode
loading and not required in this case for functional or security
purposes.


Proposed solution
-----------------

SGX architecture introduced  a new instruction called EUPDATESVN [1]
to Ice Lake. It allows updating security SVN version, given that EPC
is completely empty. The latter is required for security reasons
in order to reason that enclave security posture is as secure as the
security SVN version of the TCB that created it.

This series enables opportunistic execution of EUPDATESVN upon first
EPC page allocation for a first enclave to be run on the platform.

This series is partly based on the previous work done by Cathy Zhang
[2], which attempted to enable forceful destruction of all SGX
enclaves and execution of EUPDATESVN upon successful application of
any microcode patch. This approach is determined as being too
intrusive for the running SGX enclaves, especially taking into account
that it would be performed upon *every* microcode patch application
regardless if it changes the security SVN version or not (change to the
security SVN version is a rare event).

Testing
-------

Tested on EMR machine using kernel-6.15.0_rc5 & sgx selftests.
If Google folks in CC can test on their side, it would be greatly appreciated.


References
----------

[1] https://cdrdv2.intel.com/v1/dl/getContent/648682
[2] https://lore.kernel.org/all/20220520103904.1216-1-cathy.zhang@intel.com/

Elena Reshetova (5):
  x86/sgx: Introduce a counter to count the sgx_(vepc_)open()
  x86/cpufeatures: Add X86_FEATURE_SGX_EUPDATESVN feature flag
  x86/sgx: Define error codes for use by ENCLS[EUPDATESVN]
  x86/sgx: Implement ENCLS[EUPDATESVN]
  x86/sgx: Enable automatic SVN updates for SGX enclaves

 arch/x86/include/asm/cpufeatures.h       |   1 +
 arch/x86/include/asm/sgx.h               |  39 +++++----
 arch/x86/kernel/cpu/scattered.c          |   1 +
 arch/x86/kernel/cpu/sgx/driver.c         |  22 +++--
 arch/x86/kernel/cpu/sgx/encl.c           |   1 +
 arch/x86/kernel/cpu/sgx/encls.h          |   5 ++
 arch/x86/kernel/cpu/sgx/main.c           | 103 +++++++++++++++++++++++
 arch/x86/kernel/cpu/sgx/sgx.h            |   3 +
 arch/x86/kernel/cpu/sgx/virt.c           |  16 +++-
 tools/arch/x86/include/asm/cpufeatures.h |   1 +
 10 files changed, 169 insertions(+), 23 deletions(-)

-- 
2.45.2

[PATCH v5 1/5] x86/sgx: Introduce a counter to count the sgx_(vepc_)open()

[Elena Reshetova]

Currently SGX does not have a global counter to count the
active users from userspace or hypervisor. Implement such a counter,
sgx_usage_count. It will be used by the driver when attempting
to call EUPDATESVN SGX instruction.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
---
 arch/x86/kernel/cpu/sgx/driver.c |  1 +
 arch/x86/kernel/cpu/sgx/encl.c   |  1 +
 arch/x86/kernel/cpu/sgx/main.c   | 14 ++++++++++++++
 arch/x86/kernel/cpu/sgx/sgx.h    |  3 +++
 arch/x86/kernel/cpu/sgx/virt.c   |  2 ++
 5 files changed, 21 insertions(+)

diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c
index 7f8d1e11dbee..b5ffe104af4c 100644
--- a/arch/x86/kernel/cpu/sgx/driver.c
+++ b/arch/x86/kernel/cpu/sgx/driver.c
@@ -19,6 +19,7 @@ static int sgx_open(struct inode *inode, struct file *file)
 	struct sgx_encl *encl;
 	int ret;
 
+	sgx_inc_usage_count();
 	encl = kzalloc(sizeof(*encl), GFP_KERNEL);
 	if (!encl)
 		return -ENOMEM;
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index 279148e72459..3b54889ae4a4 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -765,6 +765,7 @@ void sgx_encl_release(struct kref *ref)
 	WARN_ON_ONCE(encl->secs.epc_page);
 
 	kfree(encl);
+	sgx_dec_usage_count();
 }
 
 /*
diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 8ce352fc72ac..80d565e6f2ad 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -914,6 +914,20 @@ int sgx_set_attribute(unsigned long *allowed_attributes,
 }
 EXPORT_SYMBOL_GPL(sgx_set_attribute);
 
+/* Counter to count the active SGX users */
+static atomic64_t sgx_usage_count;
+
+int sgx_inc_usage_count(void)
+{
+	atomic64_inc(&sgx_usage_count);
+	return 0;
+}
+
+void sgx_dec_usage_count(void)
+{
+	atomic64_dec(&sgx_usage_count);
+}
+
 static int __init sgx_init(void)
 {
 	int ret;
diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
index d2dad21259a8..f5940393d9bd 100644
--- a/arch/x86/kernel/cpu/sgx/sgx.h
+++ b/arch/x86/kernel/cpu/sgx/sgx.h
@@ -102,6 +102,9 @@ static inline int __init sgx_vepc_init(void)
 }
 #endif
 
+int sgx_inc_usage_count(void);
+void sgx_dec_usage_count(void);
+
 void sgx_update_lepubkeyhash(u64 *lepubkeyhash);
 
 #endif /* _X86_SGX_H */
diff --git a/arch/x86/kernel/cpu/sgx/virt.c b/arch/x86/kernel/cpu/sgx/virt.c
index 7aaa3652e31d..83de0907f32c 100644
--- a/arch/x86/kernel/cpu/sgx/virt.c
+++ b/arch/x86/kernel/cpu/sgx/virt.c
@@ -255,6 +255,7 @@ static int sgx_vepc_release(struct inode *inode, struct file *file)
 	xa_destroy(&vepc->page_array);
 	kfree(vepc);
 
+	sgx_dec_usage_count();
 	return 0;
 }
 
@@ -262,6 +263,7 @@ static int sgx_vepc_open(struct inode *inode, struct file *file)
 {
 	struct sgx_vepc *vepc;
 
+	sgx_inc_usage_count();
 	vepc = kzalloc(sizeof(struct sgx_vepc), GFP_KERNEL);
 	if (!vepc)
 		return -ENOMEM;
-- 
2.45.2

[PATCH v5 2/5] x86/cpufeatures: Add X86_FEATURE_SGX_EUPDATESVN feature flag

[Elena Reshetova]

Add a flag indicating whenever ENCLS[EUPDATESVN] SGX instruction
is supported. This will be used by SGX driver to perform CPU
SVN updates.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
---
 arch/x86/include/asm/cpufeatures.h       | 1 +
 arch/x86/kernel/cpu/scattered.c          | 1 +
 tools/arch/x86/include/asm/cpufeatures.h | 1 +
 3 files changed, 3 insertions(+)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 6c2c152d8a67..ed0c0fa5822a 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -481,6 +481,7 @@
 #define X86_FEATURE_AMD_HETEROGENEOUS_CORES (21*32 + 6) /* Heterogeneous Core Topology */
 #define X86_FEATURE_AMD_WORKLOAD_CLASS	(21*32 + 7) /* Workload Classification */
 #define X86_FEATURE_PREFER_YMM		(21*32 + 8) /* Avoid ZMM registers due to downclocking */
+#define X86_FEATURE_SGX_EUPDATESVN	(21*32 + 9) /* Support for ENCLS[EUPDATESVN] instruction */
 
 /*
  * BUG word(s)
diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
index 16f3ca30626a..a7e1fcedca3c 100644
--- a/arch/x86/kernel/cpu/scattered.c
+++ b/arch/x86/kernel/cpu/scattered.c
@@ -41,6 +41,7 @@ static const struct cpuid_bit cpuid_bits[] = {
 	{ X86_FEATURE_PER_THREAD_MBA,		CPUID_ECX,  0, 0x00000010, 3 },
 	{ X86_FEATURE_SGX1,			CPUID_EAX,  0, 0x00000012, 0 },
 	{ X86_FEATURE_SGX2,			CPUID_EAX,  1, 0x00000012, 0 },
+	{ X86_FEATURE_SGX_EUPDATESVN,	CPUID_EAX, 10, 0x00000012, 0 },
 	{ X86_FEATURE_SGX_EDECCSSA,		CPUID_EAX, 11, 0x00000012, 0 },
 	{ X86_FEATURE_HW_PSTATE,		CPUID_EDX,  7, 0x80000007, 0 },
 	{ X86_FEATURE_CPB,			CPUID_EDX,  9, 0x80000007, 0 },
diff --git a/tools/arch/x86/include/asm/cpufeatures.h b/tools/arch/x86/include/asm/cpufeatures.h
index 6c2c152d8a67..ed0c0fa5822a 100644
--- a/tools/arch/x86/include/asm/cpufeatures.h
+++ b/tools/arch/x86/include/asm/cpufeatures.h
@@ -481,6 +481,7 @@
 #define X86_FEATURE_AMD_HETEROGENEOUS_CORES (21*32 + 6) /* Heterogeneous Core Topology */
 #define X86_FEATURE_AMD_WORKLOAD_CLASS	(21*32 + 7) /* Workload Classification */
 #define X86_FEATURE_PREFER_YMM		(21*32 + 8) /* Avoid ZMM registers due to downclocking */
+#define X86_FEATURE_SGX_EUPDATESVN	(21*32 + 9) /* Support for ENCLS[EUPDATESVN] instruction */
 
 /*
  * BUG word(s)
-- 
2.45.2

[PATCH v5 3/5] x86/sgx: Define error codes for use by ENCLS[EUPDATESVN]

[Elena Reshetova]

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

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
---
 arch/x86/include/asm/sgx.h | 39 +++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
index 6a0069761508..0361a6f91359 100644
--- a/arch/x86/include/asm/sgx.h
+++ b/arch/x86/include/asm/sgx.h
@@ -28,21 +28,22 @@
 #define SGX_CPUID_EPC_MASK	GENMASK(3, 0)
 
 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,
 };
 
 /**
@@ -73,6 +74,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 +87,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.45.2

[PATCH v5 4/5] x86/sgx: Implement ENCLS[EUPDATESVN]

[Elena Reshetova]

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.

Add the method to perform ENCLS[EUPDATESVN].

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
---
 arch/x86/kernel/cpu/sgx/encls.h |  5 +++
 arch/x86/kernel/cpu/sgx/main.c  | 57 +++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+)

diff --git a/arch/x86/kernel/cpu/sgx/encls.h b/arch/x86/kernel/cpu/sgx/encls.h
index 99004b02e2ed..d9160c89a93d 100644
--- a/arch/x86/kernel/cpu/sgx/encls.h
+++ b/arch/x86/kernel/cpu/sgx/encls.h
@@ -233,4 +233,9 @@ static inline int __eaug(struct sgx_pageinfo *pginfo, void *addr)
 	return __encls_2(EAUG, pginfo, addr);
 }
 
+/* Attempt to 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 80d565e6f2ad..fd71e2ddca59 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -15,6 +15,7 @@
 #include <linux/sysfs.h>
 #include <linux/vmalloc.h>
 #include <asm/sgx.h>
+#include <asm/archrandom.h>
 #include "driver.h"
 #include "encl.h"
 #include "encls.h"
@@ -917,6 +918,62 @@ EXPORT_SYMBOL_GPL(sgx_set_attribute);
 /* Counter to count the active SGX users */
 static atomic64_t sgx_usage_count;
 
+/**
+ * sgx_updatesvn() - Attempt to call ENCLS[EUPDATESVN]
+ * If EPC is empty, this instruction attempts to update CPUSVN to the
+ * currently loaded microcode update SVN and generate new
+ * cryptographic assets.sgx_updatesvn() Most of the time, there will
+ * be no update and that's OK.
+ *
+ * Return:
+ * 0: Success, not supported or run out of entropy
+ */
+static int sgx_update_svn(void)
+{
+	int ret;
+
+	/*
+	 * If EUPDATESVN is not available, it is ok to
+	 * silently skip it to comply with legacy behavior.
+	 */
+	if (!X86_FEATURE_SGX_EUPDATESVN)
+		return 0;
+
+	for (int i = 0; i < RDRAND_RETRY_LOOPS; i++) {
+		ret = __eupdatesvn();
+
+		/* Stop on success or unexpected errors: */
+		if (ret != SGX_INSUFFICIENT_ENTROPY)
+			break;
+	}
+
+	/*
+	 * SVN either was up-to-date or SVN update failed due
+	 * to lack of entropy. In both cases, we want to return
+	 * 0 in order not to break sgx_(vepc_)open. We dont expect
+	 * SGX_INSUFFICIENT_ENTROPY error unless underlying RDSEED
+	 * is under heavy pressure.
+	 */
+	if ((ret == SGX_NO_UPDATE) || (ret == SGX_INSUFFICIENT_ENTROPY))
+		return 0;
+
+	if (!ret) {
+		/*
+		 * SVN successfully updated.
+		 * Let users know when the update was successful.
+		 */
+		pr_info("SVN updated successfully\n");
+		return 0;
+	}
+
+	/*
+	 * EUPDATESVN was called when EPC is empty, all other error
+	 * codes are unexpected.
+	 */
+	ENCLS_WARN(ret, "EUPDATESVN");
+	return ret;
+}
+
 int sgx_inc_usage_count(void)
 {
 	atomic64_inc(&sgx_usage_count);
-- 
2.45.2

[PATCH v5 5/5] x86/sgx: Enable automatic SVN updates for SGX enclaves

[Elena Reshetova]

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 enables
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. CPUSVN is only automatically updated at reboot.
This means that, although the microcode has been updated, enclaves can never
attest to this fact. Enclaves are stuck attesting to the old version until
a reboot.

The SGX architecture has an alternative to these reboots: the ENCLS[EUPDATESVN]
instruction [1]. It allows another snapshot to be taken to update CPUSVN
after a runtime microcode update 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.

Attempt to execute EUPDATESVN on the first open of sgx_(vepc)open().
If EUPDATESVN fails with any other error code than SGX_INSUFFICIENT_ENTROPY,
this is considered unexpected and the open() returns an error. This
should not happen in practice. On contrary SGX_INSUFFICIENT_ENTROPY might
happen due to a pressure on the system DRNG (RDSEED) and therefore
the open() is not blocked to allow normal enclave operation.

[1] Runtime Microcode Updates with Intel Software Guard Extensions,
https://cdrdv2.intel.com/v1/dl/getContent/648682

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
---
 arch/x86/kernel/cpu/sgx/driver.c | 23 +++++++++++++-------
 arch/x86/kernel/cpu/sgx/main.c   | 36 ++++++++++++++++++++++++++++++--
 arch/x86/kernel/cpu/sgx/virt.c   | 16 +++++++++++---
 3 files changed, 63 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c
index b5ffe104af4c..bde06b6755f2 100644
--- a/arch/x86/kernel/cpu/sgx/driver.c
+++ b/arch/x86/kernel/cpu/sgx/driver.c
@@ -19,10 +19,15 @@ static int sgx_open(struct inode *inode, struct file *file)
 	struct sgx_encl *encl;
 	int ret;
 
-	sgx_inc_usage_count();
+	ret = sgx_inc_usage_count();
+	if (ret)
+		return -EBUSY;
+
 	encl = kzalloc(sizeof(*encl), GFP_KERNEL);
-	if (!encl)
-		return -ENOMEM;
+	if (!encl) {
+		ret = -ENOMEM;
+		goto err_usage_count;
+	}
 
 	kref_init(&encl->refcount);
 	xa_init(&encl->page_array);
@@ -32,14 +37,18 @@ static int sgx_open(struct inode *inode, struct file *file)
 	spin_lock_init(&encl->mm_lock);
 
 	ret = init_srcu_struct(&encl->srcu);
-	if (ret) {
-		kfree(encl);
-		return ret;
-	}
+	if (ret)
+		goto err_encl;
 
 	file->private_data = encl;
 
 	return 0;
+
+err_encl:
+		kfree(encl);
+err_usage_count:
+		sgx_dec_usage_count();
+		return ret;
 }
 
 static int sgx_release(struct inode *inode, struct file *file)
diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index fd71e2ddca59..d58e0c46cbf9 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -917,6 +917,8 @@ EXPORT_SYMBOL_GPL(sgx_set_attribute);
 
 /* Counter to count the active SGX users */
 static atomic64_t sgx_usage_count;
+/* Mutex to ensure EUPDATESVN is called when EPC is empty */
+static DEFINE_MUTEX(sgx_svn_lock);
 
 /**
  * sgx_updatesvn() - Attempt to call ENCLS[EUPDATESVN]
@@ -976,8 +978,38 @@ static int sgx_update_svn(void)
 
 int sgx_inc_usage_count(void)
 {
-	atomic64_inc(&sgx_usage_count);
-	return 0;
+	int ret;
+
+	/*
+	 * Increments from non-zero indicate EPC other
+	 * active EPC users and EUPDATESVN is not attempted.
+	 */
+	if (atomic64_inc_not_zero(&sgx_usage_count))
+		return 0;
+
+	/*
+	 * Ensure no other concurrent threads can start
+	 * touching EPC while EUPDATESVN is running.
+	 */
+	guard(mutex)(&sgx_svn_lock);
+
+	if (atomic64_inc_not_zero(&sgx_usage_count))
+		return 0;
+
+	/*
+	 * Attempt to call EUPDATESVN since EPC must be
+	 * empty at this point.
+	 */
+	ret = sgx_update_svn();
+
+	/*
+	 * If EUPDATESVN failed with a non-expected error
+	 * code, return failure to sgx_(vepc_)open and
+	 * do not increment the sgx_usage_count.
+	 */
+	if (!ret)
+		atomic64_inc(&sgx_usage_count);
+	return ret;
 }
 
 void sgx_dec_usage_count(void)
diff --git a/arch/x86/kernel/cpu/sgx/virt.c b/arch/x86/kernel/cpu/sgx/virt.c
index 83de0907f32c..e6e29c09c3b9 100644
--- a/arch/x86/kernel/cpu/sgx/virt.c
+++ b/arch/x86/kernel/cpu/sgx/virt.c
@@ -262,17 +262,27 @@ static int sgx_vepc_release(struct inode *inode, struct file *file)
 static int sgx_vepc_open(struct inode *inode, struct file *file)
 {
 	struct sgx_vepc *vepc;
+	int ret;
+
+	ret = sgx_inc_usage_count();
+	if (ret)
+		return -EBUSY;
 
-	sgx_inc_usage_count();
 	vepc = kzalloc(sizeof(struct sgx_vepc), GFP_KERNEL);
-	if (!vepc)
-		return -ENOMEM;
+	if (!vepc) {
+		ret = -ENOMEM;
+		goto err_usage_count;
+	}
 	mutex_init(&vepc->lock);
 	xa_init(&vepc->page_array);
 
 	file->private_data = vepc;
 
 	return 0;
+
+err_usage_count:
+		sgx_dec_usage_count();
+		return ret;
 }
 
 static long sgx_vepc_ioctl(struct file *file,
-- 
2.45.2

Re: [PATCH v5 2/5] x86/cpufeatures: Add X86_FEATURE_SGX_EUPDATESVN feature flag

[Ingo Molnar]

* Elena Reshetova <elena.reshetova@intel.com> wrote:

> Add a flag indicating whenever ENCLS[EUPDATESVN] SGX instruction
> is supported. This will be used by SGX driver to perform CPU
> SVN updates.
> 
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> ---
>  arch/x86/include/asm/cpufeatures.h       | 1 +
>  arch/x86/kernel/cpu/scattered.c          | 1 +
>  tools/arch/x86/include/asm/cpufeatures.h | 1 +
>  3 files changed, 3 insertions(+)
> 
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index 6c2c152d8a67..ed0c0fa5822a 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -481,6 +481,7 @@
>  #define X86_FEATURE_AMD_HETEROGENEOUS_CORES (21*32 + 6) /* Heterogeneous Core Topology */
>  #define X86_FEATURE_AMD_WORKLOAD_CLASS	(21*32 + 7) /* Workload Classification */
>  #define X86_FEATURE_PREFER_YMM		(21*32 + 8) /* Avoid ZMM registers due to downclocking */
> +#define X86_FEATURE_SGX_EUPDATESVN	(21*32 + 9) /* Support for ENCLS[EUPDATESVN] instruction */

Please base the patches on the latest x86 tree, there's been two 
additions already in this area.

>  /*
>   * BUG word(s)
> diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
> index 16f3ca30626a..a7e1fcedca3c 100644
> --- a/arch/x86/kernel/cpu/scattered.c
> +++ b/arch/x86/kernel/cpu/scattered.c
> @@ -41,6 +41,7 @@ static const struct cpuid_bit cpuid_bits[] = {
>  	{ X86_FEATURE_PER_THREAD_MBA,		CPUID_ECX,  0, 0x00000010, 3 },
>  	{ X86_FEATURE_SGX1,			CPUID_EAX,  0, 0x00000012, 0 },
>  	{ X86_FEATURE_SGX2,			CPUID_EAX,  1, 0x00000012, 0 },
> +	{ X86_FEATURE_SGX_EUPDATESVN,	CPUID_EAX, 10, 0x00000012, 0 },
>  	{ X86_FEATURE_SGX_EDECCSSA,		CPUID_EAX, 11, 0x00000012, 0 },
>  	{ X86_FEATURE_HW_PSTATE,		CPUID_EDX,  7, 0x80000007, 0 },
>  	{ X86_FEATURE_CPB,			CPUID_EDX,  9, 0x80000007, 0 },

So it should be obvious at a glance that this new line doesn't follow 
the coding convention of the surrounding lines...

Thanks,

	Ingo

Re: [PATCH v5 5/5] x86/sgx: Enable automatic SVN updates for SGX enclaves

[Ingo Molnar]

* Elena Reshetova <elena.reshetova@intel.com> wrote:

> @@ -19,10 +19,15 @@ static int sgx_open(struct inode *inode, struct file *file)
>  	struct sgx_encl *encl;
>  	int ret;
>  
> +	ret = sgx_inc_usage_count();
> +	if (ret)
> +		return -EBUSY;

So if sgx_inc_usage_count() returns nonzero, it's in use already and we 
return -EBUSY, right?

But:

>  int sgx_inc_usage_count(void)
>  {
> +	int ret;
> +
> +	/*
> +	 * Increments from non-zero indicate EPC other
> +	 * active EPC users and EUPDATESVN is not attempted.
> +	 */
> +	if (atomic64_inc_not_zero(&sgx_usage_count))
> +		return 0;

If sgx_usage_count is 1 here (ie. it's busy), this will return *zero*, 
and sgx_open() will not run into the -EBUSY condition and will continue 
assuming it has claimed the usage count, while it hasn't ...

Furthermore, in the sgx_open() error paths we can then run into 
sgx_dec_usage_count(), which will merrily underflow the counter into 
negative:

 +void sgx_dec_usage_count(void)
 +{
 +       atomic64_dec(&sgx_usage_count);
 +}

How is this all supposed to work?

Thanks,

	Ingo

Re: [PATCH v5 1/5] x86/sgx: Introduce a counter to count the sgx_(vepc_)open()

[Huang, Kai]

On Mon, 2025-05-19 at 10:24 +0300, Elena Reshetova wrote:
> Currently SGX does not have a global counter to count the
> active users from userspace or hypervisor. Implement such a counter,
> sgx_usage_count. It will be used by the driver when attempting
> to call EUPDATESVN SGX instruction.
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> ---
>  arch/x86/kernel/cpu/sgx/driver.c |  1 +
>  arch/x86/kernel/cpu/sgx/encl.c   |  1 +
>  arch/x86/kernel/cpu/sgx/main.c   | 14 ++++++++++++++
>  arch/x86/kernel/cpu/sgx/sgx.h    |  3 +++
>  arch/x86/kernel/cpu/sgx/virt.c   |  2 ++
>  5 files changed, 21 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c
> index 7f8d1e11dbee..b5ffe104af4c 100644
> --- a/arch/x86/kernel/cpu/sgx/driver.c
> +++ b/arch/x86/kernel/cpu/sgx/driver.c
> @@ -19,6 +19,7 @@ static int sgx_open(struct inode *inode, struct file *file)
>  	struct sgx_encl *encl;
>  	int ret;
>  
> +	sgx_inc_usage_count();

This should be done at the end of sgx_open() where the open cannot fail, since
sgx_release() is not called if sgx_open() failed AFAICT.

>  	encl = kzalloc(sizeof(*encl), GFP_KERNEL);
>  	if (!encl)
>  		return -ENOMEM;
> diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> index 279148e72459..3b54889ae4a4 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.c
> +++ b/arch/x86/kernel/cpu/sgx/encl.c
> @@ -765,6 +765,7 @@ void sgx_encl_release(struct kref *ref)
>  	WARN_ON_ONCE(encl->secs.epc_page);
>  
>  	kfree(encl);
> +	sgx_dec_usage_count();
>  }
>  
>  /*
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index 8ce352fc72ac..80d565e6f2ad 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -914,6 +914,20 @@ int sgx_set_attribute(unsigned long *allowed_attributes,
>  }
>  EXPORT_SYMBOL_GPL(sgx_set_attribute);
>  
> +/* Counter to count the active SGX users */
> +static atomic64_t sgx_usage_count;
> +
> +int sgx_inc_usage_count(void)
> +{
> +	atomic64_inc(&sgx_usage_count);
> +	return 0;
> +}

What's the purpose of the returning int?  Seems no caller uses the return value.

> +
> +void sgx_dec_usage_count(void)
> +{
> +	atomic64_dec(&sgx_usage_count);
> +}
> +
>  static int __init sgx_init(void)
>  {
>  	int ret;
> diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
> index d2dad21259a8..f5940393d9bd 100644
> --- a/arch/x86/kernel/cpu/sgx/sgx.h
> +++ b/arch/x86/kernel/cpu/sgx/sgx.h
> @@ -102,6 +102,9 @@ static inline int __init sgx_vepc_init(void)
>  }
>  #endif
>  
> +int sgx_inc_usage_count(void);
> +void sgx_dec_usage_count(void);
> +
>  void sgx_update_lepubkeyhash(u64 *lepubkeyhash);
>  
>  #endif /* _X86_SGX_H */
> diff --git a/arch/x86/kernel/cpu/sgx/virt.c b/arch/x86/kernel/cpu/sgx/virt.c
> index 7aaa3652e31d..83de0907f32c 100644
> --- a/arch/x86/kernel/cpu/sgx/virt.c
> +++ b/arch/x86/kernel/cpu/sgx/virt.c
> @@ -255,6 +255,7 @@ static int sgx_vepc_release(struct inode *inode, struct file *file)
>  	xa_destroy(&vepc->page_array);
>  	kfree(vepc);
>  
> +	sgx_dec_usage_count();
>  	return 0;
>  }
>  
> @@ -262,6 +263,7 @@ static int sgx_vepc_open(struct inode *inode, struct file *file)
>  {
>  	struct sgx_vepc *vepc;
>  
> +	sgx_inc_usage_count();

Ditto to sgx_open().

>  	vepc = kzalloc(sizeof(struct sgx_vepc), GFP_KERNEL);
>  	if (!vepc)
>  		return -ENOMEM;

Re: [PATCH v5 2/5] x86/cpufeatures: Add X86_FEATURE_SGX_EUPDATESVN feature flag

[Huang, Kai]

On Mon, 2025-05-19 at 10:24 +0300, Elena Reshetova wrote:
> Add a flag indicating whenever ENCLS[EUPDATESVN] SGX instruction
> is supported. This will be used by SGX driver to perform CPU
> SVN updates.
> 
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> ---
>  arch/x86/include/asm/cpufeatures.h       | 1 +
>  arch/x86/kernel/cpu/scattered.c          | 1 +
>  tools/arch/x86/include/asm/cpufeatures.h | 1 +
>  3 files changed, 3 insertions(+)
> 
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index 6c2c152d8a67..ed0c0fa5822a 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -481,6 +481,7 @@
>  #define X86_FEATURE_AMD_HETEROGENEOUS_CORES (21*32 + 6) /* Heterogeneous Core Topology */
>  #define X86_FEATURE_AMD_WORKLOAD_CLASS	(21*32 + 7) /* Workload Classification */
>  #define X86_FEATURE_PREFER_YMM		(21*32 + 8) /* Avoid ZMM registers due to downclocking */
> +#define X86_FEATURE_SGX_EUPDATESVN	(21*32 + 9) /* Support for ENCLS[EUPDATESVN] instruction */
>  
>  /*
>   * BUG word(s)
> diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
> index 16f3ca30626a..a7e1fcedca3c 100644
> --- a/arch/x86/kernel/cpu/scattered.c
> +++ b/arch/x86/kernel/cpu/scattered.c
> @@ -41,6 +41,7 @@ static const struct cpuid_bit cpuid_bits[] = {
>  	{ X86_FEATURE_PER_THREAD_MBA,		CPUID_ECX,  0, 0x00000010, 3 },
>  	{ X86_FEATURE_SGX1,			CPUID_EAX,  0, 0x00000012, 0 },
>  	{ X86_FEATURE_SGX2,			CPUID_EAX,  1, 0x00000012, 0 },
> +	{ X86_FEATURE_SGX_EUPDATESVN,	CPUID_EAX, 10, 0x00000012, 0 },
>  	{ X86_FEATURE_SGX_EDECCSSA,		CPUID_EAX, 11, 0x00000012, 0 },
>  	{ X86_FEATURE_HW_PSTATE,		CPUID_EDX,  7, 0x80000007, 0 },
>  	{ X86_FEATURE_CPB,			CPUID_EDX,  9, 0x80000007, 0 },
> diff --git a/tools/arch/x86/include/asm/cpufeatures.h b/tools/arch/x86/include/asm/cpufeatures.h
> index 6c2c152d8a67..ed0c0fa5822a 100644
> --- a/tools/arch/x86/include/asm/cpufeatures.h
> +++ b/tools/arch/x86/include/asm/cpufeatures.h
> @@ -481,6 +481,7 @@
>  #define X86_FEATURE_AMD_HETEROGENEOUS_CORES (21*32 + 6) /* Heterogeneous Core Topology */
>  #define X86_FEATURE_AMD_WORKLOAD_CLASS	(21*32 + 7) /* Workload Classification */
>  #define X86_FEATURE_PREFER_YMM		(21*32 + 8) /* Avoid ZMM registers due to downclocking */
> +#define X86_FEATURE_SGX_EUPDATESVN	(21*32 + 9) /* Support for ENCLS[EUPDATESVN] instruction */
>  
> 


Additionally, the new feature bit should be added to the CPUID dependency table:

--- a/arch/x86/kernel/cpu/cpuid-deps.c
+++ b/arch/x86/kernel/cpu/cpuid-deps.c
@@ -78,6 +78,7 @@ static const struct cpuid_dep cpuid_deps[] = {
        { X86_FEATURE_SGX_LC,                   X86_FEATURE_SGX       },
        { X86_FEATURE_SGX1,                     X86_FEATURE_SGX       },
        { X86_FEATURE_SGX2,                     X86_FEATURE_SGX1      },
+       { X86_FEATURE_SGX_EUPDATESVN,           X86_FEATURE_SGX1      },
        { X86_FEATURE_SGX_EDECCSSA,             X86_FEATURE_SGX1      },
        { X86_FEATURE_XFD,                      X86_FEATURE_XSAVES    },
        { X86_FEATURE_XFD,                      X86_FEATURE_XGETBV1   },

Re: [PATCH v5 3/5] x86/sgx: Define error codes for use by ENCLS[EUPDATESVN]

[Huang, Kai]

On Mon, 2025-05-19 at 10:24 +0300, Elena Reshetova wrote:
> Add error codes for ENCLS[EUPDATESVN], then SGX CPUSVN update
> process can know the execution state of EUPDATESVN and notify
> userspace.
> 
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> ---

[...]

> 
>  /**
> @@ -73,6 +74,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 +87,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,
>  };
>  

It doesn't seem SGX_EPC_NOT_READY is used in this series.

RE: [PATCH v5 5/5] x86/sgx: Enable automatic SVN updates for SGX enclaves

[Reshetova, Elena]

 
> * Elena Reshetova <elena.reshetova@intel.com> wrote:
> 
> > @@ -19,10 +19,15 @@ static int sgx_open(struct inode *inode, struct file
> *file)
> >  	struct sgx_encl *encl;
> >  	int ret;
> >
> > +	ret = sgx_inc_usage_count();
> > +	if (ret)
> > +		return -EBUSY;
> 
> So if sgx_inc_usage_count() returns nonzero, it's in use already and we
> return -EBUSY, right?

I guess my selection of error code here was wrong. 
The intended logic is if sgx_inc_usage_count() returns nonzero,
the *incrementing of counter failed* (due to failed EUPDATESVN)
and we want to stop and report error.
  
> 
> But:
> 
> >  int sgx_inc_usage_count(void)
> >  {
> > +	int ret;
> > +
> > +	/*
> > +	 * Increments from non-zero indicate EPC other
> > +	 * active EPC users and EUPDATESVN is not attempted.
> > +	 */
> > +	if (atomic64_inc_not_zero(&sgx_usage_count))
> > +		return 0;
> 
> If sgx_usage_count is 1 here (ie. it's busy), this will return *zero*,
> and sgx_open() will not run into the -EBUSY condition and will continue
> assuming it has claimed the usage count, while it hasn't ...

Yes, meaning is different, see above. 

> 
> Furthermore, in the sgx_open() error paths we can then run into

What error paths? In case sgx_inc_usage_count() fails, we exit
immediately. 

> sgx_dec_usage_count(), which will merrily underflow the counter into
> negative:
> 
>  +void sgx_dec_usage_count(void)
>  +{
>  +       atomic64_dec(&sgx_usage_count);
>  +}
> 
> How is this all supposed to work?

Looks like I need more explanation on the counter and a better error
returned to userspace than -EBUSY. Maybe EIO then? 

Best Regards,
Elena.

> 
> Thanks,
> 
> 	Ingo

RE: [PATCH v5 2/5] x86/cpufeatures: Add X86_FEATURE_SGX_EUPDATESVN feature flag

[Reshetova, Elena]

> * Elena Reshetova <elena.reshetova@intel.com> wrote:
> 
> > Add a flag indicating whenever ENCLS[EUPDATESVN] SGX instruction
> > is supported. This will be used by SGX driver to perform CPU
> > SVN updates.
> >
> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> > ---
> >  arch/x86/include/asm/cpufeatures.h       | 1 +
> >  arch/x86/kernel/cpu/scattered.c          | 1 +
> >  tools/arch/x86/include/asm/cpufeatures.h | 1 +
> >  3 files changed, 3 insertions(+)
> >
> > diff --git a/arch/x86/include/asm/cpufeatures.h
> b/arch/x86/include/asm/cpufeatures.h
> > index 6c2c152d8a67..ed0c0fa5822a 100644
> > --- a/arch/x86/include/asm/cpufeatures.h
> > +++ b/arch/x86/include/asm/cpufeatures.h
> > @@ -481,6 +481,7 @@
> >  #define X86_FEATURE_AMD_HETEROGENEOUS_CORES (21*32 + 6) /*
> Heterogeneous Core Topology */
> >  #define X86_FEATURE_AMD_WORKLOAD_CLASS	(21*32 + 7) /*
> Workload Classification */
> >  #define X86_FEATURE_PREFER_YMM		(21*32 + 8) /* Avoid ZMM
> registers due to downclocking */
> > +#define X86_FEATURE_SGX_EUPDATESVN	(21*32 + 9) /* Support for
> ENCLS[EUPDATESVN] instruction */
> 
> Please base the patches on the latest x86 tree, there's been two
> additions already in this area.

Will do in the next iteration.

> 
> >  /*
> >   * BUG word(s)
> > diff --git a/arch/x86/kernel/cpu/scattered.c
> b/arch/x86/kernel/cpu/scattered.c
> > index 16f3ca30626a..a7e1fcedca3c 100644
> > --- a/arch/x86/kernel/cpu/scattered.c
> > +++ b/arch/x86/kernel/cpu/scattered.c
> > @@ -41,6 +41,7 @@ static const struct cpuid_bit cpuid_bits[] = {
> >  	{ X86_FEATURE_PER_THREAD_MBA,		CPUID_ECX,  0,
> 0x00000010, 3 },
> >  	{ X86_FEATURE_SGX1,			CPUID_EAX,  0, 0x00000012, 0
> },
> >  	{ X86_FEATURE_SGX2,			CPUID_EAX,  1, 0x00000012, 0
> },
> > +	{ X86_FEATURE_SGX_EUPDATESVN,	CPUID_EAX, 10, 0x00000012,
> 0 },
> >  	{ X86_FEATURE_SGX_EDECCSSA,		CPUID_EAX, 11,
> 0x00000012, 0 },
> >  	{ X86_FEATURE_HW_PSTATE,		CPUID_EDX,  7, 0x80000007, 0
> },
> >  	{ X86_FEATURE_CPB,			CPUID_EDX,  9, 0x80000007, 0
> },
> 
> So it should be obvious at a glance that this new line doesn't follow
> the coding convention of the surrounding lines...

Will fix also. 

RE: [PATCH v5 2/5] x86/cpufeatures: Add X86_FEATURE_SGX_EUPDATESVN feature flag

[Reshetova, Elena]

 
> On Mon, 2025-05-19 at 10:24 +0300, Elena Reshetova wrote:
> > Add a flag indicating whenever ENCLS[EUPDATESVN] SGX instruction
> > is supported. This will be used by SGX driver to perform CPU
> > SVN updates.
> >
> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> > ---
> >  arch/x86/include/asm/cpufeatures.h       | 1 +
> >  arch/x86/kernel/cpu/scattered.c          | 1 +
> >  tools/arch/x86/include/asm/cpufeatures.h | 1 +
> >  3 files changed, 3 insertions(+)
> >
> > diff --git a/arch/x86/include/asm/cpufeatures.h
> b/arch/x86/include/asm/cpufeatures.h
> > index 6c2c152d8a67..ed0c0fa5822a 100644
> > --- a/arch/x86/include/asm/cpufeatures.h
> > +++ b/arch/x86/include/asm/cpufeatures.h
> > @@ -481,6 +481,7 @@
> >  #define X86_FEATURE_AMD_HETEROGENEOUS_CORES (21*32 + 6) /*
> Heterogeneous Core Topology */
> >  #define X86_FEATURE_AMD_WORKLOAD_CLASS	(21*32 + 7) /*
> Workload Classification */
> >  #define X86_FEATURE_PREFER_YMM		(21*32 + 8) /* Avoid ZMM
> registers due to downclocking */
> > +#define X86_FEATURE_SGX_EUPDATESVN	(21*32 + 9) /* Support for
> ENCLS[EUPDATESVN] instruction */
> >
> >  /*
> >   * BUG word(s)
> > diff --git a/arch/x86/kernel/cpu/scattered.c
> b/arch/x86/kernel/cpu/scattered.c
> > index 16f3ca30626a..a7e1fcedca3c 100644
> > --- a/arch/x86/kernel/cpu/scattered.c
> > +++ b/arch/x86/kernel/cpu/scattered.c
> > @@ -41,6 +41,7 @@ static const struct cpuid_bit cpuid_bits[] = {
> >  	{ X86_FEATURE_PER_THREAD_MBA,		CPUID_ECX,  0,
> 0x00000010, 3 },
> >  	{ X86_FEATURE_SGX1,			CPUID_EAX,  0, 0x00000012, 0
> },
> >  	{ X86_FEATURE_SGX2,			CPUID_EAX,  1, 0x00000012, 0
> },
> > +	{ X86_FEATURE_SGX_EUPDATESVN,	CPUID_EAX, 10, 0x00000012,
> 0 },
> >  	{ X86_FEATURE_SGX_EDECCSSA,		CPUID_EAX, 11,
> 0x00000012, 0 },
> >  	{ X86_FEATURE_HW_PSTATE,		CPUID_EDX,  7, 0x80000007, 0
> },
> >  	{ X86_FEATURE_CPB,			CPUID_EDX,  9, 0x80000007, 0
> },
> > diff --git a/tools/arch/x86/include/asm/cpufeatures.h
> b/tools/arch/x86/include/asm/cpufeatures.h
> > index 6c2c152d8a67..ed0c0fa5822a 100644
> > --- a/tools/arch/x86/include/asm/cpufeatures.h
> > +++ b/tools/arch/x86/include/asm/cpufeatures.h
> > @@ -481,6 +481,7 @@
> >  #define X86_FEATURE_AMD_HETEROGENEOUS_CORES (21*32 + 6) /*
> Heterogeneous Core Topology */
> >  #define X86_FEATURE_AMD_WORKLOAD_CLASS	(21*32 + 7) /*
> Workload Classification */
> >  #define X86_FEATURE_PREFER_YMM		(21*32 + 8) /* Avoid ZMM
> registers due to downclocking */
> > +#define X86_FEATURE_SGX_EUPDATESVN	(21*32 + 9) /* Support for
> ENCLS[EUPDATESVN] instruction */
> >
> >
> 
> 
> Additionally, the new feature bit should be added to the CPUID dependency
> table:
> 
> --- a/arch/x86/kernel/cpu/cpuid-deps.c
> +++ b/arch/x86/kernel/cpu/cpuid-deps.c
> @@ -78,6 +78,7 @@ static const struct cpuid_dep cpuid_deps[] = {
>         { X86_FEATURE_SGX_LC,                   X86_FEATURE_SGX       },
>         { X86_FEATURE_SGX1,                     X86_FEATURE_SGX       },
>         { X86_FEATURE_SGX2,                     X86_FEATURE_SGX1      },
> +       { X86_FEATURE_SGX_EUPDATESVN,           X86_FEATURE_SGX1      },
>         { X86_FEATURE_SGX_EDECCSSA,             X86_FEATURE_SGX1      },
>         { X86_FEATURE_XFD,                      X86_FEATURE_XSAVES    },
>         { X86_FEATURE_XFD,                      X86_FEATURE_XGETBV1   },

This one I missed, will add. 

RE: [PATCH v5 3/5] x86/sgx: Define error codes for use by ENCLS[EUPDATESVN]

[Reshetova, Elena]

> On Mon, 2025-05-19 at 10:24 +0300, Elena Reshetova wrote:
> > Add error codes for ENCLS[EUPDATESVN], then SGX CPUSVN update
> > process can know the execution state of EUPDATESVN and notify
> > userspace.
> >
> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> > ---
> 
> [...]
> 
> >
> >  /**
> > @@ -73,6 +74,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 +87,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,
> >  };
> >
> 
> It doesn't seem SGX_EPC_NOT_READY is used in this series.


You are right, not anymore. However, it is a valid return code for the EUPDATESVN command.
Do we want to drop this one? 

Re: [PATCH v5 4/5] x86/sgx: Implement ENCLS[EUPDATESVN]

[Huang, Kai]

On Mon, 2025-05-19 at 10:24 +0300, Elena Reshetova wrote:
> 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.
> 
> Add the method to perform ENCLS[EUPDATESVN].
> 
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> ---
>  arch/x86/kernel/cpu/sgx/encls.h |  5 +++
>  arch/x86/kernel/cpu/sgx/main.c  | 57 +++++++++++++++++++++++++++++++++
>  2 files changed, 62 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/encls.h b/arch/x86/kernel/cpu/sgx/encls.h
> index 99004b02e2ed..d9160c89a93d 100644
> --- a/arch/x86/kernel/cpu/sgx/encls.h
> +++ b/arch/x86/kernel/cpu/sgx/encls.h
> @@ -233,4 +233,9 @@ static inline int __eaug(struct sgx_pageinfo *pginfo, void *addr)
>  	return __encls_2(EAUG, pginfo, addr);
>  }
>  
> +/* Attempt to 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 80d565e6f2ad..fd71e2ddca59 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -15,6 +15,7 @@
>  #include <linux/sysfs.h>
>  #include <linux/vmalloc.h>
>  #include <asm/sgx.h>
> +#include <asm/archrandom.h>
>  #include "driver.h"
>  #include "encl.h"
>  #include "encls.h"
> @@ -917,6 +918,62 @@ EXPORT_SYMBOL_GPL(sgx_set_attribute);
>  /* Counter to count the active SGX users */
>  static atomic64_t sgx_usage_count;
>  
> +/**
> + * sgx_updatesvn() - Attempt to call ENCLS[EUPDATESVN]
> + * If EPC is empty, this instruction attempts to update CPUSVN to the
> + * currently loaded microcode update SVN and generate new
> + * cryptographic assets.sgx_updatesvn() Most of the time, there will
> + * be no update and that's OK.
> + *
> + * Return:
> + * 0: Success, not supported or run out of entropy
> + */
> +static int sgx_update_svn(void)
> +{
> +	int ret;
> +
> +	/*
> +	 * If EUPDATESVN is not available, it is ok to
> +	 * silently skip it to comply with legacy behavior.
> +	 */
> +	if (!X86_FEATURE_SGX_EUPDATESVN)
> +		return 0;

Should be:

	if (!cpu_feature_enabled(X86_FEATURE_SGX_EUPDATESVN))
		return 0;

> +
> +	for (int i = 0; i < RDRAND_RETRY_LOOPS; i++) {
> +		ret = __eupdatesvn();
> +
> +		/* Stop on success or unexpected errors: */
> +		if (ret != SGX_INSUFFICIENT_ENTROPY)
> +			break;
> +	}
> +
> +	/*
> +	 * SVN either was up-to-date or SVN update failed due
> +	 * to lack of entropy. In both cases, we want to return
> +	 * 0 in order not to break sgx_(vepc_)open. We dont expect
> +	 * SGX_INSUFFICIENT_ENTROPY error unless underlying RDSEED
> +	 * is under heavy pressure.
> +	 */
> +	if ((ret == SGX_NO_UPDATE) || (ret == SGX_INSUFFICIENT_ENTROPY))
> +		return 0;

I am a little bit confused why we should return 0 when running out of entropy.

It seems in v4 you said it's not that hard to cause EUPDATESVN to fail reliably:

    And to make it more concrete, I made some simple tests based
    on program for stress testing rdrand/rdseed that was discussed in that
    threat earlier: https://lkml.org/lkml/2024/2/6/746 
    Using this stress testing and enough threads, I can make EUPDATESVN fail
    reliably and quite easily even with 10 time retry loop by kernel. 

Returning 0 will make sgx_open() succeed if I read your next patch correctly,
which means this will allow enclave to be created when updating SVN fails.

Why not just fail sgx_open() (e.g., return -EBUSY, or -EAGAIN) in that case?  
Userspace can then retry?

> +
> +	if (!ret) {
> +		/*
> +		 * SVN successfully updated.
> +		 * Let users know when the update was successful.
> +		 */
> +		pr_info("SVN updated successfully\n");
> +		return 0;
> +	}
> +
> +	/*
> +	 * EUPDATESVN was called when EPC is empty, all other error
> +	 * codes are unexpected.
> +	 */
> +	ENCLS_WARN(ret, "EUPDATESVN");
> +	return ret;
> +}
> +
>  int sgx_inc_usage_count(void)
>  {
>  	atomic64_inc(&sgx_usage_count);

Re: [PATCH v5 1/5] x86/sgx: Introduce a counter to count the sgx_(vepc_)open()

[Huang, Kai]

On Mon, 2025-05-19 at 10:47 +0000, Huang, Kai wrote:
> > +/* Counter to count the active SGX users */
> > +static atomic64_t sgx_usage_count;
> > +
> > +int sgx_inc_usage_count(void)
> > +{
> > +	atomic64_inc(&sgx_usage_count);
> > +	return 0;
> > +}
> 
> What's the purpose of the returning int?  Seems no caller uses the return value.

I saw your last patch uses the returning value.  Perhaps add a sentence to
mention in the changelog, otherwise it's kinda out of blue.

Re: [PATCH v5 3/5] x86/sgx: Define error codes for use by ENCLS[EUPDATESVN]

[Huang, Kai]

On Mon, 2025-05-19 at 11:30 +0000, Reshetova, Elena wrote:
> > On Mon, 2025-05-19 at 10:24 +0300, Elena Reshetova wrote:
> > > Add error codes for ENCLS[EUPDATESVN], then SGX CPUSVN update
> > > process can know the execution state of EUPDATESVN and notify
> > > userspace.
> > > 
> > > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> > > ---
> > 
> > [...]
> > 
> > > 
> > >  /**
> > > @@ -73,6 +74,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 +87,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,
> > >  };
> > > 
> > 
> > It doesn't seem SGX_EPC_NOT_READY is used in this series.
> 
> 
> You are right, not anymore. However, it is a valid return code for the EUPDATESVN command.
> Do we want to drop this one? 

I think we should drop if it is not used.

RE: [PATCH v5 4/5] x86/sgx: Implement ENCLS[EUPDATESVN]

[Reshetova, Elena]

> On Mon, 2025-05-19 at 10:24 +0300, Elena Reshetova wrote:
> > 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.
> >
> > Add the method to perform ENCLS[EUPDATESVN].
> >
> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> > ---
> >  arch/x86/kernel/cpu/sgx/encls.h |  5 +++
> >  arch/x86/kernel/cpu/sgx/main.c  | 57
> +++++++++++++++++++++++++++++++++
> >  2 files changed, 62 insertions(+)
> >
> > diff --git a/arch/x86/kernel/cpu/sgx/encls.h
> b/arch/x86/kernel/cpu/sgx/encls.h
> > index 99004b02e2ed..d9160c89a93d 100644
> > --- a/arch/x86/kernel/cpu/sgx/encls.h
> > +++ b/arch/x86/kernel/cpu/sgx/encls.h
> > @@ -233,4 +233,9 @@ static inline int __eaug(struct sgx_pageinfo *pginfo,
> void *addr)
> >  	return __encls_2(EAUG, pginfo, addr);
> >  }
> >
> > +/* Attempt to 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 80d565e6f2ad..fd71e2ddca59 100644
> > --- a/arch/x86/kernel/cpu/sgx/main.c
> > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > @@ -15,6 +15,7 @@
> >  #include <linux/sysfs.h>
> >  #include <linux/vmalloc.h>
> >  #include <asm/sgx.h>
> > +#include <asm/archrandom.h>
> >  #include "driver.h"
> >  #include "encl.h"
> >  #include "encls.h"
> > @@ -917,6 +918,62 @@ EXPORT_SYMBOL_GPL(sgx_set_attribute);
> >  /* Counter to count the active SGX users */
> >  static atomic64_t sgx_usage_count;
> >
> > +/**
> > + * sgx_updatesvn() - Attempt to call ENCLS[EUPDATESVN]
> > + * If EPC is empty, this instruction attempts to update CPUSVN to the
> > + * currently loaded microcode update SVN and generate new
> > + * cryptographic assets.sgx_updatesvn() Most of the time, there will
> > + * be no update and that's OK.
> > + *
> > + * Return:
> > + * 0: Success, not supported or run out of entropy
> > + */
> > +static int sgx_update_svn(void)
> > +{
> > +	int ret;
> > +
> > +	/*
> > +	 * If EUPDATESVN is not available, it is ok to
> > +	 * silently skip it to comply with legacy behavior.
> > +	 */
> > +	if (!X86_FEATURE_SGX_EUPDATESVN)
> > +		return 0;
> 
> Should be:
> 
> 	if (!cpu_feature_enabled(X86_FEATURE_SGX_EUPDATESVN))
> 		return 0;

Yes, right. Will fix. 

> 
> > +
> > +	for (int i = 0; i < RDRAND_RETRY_LOOPS; i++) {
> > +		ret = __eupdatesvn();
> > +
> > +		/* Stop on success or unexpected errors: */
> > +		if (ret != SGX_INSUFFICIENT_ENTROPY)
> > +			break;
> > +	}
> > +
> > +	/*
> > +	 * SVN either was up-to-date or SVN update failed due
> > +	 * to lack of entropy. In both cases, we want to return
> > +	 * 0 in order not to break sgx_(vepc_)open. We dont expect
> > +	 * SGX_INSUFFICIENT_ENTROPY error unless underlying RDSEED
> > +	 * is under heavy pressure.
> > +	 */
> > +	if ((ret == SGX_NO_UPDATE) || (ret == SGX_INSUFFICIENT_ENTROPY))
> > +		return 0;
> 
> I am a little bit confused why we should return 0 when running out of
> entropy.
> 
> It seems in v4 you said it's not that hard to cause EUPDATESVN to fail reliably:
> 
>     And to make it more concrete, I made some simple tests based
>     on program for stress testing rdrand/rdseed that was discussed in that
>     threat earlier: https://lkml.org/lkml/2024/2/6/746
>     Using this stress testing and enough threads, I can make EUPDATESVN fail
>     reliably and quite easily even with 10 time retry loop by kernel.
> 
> Returning 0 will make sgx_open() succeed if I read your next patch correctly,
> which means this will allow enclave to be created when updating SVN fails.

Yes, correct. 

> 
> Why not just fail sgx_open() (e.g., return -EBUSY, or -EAGAIN) in that case?
> Userspace can then retry?

The idea on the patch was that such a scenario where we run out of entropy
should not happen in real life unless RDSEED is under stress (in case we 
accidentally collided, we do a 10 time retry). So, in this case we keep the legacy
behaviour, i.e. proceeding without EUPDATESVN. But I can change to the above
logic to return -EAGAIN in this case if everyone thinks it is a better approach. 

Best Regards,
Elena.

> 
> > +
> > +	if (!ret) {
> > +		/*
> > +		 * SVN successfully updated.
> > +		 * Let users know when the update was successful.
> > +		 */
> > +		pr_info("SVN updated successfully\n");
> > +		return 0;
> > +	}
> > +
> > +	/*
> > +	 * EUPDATESVN was called when EPC is empty, all other error
> > +	 * codes are unexpected.
> > +	 */
> > +	ENCLS_WARN(ret, "EUPDATESVN");
> > +	return ret;
> > +}
> > +
> >  int sgx_inc_usage_count(void)
> >  {
> >  	atomic64_inc(&sgx_usage_count);

RE: [PATCH v5 1/5] x86/sgx: Introduce a counter to count the sgx_(vepc_)open()

[Reshetova, Elena]

> On Mon, 2025-05-19 at 10:47 +0000, Huang, Kai wrote:
> > > +/* Counter to count the active SGX users */
> > > +static atomic64_t sgx_usage_count;
> > > +
> > > +int sgx_inc_usage_count(void)
> > > +{
> > > +	atomic64_inc(&sgx_usage_count);
> > > +	return 0;
> > > +}
> >
> > What's the purpose of the returning int?  Seems no caller uses the return
> value.
> 
> I saw your last patch uses the returning value.  Perhaps add a sentence to
> mention in the changelog, otherwise it's kinda out of blue.

Yes, I can add a comment. I didn’t want to change the function definition. 

RE: [PATCH v5 1/5] x86/sgx: Introduce a counter to count the sgx_(vepc_)open()

[Reshetova, Elena]

> On Mon, 2025-05-19 at 10:24 +0300, Elena Reshetova wrote:
> > Currently SGX does not have a global counter to count the
> > active users from userspace or hypervisor. Implement such a counter,
> > sgx_usage_count. It will be used by the driver when attempting
> > to call EUPDATESVN SGX instruction.
> >
> > Suggested-by: Sean Christopherson <seanjc@google.com>
> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> > ---
> >  arch/x86/kernel/cpu/sgx/driver.c |  1 +
> >  arch/x86/kernel/cpu/sgx/encl.c   |  1 +
> >  arch/x86/kernel/cpu/sgx/main.c   | 14 ++++++++++++++
> >  arch/x86/kernel/cpu/sgx/sgx.h    |  3 +++
> >  arch/x86/kernel/cpu/sgx/virt.c   |  2 ++
> >  5 files changed, 21 insertions(+)
> >
> > diff --git a/arch/x86/kernel/cpu/sgx/driver.c
> b/arch/x86/kernel/cpu/sgx/driver.c
> > index 7f8d1e11dbee..b5ffe104af4c 100644
> > --- a/arch/x86/kernel/cpu/sgx/driver.c
> > +++ b/arch/x86/kernel/cpu/sgx/driver.c
> > @@ -19,6 +19,7 @@ static int sgx_open(struct inode *inode, struct file *file)
> >  	struct sgx_encl *encl;
> >  	int ret;
> >
> > +	sgx_inc_usage_count();
> 
> This should be done at the end of sgx_open() where the open cannot fail,
> since
> sgx_release() is not called if sgx_open() failed AFAICT.


Could you please elaborate a bit more on this? 
In case sgx_inc_usage_count fails, we dont allocate resources yet, so what is 
wrong? 

Best Regards,
Elena.

Re: [PATCH v5 5/5] x86/sgx: Enable automatic SVN updates for SGX enclaves

[Ingo Molnar]

* Reshetova, Elena <elena.reshetova@intel.com> wrote:

>  
> > * Elena Reshetova <elena.reshetova@intel.com> wrote:
> > 
> > > @@ -19,10 +19,15 @@ static int sgx_open(struct inode *inode, struct file
> > *file)
> > >  	struct sgx_encl *encl;
> > >  	int ret;
> > >
> > > +	ret = sgx_inc_usage_count();
> > > +	if (ret)
> > > +		return -EBUSY;
> > 
> > So if sgx_inc_usage_count() returns nonzero, it's in use already and we
> > return -EBUSY, right?
> 
> I guess my selection of error code here was wrong. 
> The intended logic is if sgx_inc_usage_count() returns nonzero,
> the *incrementing of counter failed* (due to failed EUPDATESVN)
> and we want to stop and report error.
>   
> > 
> > But:
> > 
> > >  int sgx_inc_usage_count(void)
> > >  {
> > > +	int ret;
> > > +
> > > +	/*
> > > +	 * Increments from non-zero indicate EPC other
> > > +	 * active EPC users and EUPDATESVN is not attempted.
> > > +	 */
> > > +	if (atomic64_inc_not_zero(&sgx_usage_count))
> > > +		return 0;
> > 
> > If sgx_usage_count is 1 here (ie. it's busy), this will return *zero*,
> > and sgx_open() will not run into the -EBUSY condition and will continue
> > assuming it has claimed the usage count, while it hasn't ...
> 
> Yes, meaning is different, see above. 

So that's rather convoluted:

	atomic64_inc_not_zero():   returns 1 on successful increase, 0 on failure
        sgx_inc_usage_count():     returns 0 on successful increase, 1 on failure
        sgx_open():                returns 0 on successful increase, -EBUSY on failure

Could we at least standardize sgx_inc_usage_count() on -EBUSY in the 
failure case, so it's a more obvious pattern:

+       ret = sgx_inc_usage_count();
+       if (ret < 0)
+               return ret;

Thanks,

	Ingo

Re: [PATCH v5 4/5] x86/sgx: Implement ENCLS[EUPDATESVN]

[Dave Hansen]

On 5/19/25 00:24, Elena Reshetova wrote:
> 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.

That's really bizarre text wrapping. You might want to have your editor
give it another go.

I also found this pretty hard to read, but a friendly AI chatbot helped
me rewrite it:

All running enclaves and cryptographic assets (such as internal SGX
encryption keys) are assumed to be compromised whenever an SGX-related
microcode update occurs. To mitigate this assumed compromise the new
supervisor SGX instruction ENCLS[EUPDATESVN] can generate fresh
cryptographic assets.

Before executing EUPDATESVN, all SGX memory must be marked as unused.
This requirement ensures that no potentially compromised enclave
survives the update and allows the system to safely regenerate
cryptographic assets.

> @@ -917,6 +918,62 @@ EXPORT_SYMBOL_GPL(sgx_set_attribute);
>  /* Counter to count the active SGX users */
>  static atomic64_t sgx_usage_count;
>  
> +/**
> + * sgx_updatesvn() - Attempt to call ENCLS[EUPDATESVN]
> + * If EPC is empty, this instruction attempts to update CPUSVN to the
> + * currently loaded microcode update SVN and generate new
> + * cryptographic assets.sgx_updatesvn() Most of the time, there will

				^ What happened here?

> + * be no update and that's OK.
> + *
> + * Return:
> + * 0: Success, not supported or run out of entropy
> + */

Good documentation will help folks know when they might call this. It
sets clear expectations. When is it safe to call? What do I do if it fails?

> +static int sgx_update_svn(void)
> +{
> +	int ret;
> +
> +	/*
> +	 * If EUPDATESVN is not available, it is ok to
> +	 * silently skip it to comply with legacy behavior.
> +	 */
> +	if (!X86_FEATURE_SGX_EUPDATESVN)
> +		return 0;

At this point, it's pretty obvious that this code hasn't been tested.
This code would (I think) croak on any SGX users on non-EUPDATESVN
hardware. I'm going to stop reviewing this now. You've gotten quite a
bit of feedback on it and I think I'll wait for v6, which I'm sure will
have a few more correctness passes on it before going out.

> +
> +	for (int i = 0; i < RDRAND_RETRY_LOOPS; i++) {
> +		ret = __eupdatesvn();
> +
> +		/* Stop on success or unexpected errors: */
> +		if (ret != SGX_INSUFFICIENT_ENTROPY)
> +			break;
> +	}
> +
> +	/*
> +	 * SVN either was up-to-date or SVN update failed due
> +	 * to lack of entropy. In both cases, we want to return
> +	 * 0 in order not to break sgx_(vepc_)open. We dont expect

Remember, no "we's". Use imperative voice.

Re: [PATCH v5 1/5] x86/sgx: Introduce a counter to count the sgx_(vepc_)open()

[Jarkko Sakkinen]

On Mon, May 19, 2025 at 10:24:27AM +0300, Elena Reshetova wrote:
> Currently SGX does not have a global counter to count the
> active users from userspace or hypervisor. Implement such a counter,
> sgx_usage_count. It will be used by the driver when attempting
> to call EUPDATESVN SGX instruction.
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> ---
>  arch/x86/kernel/cpu/sgx/driver.c |  1 +
>  arch/x86/kernel/cpu/sgx/encl.c   |  1 +
>  arch/x86/kernel/cpu/sgx/main.c   | 14 ++++++++++++++
>  arch/x86/kernel/cpu/sgx/sgx.h    |  3 +++
>  arch/x86/kernel/cpu/sgx/virt.c   |  2 ++
>  5 files changed, 21 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c
> index 7f8d1e11dbee..b5ffe104af4c 100644
> --- a/arch/x86/kernel/cpu/sgx/driver.c
> +++ b/arch/x86/kernel/cpu/sgx/driver.c
> @@ -19,6 +19,7 @@ static int sgx_open(struct inode *inode, struct file *file)
>  	struct sgx_encl *encl;
>  	int ret;
>  
> +	sgx_inc_usage_count();
>  	encl = kzalloc(sizeof(*encl), GFP_KERNEL);
>  	if (!encl)
>  		return -ENOMEM;
> diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> index 279148e72459..3b54889ae4a4 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.c
> +++ b/arch/x86/kernel/cpu/sgx/encl.c
> @@ -765,6 +765,7 @@ void sgx_encl_release(struct kref *ref)
>  	WARN_ON_ONCE(encl->secs.epc_page);
>  
>  	kfree(encl);
> +	sgx_dec_usage_count();
>  }
>  
>  /*
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index 8ce352fc72ac..80d565e6f2ad 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -914,6 +914,20 @@ int sgx_set_attribute(unsigned long *allowed_attributes,
>  }
>  EXPORT_SYMBOL_GPL(sgx_set_attribute);
>  
> +/* Counter to count the active SGX users */
> +static atomic64_t sgx_usage_count;
> +
> +int sgx_inc_usage_count(void)
> +{
> +	atomic64_inc(&sgx_usage_count);
> +	return 0;
> +}
> +
> +void sgx_dec_usage_count(void)
> +{
> +	atomic64_dec(&sgx_usage_count);
> +}
> +
>  static int __init sgx_init(void)
>  {
>  	int ret;
> diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
> index d2dad21259a8..f5940393d9bd 100644
> --- a/arch/x86/kernel/cpu/sgx/sgx.h
> +++ b/arch/x86/kernel/cpu/sgx/sgx.h
> @@ -102,6 +102,9 @@ static inline int __init sgx_vepc_init(void)
>  }
>  #endif
>  
> +int sgx_inc_usage_count(void);
> +void sgx_dec_usage_count(void);
> +
>  void sgx_update_lepubkeyhash(u64 *lepubkeyhash);
>  
>  #endif /* _X86_SGX_H */
> diff --git a/arch/x86/kernel/cpu/sgx/virt.c b/arch/x86/kernel/cpu/sgx/virt.c
> index 7aaa3652e31d..83de0907f32c 100644
> --- a/arch/x86/kernel/cpu/sgx/virt.c
> +++ b/arch/x86/kernel/cpu/sgx/virt.c
> @@ -255,6 +255,7 @@ static int sgx_vepc_release(struct inode *inode, struct file *file)
>  	xa_destroy(&vepc->page_array);
>  	kfree(vepc);
>  
> +	sgx_dec_usage_count();
>  	return 0;
>  }
>  
> @@ -262,6 +263,7 @@ static int sgx_vepc_open(struct inode *inode, struct file *file)
>  {
>  	struct sgx_vepc *vepc;
>  
> +	sgx_inc_usage_count();
>  	vepc = kzalloc(sizeof(struct sgx_vepc), GFP_KERNEL);
>  	if (!vepc)
>  		return -ENOMEM;
> -- 
> 2.45.2
> 

Maybe just use raw atomic_inc() and atomic_dec() at the sites?

IMHO, it makes only sense to wrap, when it makes sense to wrap.

BR, Jarkko

Re: [PATCH v5 1/5] x86/sgx: Introduce a counter to count the sgx_(vepc_)open()

[Jarkko Sakkinen]

On Mon, May 19, 2025 at 11:47:32AM +0000, Reshetova, Elena wrote:
> > On Mon, 2025-05-19 at 10:24 +0300, Elena Reshetova wrote:
> > > Currently SGX does not have a global counter to count the
> > > active users from userspace or hypervisor. Implement such a counter,
> > > sgx_usage_count. It will be used by the driver when attempting
> > > to call EUPDATESVN SGX instruction.
> > >
> > > Suggested-by: Sean Christopherson <seanjc@google.com>
> > > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> > > ---
> > >  arch/x86/kernel/cpu/sgx/driver.c |  1 +
> > >  arch/x86/kernel/cpu/sgx/encl.c   |  1 +
> > >  arch/x86/kernel/cpu/sgx/main.c   | 14 ++++++++++++++
> > >  arch/x86/kernel/cpu/sgx/sgx.h    |  3 +++
> > >  arch/x86/kernel/cpu/sgx/virt.c   |  2 ++
> > >  5 files changed, 21 insertions(+)
> > >
> > > diff --git a/arch/x86/kernel/cpu/sgx/driver.c
> > b/arch/x86/kernel/cpu/sgx/driver.c
> > > index 7f8d1e11dbee..b5ffe104af4c 100644
> > > --- a/arch/x86/kernel/cpu/sgx/driver.c
> > > +++ b/arch/x86/kernel/cpu/sgx/driver.c
> > > @@ -19,6 +19,7 @@ static int sgx_open(struct inode *inode, struct file *file)
> > >  	struct sgx_encl *encl;
> > >  	int ret;
> > >
> > > +	sgx_inc_usage_count();
> > 
> > This should be done at the end of sgx_open() where the open cannot fail,
> > since
> > sgx_release() is not called if sgx_open() failed AFAICT.
> 
> 
> Could you please elaborate a bit more on this? 
> In case sgx_inc_usage_count fails, we dont allocate resources yet, so what is 
> wrong? 

It's fine to do (or even perhaps advicable) to do right at the get go,
before doing anything else, because it is "keep alive counter". I.e.,
I think the call is at the right place.

And now there is a proper rollback procedure, as my previous review
comments have been adressed.

> 
> Best Regards,
> Elena.


BR, Jarkko

Re: [PATCH v5 4/5] x86/sgx: Implement ENCLS[EUPDATESVN]

[Jarkko Sakkinen]

On Mon, May 19, 2025 at 10:24:30AM +0300, Elena Reshetova wrote:
> 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.
> 
> Add the method to perform ENCLS[EUPDATESVN].
> 
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> ---
>  arch/x86/kernel/cpu/sgx/encls.h |  5 +++
>  arch/x86/kernel/cpu/sgx/main.c  | 57 +++++++++++++++++++++++++++++++++
>  2 files changed, 62 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/encls.h b/arch/x86/kernel/cpu/sgx/encls.h
> index 99004b02e2ed..d9160c89a93d 100644
> --- a/arch/x86/kernel/cpu/sgx/encls.h
> +++ b/arch/x86/kernel/cpu/sgx/encls.h
> @@ -233,4 +233,9 @@ static inline int __eaug(struct sgx_pageinfo *pginfo, void *addr)
>  	return __encls_2(EAUG, pginfo, addr);
>  }
>  
> +/* Attempt to 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 80d565e6f2ad..fd71e2ddca59 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -15,6 +15,7 @@
>  #include <linux/sysfs.h>
>  #include <linux/vmalloc.h>
>  #include <asm/sgx.h>
> +#include <asm/archrandom.h>
>  #include "driver.h"
>  #include "encl.h"
>  #include "encls.h"
> @@ -917,6 +918,62 @@ EXPORT_SYMBOL_GPL(sgx_set_attribute);
>  /* Counter to count the active SGX users */
>  static atomic64_t sgx_usage_count;
>  
> +/**
> + * sgx_updatesvn() - Attempt to call ENCLS[EUPDATESVN]
> + * If EPC is empty, this instruction attempts to update CPUSVN to the
> + * currently loaded microcode update SVN and generate new
> + * cryptographic assets.sgx_updatesvn() Most of the time, there will

Is there something wrong here in the text? It looks malformed.

> + * be no update and that's OK.
> + *
> + * Return:
> + * 0: Success, not supported or run out of entropy
> + */
> +static int sgx_update_svn(void)
> +{
> +	int ret;
> +
> +	/*
> +	 * If EUPDATESVN is not available, it is ok to
> +	 * silently skip it to comply with legacy behavior.
> +	 */
> +	if (!X86_FEATURE_SGX_EUPDATESVN)
> +		return 0;
> +
> +	for (int i = 0; i < RDRAND_RETRY_LOOPS; i++) {
> +		ret = __eupdatesvn();
> +
> +		/* Stop on success or unexpected errors: */
> +		if (ret != SGX_INSUFFICIENT_ENTROPY)
> +			break;
> +	}
> +
> +	/*
> +	 * SVN either was up-to-date or SVN update failed due
> +	 * to lack of entropy. In both cases, we want to return
> +	 * 0 in order not to break sgx_(vepc_)open. We dont expect
> +	 * SGX_INSUFFICIENT_ENTROPY error unless underlying RDSEED
> +	 * is under heavy pressure.
> +	 */
> +	if ((ret == SGX_NO_UPDATE) || (ret == SGX_INSUFFICIENT_ENTROPY))
	
	if (ret == SGX_NO_UPDATE || ret == SGX_INSUFFICIENT_ENTROPY)

> +		return 0;
> +
> +	if (!ret) {
> +		/*
> +		 * SVN successfully updated.
> +		 * Let users know when the update was successful.
> +		 */

This comment is like as useless as an inline comment can ever possibly
be. Please, remove it.

> +		pr_info("SVN updated successfully\n");

Let's not add this either in the scope of this patch set.

> +		return 0;
> +	}

Since you parse error codes already, I don't understand why deal with
the success case in the middle of doing that.

More consistent would be (not also the use of unlikely()):

	if (ret == SGX_NO_UPDATE || ret == SGX_INSUFFICIENT_ENTROPY)
		return 0;

	/*
	 * EUPDATESVN was called when EPC is empty, all other error
	 * codes are unexpected.
	 */
	if (unlikely(ret)) {
		ENCLS_WARN(ret, "EUPDATESVN");
		return ret;
	}
		
	return 0;
}

This is how I would rewrite the tail of this function.

BR, Jarkko

Re: [PATCH v5 5/5] x86/sgx: Enable automatic SVN updates for SGX enclaves

[Jarkko Sakkinen]

On Mon, May 19, 2025 at 10:24:31AM +0300, Elena Reshetova wrote:
> 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 enables
> 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. CPUSVN is only automatically updated at reboot.
> This means that, although the microcode has been updated, enclaves can never
> attest to this fact. Enclaves are stuck attesting to the old version until
> a reboot.
> 
> The SGX architecture has an alternative to these reboots: the ENCLS[EUPDATESVN]
> instruction [1]. It allows another snapshot to be taken to update CPUSVN
> after a runtime microcode update 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.
> 
> Attempt to execute EUPDATESVN on the first open of sgx_(vepc)open().
> If EUPDATESVN fails with any other error code than SGX_INSUFFICIENT_ENTROPY,
> this is considered unexpected and the open() returns an error. This
> should not happen in practice. On contrary SGX_INSUFFICIENT_ENTROPY might
> happen due to a pressure on the system DRNG (RDSEED) and therefore
> the open() is not blocked to allow normal enclave operation.
> 
> [1] Runtime Microcode Updates with Intel Software Guard Extensions,
> https://cdrdv2.intel.com/v1/dl/getContent/648682

I'd hope, despite having the wealth of information in it, this commit
message would a go round or few of editing, and nail the gist of this
commit better. Then it would be easier in future review the choices
made.

BR, Jarkko

Re: [PATCH v5 1/5] x86/sgx: Introduce a counter to count the sgx_(vepc_)open()

[Huang, Kai]

On Mon, 2025-05-19 at 11:47 +0000, Reshetova, Elena wrote:
> > On Mon, 2025-05-19 at 10:24 +0300, Elena Reshetova wrote:
> > > Currently SGX does not have a global counter to count the
> > > active users from userspace or hypervisor. Implement such a counter,
> > > sgx_usage_count. It will be used by the driver when attempting
> > > to call EUPDATESVN SGX instruction.
> > > 
> > > Suggested-by: Sean Christopherson <seanjc@google.com>
> > > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> > > ---
> > >  arch/x86/kernel/cpu/sgx/driver.c |  1 +
> > >  arch/x86/kernel/cpu/sgx/encl.c   |  1 +
> > >  arch/x86/kernel/cpu/sgx/main.c   | 14 ++++++++++++++
> > >  arch/x86/kernel/cpu/sgx/sgx.h    |  3 +++
> > >  arch/x86/kernel/cpu/sgx/virt.c   |  2 ++
> > >  5 files changed, 21 insertions(+)
> > > 
> > > diff --git a/arch/x86/kernel/cpu/sgx/driver.c
> > b/arch/x86/kernel/cpu/sgx/driver.c
> > > index 7f8d1e11dbee..b5ffe104af4c 100644
> > > --- a/arch/x86/kernel/cpu/sgx/driver.c
> > > +++ b/arch/x86/kernel/cpu/sgx/driver.c
> > > @@ -19,6 +19,7 @@ static int sgx_open(struct inode *inode, struct file *file)
> > >  	struct sgx_encl *encl;
> > >  	int ret;
> > > 
> > > +	sgx_inc_usage_count();
> > 
> > This should be done at the end of sgx_open() where the open cannot fail,
> > since
> > sgx_release() is not called if sgx_open() failed AFAICT.
> 
> 
> Could you please elaborate a bit more on this? 
> In case sgx_inc_usage_count fails, we dont allocate resources yet, so what is 
> wrong? 
> 

I haven't looked into (details of) the last patch yet, but for _this_ patch
only, doing sgx_inc_usage_count() at the beginning of sgx_open() will result in
the usage count being increased even when sgx_open() fails at a later time. 
Since sgx_release() is not called when sgx_open() fails, the usage count will
not be decreased correctly.

No?

Re: [PATCH v5 4/5] x86/sgx: Implement ENCLS[EUPDATESVN]

[Huang, Kai]

> > 
> > > +
> > > +	for (int i = 0; i < RDRAND_RETRY_LOOPS; i++) {
> > > +		ret = __eupdatesvn();
> > > +
> > > +		/* Stop on success or unexpected errors: */
> > > +		if (ret != SGX_INSUFFICIENT_ENTROPY)
> > > +			break;
> > > +	}
> > > +
> > > +	/*
> > > +	 * SVN either was up-to-date or SVN update failed due
> > > +	 * to lack of entropy. In both cases, we want to return
> > > +	 * 0 in order not to break sgx_(vepc_)open. We dont expect
> > > +	 * SGX_INSUFFICIENT_ENTROPY error unless underlying RDSEED
> > > +	 * is under heavy pressure.
> > > +	 */
> > > +	if ((ret == SGX_NO_UPDATE) || (ret == SGX_INSUFFICIENT_ENTROPY))
> > > +		return 0;
> > 
> > I am a little bit confused why we should return 0 when running out of
> > entropy.
> > 
> > It seems in v4 you said it's not that hard to cause EUPDATESVN to fail reliably:
> > 
> >     And to make it more concrete, I made some simple tests based
> >     on program for stress testing rdrand/rdseed that was discussed in that
> >     threat earlier: https://lkml.org/lkml/2024/2/6/746
> >     Using this stress testing and enough threads, I can make EUPDATESVN fail
> >     reliably and quite easily even with 10 time retry loop by kernel.
> > 
> > Returning 0 will make sgx_open() succeed if I read your next patch correctly,
> > which means this will allow enclave to be created when updating SVN fails.
> 
> Yes, correct. 
> 
> > 
> > Why not just fail sgx_open() (e.g., return -EBUSY, or -EAGAIN) in that case?
> > Userspace can then retry?
> 
> The idea on the patch was that such a scenario where we run out of entropy
> should not happen in real life unless RDSEED is under stress (in case we 
> accidentally collided, we do a 10 time retry). So, in this case we keep the legacy
> behaviour, i.e. proceeding without EUPDATESVN. But I can change to the above
> logic to return -EAGAIN in this case if everyone thinks it is a better approach. 

Well I think I am seeing conflicting message:

You mentioned in v4 that some simple (userspace!) tests can make EUPDATESVN fail
"reliably and quite easily even with 10 time retry loop by kernel".  This seems
to me that "RDSEED is under stress" can certainly happen in in real life.

Or are you suggesting that kinda "simple tests" cannot happen in real life?

Even we agree that such test cannot happen in real life, since updating SVN is
about security, I think it's quite fair that we need to consider that the
platform is under attack.

Allowing enclave to be created when EUPDATESVN fails due to running out of
entropy is a clear violation of security to me.  And what's even worse is AFAICT
userspace is not notified about this by any means.

RE: [PATCH v5 1/5] x86/sgx: Introduce a counter to count the sgx_(vepc_)open()

[Reshetova, Elena]

> > > > diff --git a/arch/x86/kernel/cpu/sgx/driver.c
> > > b/arch/x86/kernel/cpu/sgx/driver.c
> > > > index 7f8d1e11dbee..b5ffe104af4c 100644
> > > > --- a/arch/x86/kernel/cpu/sgx/driver.c
> > > > +++ b/arch/x86/kernel/cpu/sgx/driver.c
> > > > @@ -19,6 +19,7 @@ static int sgx_open(struct inode *inode, struct file
> *file)
> > > >  	struct sgx_encl *encl;
> > > >  	int ret;
> > > >
> > > > +	sgx_inc_usage_count();
> > >
> > > This should be done at the end of sgx_open() where the open cannot fail,
> > > since
> > > sgx_release() is not called if sgx_open() failed AFAICT.
> >
> >
> > Could you please elaborate a bit more on this?
> > In case sgx_inc_usage_count fails, we dont allocate resources yet, so what is
> > wrong?
> >
> 
> I haven't looked into (details of) the last patch yet, but for _this_ patch
> only, doing sgx_inc_usage_count() at the beginning of sgx_open() will result in
> the usage count being increased even when sgx_open() fails at a later time.
> Since sgx_release() is not called when sgx_open() fails, the usage count will
> not be decreased correctly.
> 
> No?

Yes, I already yesterday understood that you meant *this* patch and
yes, you are correct in this patch it must be moved to the end that we
don't increment in case of failures. 

RE: [PATCH v5 1/5] x86/sgx: Introduce a counter to count the sgx_(vepc_)open()

[Reshetova, Elena]

> Maybe just use raw atomic_inc() and atomic_dec() at the sites?
> 
> IMHO, it makes only sense to wrap, when it makes sense to wrap.

You mean for this patch or overall? For overall we discussed in v4
why we would like to raise it to atomic64.
Or do I misunderstand your comment?

Best Regards,
Elena.

RE: [PATCH v5 4/5] x86/sgx: Implement ENCLS[EUPDATESVN]

[Reshetova, Elena]

 > +/**
> > + * sgx_updatesvn() - Attempt to call ENCLS[EUPDATESVN]
> > + * If EPC is empty, this instruction attempts to update CPUSVN to the
> > + * currently loaded microcode update SVN and generate new
> > + * cryptographic assets.sgx_updatesvn() Most of the time, there will
> 
> Is there something wrong here in the text? It looks malformed.

Yes, sorry, looks like copy-paste error I missed in the comment. 
Will fix. 

> 
> > + * be no update and that's OK.
> > + *
> > + * Return:
> > + * 0: Success, not supported or run out of entropy
> > + */
> > +static int sgx_update_svn(void)
> > +{
> > +	int ret;
> > +
> > +	/*
> > +	 * If EUPDATESVN is not available, it is ok to
> > +	 * silently skip it to comply with legacy behavior.
> > +	 */
> > +	if (!X86_FEATURE_SGX_EUPDATESVN)
> > +		return 0;
> > +
> > +	for (int i = 0; i < RDRAND_RETRY_LOOPS; i++) {
> > +		ret = __eupdatesvn();
> > +
> > +		/* Stop on success or unexpected errors: */
> > +		if (ret != SGX_INSUFFICIENT_ENTROPY)
> > +			break;
> > +	}
> > +
> > +	/*
> > +	 * SVN either was up-to-date or SVN update failed due
> > +	 * to lack of entropy. In both cases, we want to return
> > +	 * 0 in order not to break sgx_(vepc_)open. We dont expect
> > +	 * SGX_INSUFFICIENT_ENTROPY error unless underlying RDSEED
> > +	 * is under heavy pressure.
> > +	 */
> > +	if ((ret == SGX_NO_UPDATE) || (ret == SGX_INSUFFICIENT_ENTROPY))
> 
> 	if (ret == SGX_NO_UPDATE || ret == SGX_INSUFFICIENT_ENTROPY)

Ok, but I will have to change this anyhow since we seems to trend that we want
to return -EBUSY when SGX_INSUFFICIENT_ENTROPY and do not
proceed with open() call. 

> 
> > +		return 0;
> > +
> > +	if (!ret) {
> > +		/*
> > +		 * SVN successfully updated.
> > +		 * Let users know when the update was successful.
> > +		 */
> 
> This comment is like as useless as an inline comment can ever possibly
> be. Please, remove it.

It is actually not quite so useless because this is the rare case we know
the EUPDATESVN actually executed and hence the pr_info also below.
Without this, there will be no way for sysadmin to trace whenever CPU
SVN was upgraded or not (Sean mentioned that this is already pretty
opaque to users). 

> 
> > +		pr_info("SVN updated successfully\n");
> 
> Let's not add this either in the scope of this patch set.

See above. 

> 
> > +		return 0;
> > +	}
> 
> Since you parse error codes already, I don't understand why deal with
> the success case in the middle of doing that.
> 
> More consistent would be (not also the use of unlikely()):
> 
> 	if (ret == SGX_NO_UPDATE || ret == SGX_INSUFFICIENT_ENTROPY)
> 		return 0;
> 
> 	/*
> 	 * EUPDATESVN was called when EPC is empty, all other error
> 	 * codes are unexpected.
> 	 */
> 	if (unlikely(ret)) {
> 		ENCLS_WARN(ret, "EUPDATESVN");
> 		return ret;
> 	}
> 
> 	return 0;
> }
> 
> This is how I would rewrite the tail of this function.

I think everyone already re-wrote this function at least once and no one is
happy with the version from previous person )) 
Let me try another version again, taking into account changes in return codes
discussed in this thread also. 

Best Regards,
Elena.

RE: [PATCH v5 4/5] x86/sgx: Implement ENCLS[EUPDATESVN]

[Reshetova, Elena]

 > >
> > > Why not just fail sgx_open() (e.g., return -EBUSY, or -EAGAIN) in that case?
> > > Userspace can then retry?
> >
> > The idea on the patch was that such a scenario where we run out of entropy
> > should not happen in real life unless RDSEED is under stress (in case we
> > accidentally collided, we do a 10 time retry). So, in this case we keep the
> legacy
> > behaviour, i.e. proceeding without EUPDATESVN. But I can change to the
> above
> > logic to return -EAGAIN in this case if everyone thinks it is a better
> approach.
> 
> Well I think I am seeing conflicting message:
> 
> You mentioned in v4 that some simple (userspace!) tests can make
> EUPDATESVN fail
> "reliably and quite easily even with 10 time retry loop by kernel".  This seems
> to me that "RDSEED is under stress" can certainly happen in in real life.
> 
> Or are you suggesting that kinda "simple tests" cannot happen in real life?

Yes, only under explicit attack. 

> 
> Even we agree that such test cannot happen in real life, since updating SVN is
> about security, I think it's quite fair that we need to consider that the
> platform is under attack.
> 
> Allowing enclave to be created when EUPDATESVN fails due to running out of
> entropy is a clear violation of security to me.  And what's even worse is
> AFAICT
> userspace is not notified about this by any means.

There is no security issues since you can always see the CPU SVN via the
remote attestation process of the given enclave, so you will know for sure
what microcode you run with. And we have this tiny helper in the form of
pr_info() to indicate when kernel finally managed to do the update successfully.
But overall I agree, l will change the logic to return - EAGAIN on entropy issues,
and smth like -EIO on other non-expected errors. 

Best Regards,
Elena.

RE: [PATCH v5 5/5] x86/sgx: Enable automatic SVN updates for SGX enclaves

[Reshetova, Elena]

> > Yes, meaning is different, see above.
> 
> So that's rather convoluted:
> 
> 	atomic64_inc_not_zero():   returns 1 on successful increase, 0 on
> failure
>         sgx_inc_usage_count():     returns 0 on successful increase, 1 on failure
>         sgx_open():                returns 0 on successful increase, -EBUSY on failure
> 
> Could we at least standardize sgx_inc_usage_count() on -EBUSY in the
> failure case, so it's a more obvious pattern:
> 
> +       ret = sgx_inc_usage_count();
> +       if (ret < 0)
> +               return ret;
> 

Yes, will rewrite accordingly. Especially since I have to return two different
error codes into sgx_open() now to indicate different nature of issues with
running EUDPATESVN: temporal failure due to lack of entropy (-EAGAIN)
and potentially persistent problem when getting unexpected error codes
(-EIO).

Best Regards,
Elena.

RE: [PATCH v5 5/5] x86/sgx: Enable automatic SVN updates for SGX enclaves

[Reshetova, Elena]

> 
> On Mon, May 19, 2025 at 10:24:31AM +0300, Elena Reshetova wrote:
> > 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 enables
> > 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. CPUSVN is only automatically updated at
> reboot.
> > This means that, although the microcode has been updated, enclaves can
> never
> > attest to this fact. Enclaves are stuck attesting to the old version until
> > a reboot.
> >
> > The SGX architecture has an alternative to these reboots: the
> ENCLS[EUPDATESVN]
> > instruction [1]. It allows another snapshot to be taken to update CPUSVN
> > after a runtime microcode update 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.
> >
> > Attempt to execute EUPDATESVN on the first open of sgx_(vepc)open().
> > If EUPDATESVN fails with any other error code than
> SGX_INSUFFICIENT_ENTROPY,
> > this is considered unexpected and the open() returns an error. This
> > should not happen in practice. On contrary SGX_INSUFFICIENT_ENTROPY
> might
> > happen due to a pressure on the system DRNG (RDSEED) and therefore
> > the open() is not blocked to allow normal enclave operation.
> >
> > [1] Runtime Microcode Updates with Intel Software Guard Extensions,
> > https://cdrdv2.intel.com/v1/dl/getContent/648682
> 
> I'd hope, despite having the wealth of information in it, this commit
> message would a go round or few of editing, and nail the gist of this
> commit better. Then it would be easier in future review the choices
> made.

I will try to trim the background text more.

Best Regards,
Elena.

Re: [PATCH v5 5/5] x86/sgx: Enable automatic SVN updates for SGX enclaves

[Ingo Molnar]

* Reshetova, Elena <elena.reshetova@intel.com> wrote:

> > > Yes, meaning is different, see above.
> > 
> > So that's rather convoluted:
> > 
> > 	atomic64_inc_not_zero():   returns 1 on successful increase, 0 on
> > failure
> >         sgx_inc_usage_count():     returns 0 on successful increase, 1 on failure
> >         sgx_open():                returns 0 on successful increase, -EBUSY on failure
> > 
> > Could we at least standardize sgx_inc_usage_count() on -EBUSY in the
> > failure case, so it's a more obvious pattern:
> > 
> > +       ret = sgx_inc_usage_count();
> > +       if (ret < 0)
> > +               return ret;
> > 
> 
> Yes, will rewrite accordingly. Especially since I have to return two different
> error codes into sgx_open() now to indicate different nature of issues with
> running EUDPATESVN: temporal failure due to lack of entropy (-EAGAIN)
> and potentially persistent problem when getting unexpected error codes
> (-EIO).

Makes sense!

Thanks,

	Ingo

Re: [PATCH v5 4/5] x86/sgx: Implement ENCLS[EUPDATESVN]

[Huang, Kai]

On Tue, 2025-05-20 at 06:36 +0000, Reshetova, Elena wrote:
>  > >
> > > > Why not just fail sgx_open() (e.g., return -EBUSY, or -EAGAIN) in that case?
> > > > Userspace can then retry?
> > > 
> > > The idea on the patch was that such a scenario where we run out of entropy
> > > should not happen in real life unless RDSEED is under stress (in case we
> > > accidentally collided, we do a 10 time retry). So, in this case we keep the
> > legacy
> > > behaviour, i.e. proceeding without EUPDATESVN. But I can change to the
> > above
> > > logic to return -EAGAIN in this case if everyone thinks it is a better
> > approach.
> > 
> > Well I think I am seeing conflicting message:
> > 
> > You mentioned in v4 that some simple (userspace!) tests can make
> > EUPDATESVN fail
> > "reliably and quite easily even with 10 time retry loop by kernel".  This seems
> > to me that "RDSEED is under stress" can certainly happen in in real life.
> > 
> > Or are you suggesting that kinda "simple tests" cannot happen in real life?
> 
> Yes, only under explicit attack. 
> 
> > 
> > Even we agree that such test cannot happen in real life, since updating SVN is
> > about security, I think it's quite fair that we need to consider that the
> > platform is under attack.
> > 
> > Allowing enclave to be created when EUPDATESVN fails due to running out of
> > entropy is a clear violation of security to me.  And what's even worse is
> > AFAICT
> > userspace is not notified about this by any means.
> 
> There is no security issues since you can always see the CPU SVN via the
> remote attestation process of the given enclave, so you will know for sure
> what microcode you run with. 

Remote attestation can certainly tell the challenger the enclave is still
running on a compromised platform, but I wouldn't say there is *no* security
issues.

E.g., the "crypto assets" that the EUPDATESVN fails to re-generate might have
already been leaked on the compromised platform.  If we allow enclave to run,
the attacker may have chance to steal secrets that aren't remotely provisioned.

You may argue the enclave shouldn't have any secrets before it's verified, but I
think in real world it may not always be the case.

Re: [PATCH v5 1/5] x86/sgx: Introduce a counter to count the sgx_(vepc_)open()

[Jarkko Sakkinen]

On Tue, May 20, 2025 at 06:25:04AM +0000, Reshetova, Elena wrote:
> > Maybe just use raw atomic_inc() and atomic_dec() at the sites?
> > 
> > IMHO, it makes only sense to wrap, when it makes sense to wrap.
> 
> You mean for this patch or overall? For overall we discussed in v4
> why we would like to raise it to atomic64.
> Or do I misunderstand your comment?

So I was thinking whether we want wrappers but I guess we can keep
them.

> 
> Best Regards,
> Elena.

BR, Jarkko

Re: [PATCH v5 4/5] x86/sgx: Implement ENCLS[EUPDATESVN]

[Jarkko Sakkinen]

On Tue, May 20, 2025 at 06:31:46AM +0000, Reshetova, Elena wrote:

BTW, please keep the line which tells who responded.

>  > +/**
> > > + * sgx_updatesvn() - Attempt to call ENCLS[EUPDATESVN]
> > > + * If EPC is empty, this instruction attempts to update CPUSVN to the
> > > + * currently loaded microcode update SVN and generate new
> > > + * cryptographic assets.sgx_updatesvn() Most of the time, there will
> > 
> > Is there something wrong here in the text? It looks malformed.
> 
> Yes, sorry, looks like copy-paste error I missed in the comment. 
> Will fix. 
> 
> > 
> > > + * be no update and that's OK.
> > > + *
> > > + * Return:
> > > + * 0: Success, not supported or run out of entropy
> > > + */
> > > +static int sgx_update_svn(void)
> > > +{
> > > +	int ret;
> > > +
> > > +	/*
> > > +	 * If EUPDATESVN is not available, it is ok to
> > > +	 * silently skip it to comply with legacy behavior.
> > > +	 */
> > > +	if (!X86_FEATURE_SGX_EUPDATESVN)
> > > +		return 0;
> > > +
> > > +	for (int i = 0; i < RDRAND_RETRY_LOOPS; i++) {
> > > +		ret = __eupdatesvn();
> > > +
> > > +		/* Stop on success or unexpected errors: */
> > > +		if (ret != SGX_INSUFFICIENT_ENTROPY)
> > > +			break;
> > > +	}
> > > +
> > > +	/*
> > > +	 * SVN either was up-to-date or SVN update failed due
> > > +	 * to lack of entropy. In both cases, we want to return
> > > +	 * 0 in order not to break sgx_(vepc_)open. We dont expect
> > > +	 * SGX_INSUFFICIENT_ENTROPY error unless underlying RDSEED
> > > +	 * is under heavy pressure.
> > > +	 */
> > > +	if ((ret == SGX_NO_UPDATE) || (ret == SGX_INSUFFICIENT_ENTROPY))
> > 
> > 	if (ret == SGX_NO_UPDATE || ret == SGX_INSUFFICIENT_ENTROPY)
> 
> Ok, but I will have to change this anyhow since we seems to trend that we want
> to return -EBUSY when SGX_INSUFFICIENT_ENTROPY and do not
> proceed with open() call. 
> 
> > 
> > > +		return 0;
> > > +
> > > +	if (!ret) {
> > > +		/*
> > > +		 * SVN successfully updated.
> > > +		 * Let users know when the update was successful.
> > > +		 */
> > 
> > This comment is like as useless as an inline comment can ever possibly
> > be. Please, remove it.
> 
> It is actually not quite so useless because this is the rare case we know
> the EUPDATESVN actually executed and hence the pr_info also below.
> Without this, there will be no way for sysadmin to trace whenever CPU
> SVN was upgraded or not (Sean mentioned that this is already pretty
> opaque to users). 
> 
> > 
> > > +		pr_info("SVN updated successfully\n");
> > 
> > Let's not add this either in the scope of this patch set.
> 
> See above. 
> 
> > 
> > > +		return 0;
> > > +	}
> > 
> > Since you parse error codes already, I don't understand why deal with
> > the success case in the middle of doing that.
> > 
> > More consistent would be (not also the use of unlikely()):
> > 
> > 	if (ret == SGX_NO_UPDATE || ret == SGX_INSUFFICIENT_ENTROPY)
> > 		return 0;
> > 
> > 	/*
> > 	 * EUPDATESVN was called when EPC is empty, all other error
> > 	 * codes are unexpected.
> > 	 */
> > 	if (unlikely(ret)) {
> > 		ENCLS_WARN(ret, "EUPDATESVN");
> > 		return ret;
> > 	}
> > 
> > 	return 0;
> > }
> > 
> > This is how I would rewrite the tail of this function.
> 
> I think everyone already re-wrote this function at least once and no one is
> happy with the version from previous person )) 
> Let me try another version again, taking into account changes in return codes
> discussed in this thread also. 

unlikely() is both (minor) optimization and documents that it is not expected
branch so it obviously makes sense here.

> 
> Best Regards,
> Elena.

BR, Jarkko

Re: [PATCH v5 4/5] x86/sgx: Implement ENCLS[EUPDATESVN]

[Dave Hansen]

On 5/20/25 12:57, Jarkko Sakkinen wrote:
> unlikely() is both (minor) optimization and documents that it is not expected
> branch so it obviously makes sense here.

I'm not a big fan of unlikely() being thrown in without specific
reasons. I don't think we need an annotation to tell us that the path
that spits out a warning is not the common path. I'm also pretty sure
the compiler doesn't need any help.

In this case, it is pure noise.