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

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

[Elena Reshetova]

Changes since v10 following reviews by Dave:

 - merge patch 1 and 2
 - patch 1: clarify the comment about the function prototype
 - patch 3: clarify the description of SGX_NO_UPDATE
  error code, move the definition of EUPDATESVN enum leaf
  to patch 4
 - patch 4: clarify commit, adjust sgx_update_svn() function
  according to feedback

Changes since v9 following reviews by Kai:

 - postpone the definition of sgx_inc_usage_count
   until patch 6
 - clarify the commit message in patch 6
 - minor fixes

Note: I didn't merge patch 1 and 2 since it goes against
previous suggestion made by Jarkko.

Changes since v8 following reviews by Dave and Jarkko:

 - fix the sgx_inc/dec_usage_count() to not do any
 actual counting until the later patch when adequate
 mutex is introduced as suggested by Dave
 - add an additional patch (patch 1) to redefine
 existing sgx_(vepc_)open into wrappers to allow
 the follow up patch to introduce the sgx_inc/dec_usage_count()
 functions into sgx_(vepc_)open cleanly as suggested
 by Jarkko. 

Changes since v7 following reviews by Dave:

 - change sgx_usage_count to be a normal int type
 and greatly simplify the sgx_inc_usage_count func.
 This results in requiring a mutex for each sgx_(vepc_)open
 but given that the mutex is held a short amount of
 time it should be ok perf-wise.

Changes since v6 following reviews by Kai,Jarkko
and Dave:

 - fix sgx_update_svn function description
 - add maybe_unused for sgx_update_svn in patch 4
 to silence the warning, remove it in patch 5
 - add note to patch 1 explaining why the prototype
 sgx_inc_usage_count returns int and not void
 - fix the order of return code checks in 
 sgx_update_svn
 - cosmetic fixes
 
Note: I didn't change the sgx_inc/dec_usage_count
to statics because they are called from a number of
different code locations and also rely on a static
sgx_usage_count, which lives naturally in main.c.  
 
Changes since v5 following reviews by Ingo, Kai,
Jarkko and Dave:

 - rebase on x86 tip
 - patch 1 is fixed to do correct unwinding in
 case of errors
 - patch 2: add feature flag to cpuid-deps.c
 - patch 3: remove unused SGX_EPC_NOT_READY error code
 - patch 4: fix x86 feature check, return -EAGAIN
 on SGX_INSUFFICIENT_ENTROPY and -EIO on other non-
 expected errors. Comments/style are also fixed.
 - patch 5: rewrite commit message, add comments inline

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.16.0_rc7 & sgx selftests.
Also tested on a Kaby Lake machine without EUPDATESVN support.
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?explicitVersion=true
[2] https://lore.kernel.org/all/20220520103904.1216-1-cathy.zhang@intel.com/

Elena Reshetova (5):
  x86/sgx: Introduce functions 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               | 37 ++++++----
 arch/x86/kernel/cpu/cpuid-deps.c         |  1 +
 arch/x86/kernel/cpu/scattered.c          |  1 +
 arch/x86/kernel/cpu/sgx/driver.c         | 19 ++++-
 arch/x86/kernel/cpu/sgx/encl.c           |  1 +
 arch/x86/kernel/cpu/sgx/encls.h          |  5 ++
 arch/x86/kernel/cpu/sgx/main.c           | 91 ++++++++++++++++++++++++
 arch/x86/kernel/cpu/sgx/sgx.h            |  3 +
 arch/x86/kernel/cpu/sgx/virt.c           | 20 +++++-
 tools/arch/x86/include/asm/cpufeatures.h |  1 +
 11 files changed, 163 insertions(+), 17 deletions(-)

-- 
2.45.2

[PATCH v11 1/5] x86/sgx: Introduce functions 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. Define placeholder
functions sgx_inc/dec_usage_count() that are used to increment
and decrement such a counter. Also, wire the call sites for
these functions. For the latter, in order to introduce the
counting of active sgx users on top of clean functions that
allocate vepc structures, covert existing sgx_(vepc_)open() to
__sgx_(vepc_)open().

The definition of the counter itself and the actual implementation
of these two functions comes next. The counter will be used by
the driver that would be attempting to call EUPDATESVN SGX instruction
only when incrementing from zero.

Note: the sgx_inc_usage_count() prototype is defined to return
int for the cleanliness of the follow-up patches despite always
returning zero in this patch. When the EUPDATESVN SGX instruction
will be enabled in the follow-up patch, the sgx_inc_usage_count()
will start to return the actual return code.

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

diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c
index 7f8d1e11dbee..79d6020dfe9c 100644
--- a/arch/x86/kernel/cpu/sgx/driver.c
+++ b/arch/x86/kernel/cpu/sgx/driver.c
@@ -14,7 +14,7 @@ u64 sgx_attributes_reserved_mask;
 u64 sgx_xfrm_reserved_mask = ~0x3;
 u32 sgx_misc_reserved_mask;
 
-static int sgx_open(struct inode *inode, struct file *file)
+static int __sgx_open(struct inode *inode, struct file *file)
 {
 	struct sgx_encl *encl;
 	int ret;
@@ -41,6 +41,23 @@ static int sgx_open(struct inode *inode, struct file *file)
 	return 0;
 }
 
+static int sgx_open(struct inode *inode, struct file *file)
+{
+	int ret;
+
+	ret = sgx_inc_usage_count();
+	if (ret)
+		return ret;
+
+	ret = __sgx_open(inode, file);
+	if (ret) {
+		sgx_dec_usage_count();
+		return ret;
+	}
+
+	return 0;
+}
+
 static int sgx_release(struct inode *inode, struct file *file)
 {
 	struct sgx_encl *encl = file->private_data;
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index 308dbbae6c6e..cf149b9f4916 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 2de01b379aa3..3a5cbd1c170e 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -917,6 +917,16 @@ int sgx_set_attribute(unsigned long *allowed_attributes,
 }
 EXPORT_SYMBOL_GPL(sgx_set_attribute);
 
+int sgx_inc_usage_count(void)
+{
+	return 0;
+}
+
+void sgx_dec_usage_count(void)
+{
+	return;
+}
+
 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..b649c0610019 100644
--- a/arch/x86/kernel/cpu/sgx/virt.c
+++ b/arch/x86/kernel/cpu/sgx/virt.c
@@ -255,10 +255,11 @@ static int sgx_vepc_release(struct inode *inode, struct file *file)
 	xa_destroy(&vepc->page_array);
 	kfree(vepc);
 
+	sgx_dec_usage_count();
 	return 0;
 }
 
-static int sgx_vepc_open(struct inode *inode, struct file *file)
+static int __sgx_vepc_open(struct inode *inode, struct file *file)
 {
 	struct sgx_vepc *vepc;
 
@@ -273,6 +274,23 @@ static int sgx_vepc_open(struct inode *inode, struct file *file)
 	return 0;
 }
 
+static int sgx_vepc_open(struct inode *inode, struct file *file)
+{
+	int ret;
+
+	ret = sgx_inc_usage_count();
+	if (ret)
+		return ret;
+
+	ret =  __sgx_vepc_open(inode, file);
+	if (ret) {
+		sgx_dec_usage_count();
+		return ret;
+	}
+
+	return 0;
+}
+
 static long sgx_vepc_ioctl(struct file *file,
 			   unsigned int cmd, unsigned long arg)
 {
-- 
2.45.2

[PATCH v11 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.

Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>
Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
---
 arch/x86/include/asm/cpufeatures.h       | 1 +
 arch/x86/kernel/cpu/cpuid-deps.c         | 1 +
 arch/x86/kernel/cpu/scattered.c          | 1 +
 tools/arch/x86/include/asm/cpufeatures.h | 1 +
 4 files changed, 4 insertions(+)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 602957dd2609..830d24ff1ada 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -494,6 +494,7 @@
 #define X86_FEATURE_TSA_SQ_NO		(21*32+11) /* AMD CPU not vulnerable to TSA-SQ */
 #define X86_FEATURE_TSA_L1_NO		(21*32+12) /* AMD CPU not vulnerable to TSA-L1 */
 #define X86_FEATURE_CLEAR_CPU_BUF_VM	(21*32+13) /* Clear CPU buffers using VERW before VMRUN */
+#define X86_FEATURE_SGX_EUPDATESVN	(21*32+14) /* Support for ENCLS[EUPDATESVN] instruction */
 
 /*
  * BUG word(s)
diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
index 46efcbd6afa4..3d9f49ad0efd 100644
--- a/arch/x86/kernel/cpu/cpuid-deps.c
+++ b/arch/x86/kernel/cpu/cpuid-deps.c
@@ -79,6 +79,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   },
diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
index b4a1f6732a3a..d13444d11ba0 100644
--- a/arch/x86/kernel/cpu/scattered.c
+++ b/arch/x86/kernel/cpu/scattered.c
@@ -42,6 +42,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 ee176236c2be..78c3894c17c1 100644
--- a/tools/arch/x86/include/asm/cpufeatures.h
+++ b/tools/arch/x86/include/asm/cpufeatures.h
@@ -487,6 +487,7 @@
 #define X86_FEATURE_PREFER_YMM		(21*32+ 8) /* Avoid ZMM registers due to downclocking */
 #define X86_FEATURE_APX			(21*32+ 9) /* Advanced Performance Extensions */
 #define X86_FEATURE_INDIRECT_THUNK_ITS	(21*32+10) /* Use thunk for indirect branches in lower half of cacheline */
+#define X86_FEATURE_SGX_EUPDATESVN	(21*32+14) /* Support for ENCLS[EUPDATESVN] instruction */
 
 /*
  * BUG word(s)
-- 
2.45.2

[PATCH v11 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 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
index 6a0069761508..2da5b3b117a1 100644
--- a/arch/x86/include/asm/sgx.h
+++ b/arch/x86/include/asm/sgx.h
@@ -73,6 +73,10 @@ 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_NO_UPDATE:		EUPDATESVN could not update the CPUSVN because the
+ *				current SVN was not newer than CPUSVN. This is the most
+ *				common error code returned by EUPDATESVN.
  * %SGX_UNMASKED_EVENT:		An unmasked event, e.g. INTR, was received
  */
 enum sgx_return_code {
@@ -81,6 +85,8 @@ enum sgx_return_code {
 	SGX_CHILD_PRESENT		= 13,
 	SGX_INVALID_EINITTOKEN		= 16,
 	SGX_PAGE_NOT_MODIFIABLE		= 20,
+	SGX_INSUFFICIENT_ENTROPY	= 29,
+	SGX_NO_UPDATE				= 31,
 	SGX_UNMASKED_EVENT		= 128,
 };
 
-- 
2.45.2

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

[Elena Reshetova]

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.

Add the method to perform ENCLS[EUPDATESVN]. However, until the
follow up patch that wires calling sgx_update_svn() from
sgx_inc_usage_count(), this code is not reachable.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
---
 arch/x86/include/asm/sgx.h      | 31 +++++++-------
 arch/x86/kernel/cpu/sgx/encls.h |  5 +++
 arch/x86/kernel/cpu/sgx/main.c  | 73 +++++++++++++++++++++++++++++++++
 3 files changed, 94 insertions(+), 15 deletions(-)

diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
index 2da5b3b117a1..0e13678f9cbd 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,
 };
 
 /**
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 3a5cbd1c170e..d8c42524b590 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -16,6 +16,7 @@
 #include <linux/vmalloc.h>
 #include <asm/msr.h>
 #include <asm/sgx.h>
+#include <asm/archrandom.h>
 #include "driver.h"
 #include "encl.h"
 #include "encls.h"
@@ -917,6 +918,78 @@ int sgx_set_attribute(unsigned long *allowed_attributes,
 }
 EXPORT_SYMBOL_GPL(sgx_set_attribute);
 
+/* Counter to count the active SGX users */
+static int sgx_usage_count;
+
+/**
+ * sgx_update_svn() - Attempt to call ENCLS[EUPDATESVN].
+ * This instruction attempts to update CPUSVN to the
+ * currently loaded microcode update SVN and generate new
+ * cryptographic assets.
+ * Return:
+ * 0: Success or not supported
+ * -EAGAIN: Can be safely retried, failure is due to lack of
+ *  entropy in RNG.
+ * -EIO: Unexpected error, retries are not advisable.
+ */
+static int __maybe_unused sgx_update_svn(void)
+{
+	int ret;
+
+	/*
+	 * If EUPDATESVN is not available, it is ok to
+	 * silently skip it to comply with legacy behavior.
+	 */
+	if (!cpu_feature_enabled(X86_FEATURE_SGX_EUPDATESVN))
+		return 0;
+
+	/*
+	 * EPC is guaranteed to be empty when there are no users.
+	 * Ensure we are on our first user before proceeding further.
+	 */
+	WARN(sgx_usage_count != 1, "Elevated usage count when calling EUPDATESVN\n");
+
+	for (int i = 0; i < RDRAND_RETRY_LOOPS; i++) {
+		ret = __eupdatesvn();
+
+		/* Stop on success or unexpected errors: */
+		if (ret != SGX_INSUFFICIENT_ENTROPY)
+			break;
+	}
+
+	switch (ret) {
+	/*
+	 * SVN successfully updated.
+	 * Let users know when the update was successful.
+	 */
+	case 0:
+		pr_info("SVN updated successfully\n");
+		return 0;
+	/*
+	 * SVN update failed since the current SVN is
+	 * not newer than CPUSVN. This is the most
+	 * common case and indicates no harm.
+	 */
+	case SGX_NO_UPDATE:
+		return 0;
+	/*
+	 * SVN update failed due to lack of entropy in DRNG.
+	 * Indicate to userspace that it should retry.
+	 */
+	case SGX_INSUFFICIENT_ENTROPY:
+		return -EAGAIN;
+	default:
+		break;
+	}
+
+	/*
+	 * EUPDATESVN was called when EPC is empty, all other error
+	 * codes are unexpected.
+	 */
+	ENCLS_WARN(ret, "EUPDATESVN");
+	return -EIO;
+}
+
 int sgx_inc_usage_count(void)
 {
 	return 0;
-- 
2.45.2

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

[Elena Reshetova]

== Background ==

ENCLS[EUPDATESVN] is a new SGX instruction [1] which allows enclave
attestation to include information about updated microcode SVN without a
reboot. Before an EUPDATESVN operation can be successful, all SGX memory
(aka. EPC) must be marked as “unused” in the SGX hardware metadata
(aka.EPCM). This requirement ensures that no compromised enclave can
survive the EUPDATESVN procedure and provides an opportunity to generate
new cryptographic assets.

== Solution ==

Attempt to execute ENCLS[EUPDATESVN] every time the first file descriptor
is obtained via sgx_(vepc_)open(). In the most common case the microcode
SVN is already up-to-date, and the operation succeeds without updating SVN.
Note: while in such cases the underlying CR_BASE_KEY is regenrated, it does
not affect enclaves' visible keys obtained via EGETKEY instruction.

If it 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's DRNG (RDSEED) and therefore the *open() can
be safely retried 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/main.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index d8c42524b590..b6f024802026 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -932,7 +932,7 @@ static int sgx_usage_count;
  *  entropy in RNG.
  * -EIO: Unexpected error, retries are not advisable.
  */
-static int __maybe_unused sgx_update_svn(void)
+static int sgx_update_svn(void)
 {
 	int ret;
 
@@ -990,14 +990,22 @@ static int __maybe_unused sgx_update_svn(void)
 	return -EIO;
 }
 
+/* Mutex to ensure no concurrent EPC accesses during EUPDATESVN */
+static DEFINE_MUTEX(sgx_svn_lock);
+
 int sgx_inc_usage_count(void)
 {
+	guard(mutex)(&sgx_svn_lock);
+
+	if (sgx_usage_count++ == 0)
+		return sgx_update_svn();
+
 	return 0;
 }
 
 void sgx_dec_usage_count(void)
 {
-	return;
+	sgx_usage_count--;
 }
 
 static int __init sgx_init(void)
-- 
2.45.2

Re: [PATCH v11 1/5] x86/sgx: Introduce functions to count the sgx_(vepc_)open()

[Huang, Kai]

(sorry for back-and-forth and not saying those before, but since I found
some issues in this version so I feel I should still point out.)

On Wed, 2025-08-06 at 11:11 +0300, Elena Reshetova wrote:
> Currently SGX does not have a global counter to count the
> active users from userspace or hypervisor. 

First, the text wrap of this is not consistent with other lines.  It
breaks too aggressively AFAICT.  I personally set textwidth=72, but I've
seen other people suggesting to wrap at 75 characters.  But this looks too
aggressive and not consistent with  other lines.

I also observed this problem in other patches too.  Could you check all
changelogs and re-wrap the text if needed? 

Back to technical:

"virtual EPC" is also opened from the userspace, so using "hypervisor"
doesn't look quite right to me.

Also, it would be better to mention the "why" first, so people don't need
to find out the reason after reading these "how"s.

How about:

Currently, when SGX is compromised and the microcode update fix is
applied, the machine needs to be rebooted to invalidate old SGX crypto-
assets and make SGX be in an updated safe state.  It's not friendly for
the cloud.

To avoid having to reboot, a new ENCLS[EUPDATESVN] is introduced to update
SGX environment at runtime.  This process needs to be done when there's no
SGX user to make sure no compromised enclaves can survive from the update
and allow the system to regenerate crypto-assets etc.

For now there's no counter to track the active SGX users of host enclave
and virtual EPC.  Introduce such counter mechanism so that the EUPDATESVN
can be done only when there's no SGX users.

> Define placeholder
> functions sgx_inc/dec_usage_count() that are used to increment
> and decrement such a counter. Also, wire the call sites for
> these functions. 
> 

[...]

> For the latter, in order to introduce the
> counting of active sgx users on top of clean functions that
> allocate vepc structures
> 

It's not just "vepc structures" only, right?

Encapsulate the current sgx_(vepc_)open() to __sgx_(vepc_)open() to make
the new sgx_(vepc_)open() easy to read. 

> , covert existing sgx_(vepc_)open() to __sgx_(vepc_)open().
    ^
    convert ?

> 
> The definition of the counter itself and the actual implementation
> of these two functions comes next. 
			 ^
			 come next ?

> The counter will be used by
> the driver that would be attempting to call EUPDATESVN SGX instruction
> only when incrementing from zero.

This can be removed if my paragraphs are used (which already mentioned
this).

> 
> Note: the sgx_inc_usage_count() prototype is defined to return
> int for the cleanliness of the follow-up patches despite always
> returning zero in this patch. When the EUPDATESVN SGX instruction
> will be enabled in the follow-up patch, the sgx_inc_usage_count()
			 ^
			 follow-up patches?

> will start to return the actual return code.

Could this paragraph be shorter, like below?

The EUPDATESVN, which may fail, will be done in sgx_inc_usage_count(). 
Make it return 'int' to make subsequent patches which implement EUPDATESVN
easier to review.  For now it always returns success.


[...]

> diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> index 308dbbae6c6e..cf149b9f4916 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();
>  }
>  
> 

[...]

> --- a/arch/x86/kernel/cpu/sgx/virt.c
> +++ b/arch/x86/kernel/cpu/sgx/virt.c
> @@ -255,10 +255,11 @@ static int sgx_vepc_release(struct inode *inode, struct file *file)
>  	xa_destroy(&vepc->page_array);
>  	kfree(vepc);
>  
> +	sgx_dec_usage_count();
>  	return 0;
>  }

Given we have __sgx_(vepc_)open(), I think it makes more sense to have
__sgx_(encl_|vepc_)release() counterpart?

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

[Huang, Kai]

On Wed, 2025-08-06 at 11:11 +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.

You may need to pay attention to text wrap, anyway ...
> 
> Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>

Reviewed-by: Kai Huang <kai.huang@intel.com>

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

[Huang, Kai]

On Wed, 2025-08-06 at 11:11 +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>
> ---
>  arch/x86/include/asm/sgx.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
> index 6a0069761508..2da5b3b117a1 100644
> --- a/arch/x86/include/asm/sgx.h
> +++ b/arch/x86/include/asm/sgx.h
> @@ -73,6 +73,10 @@ 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_NO_UPDATE:		EUPDATESVN could not update the CPUSVN because the
> + *				current SVN was not newer than CPUSVN. This is the most
> + *				common error code returned by EUPDATESVN.
>   * %SGX_UNMASKED_EVENT:		An unmasked event, e.g. INTR, was received
>   */
>  enum sgx_return_code {
> @@ -81,6 +85,8 @@ enum sgx_return_code {
>  	SGX_CHILD_PRESENT		= 13,
>  	SGX_INVALID_EINITTOKEN		= 16,
>  	SGX_PAGE_NOT_MODIFIABLE		= 20,
> +	SGX_INSUFFICIENT_ENTROPY	= 29,
> +	SGX_NO_UPDATE				= 31,

It seems the tap/writespace is broken, i.e., this error code is not
aligned with others.

>  	SGX_UNMASKED_EVENT		= 128,
>  };
>  

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

[Huang, Kai]

On Wed, 2025-08-06 at 11:11 +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.

I would also add some text to explain why other error codes are not
defined:

EUPDATESVN will be called when no active SGX user is guaranteed.  Only add
the error codes that can legally happen.  E.g., it could also fail due to
"SGX not ready" when there's SGX users but it wouldn't happen in this
implementation.

> 
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> ---
>  arch/x86/include/asm/sgx.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
> index 6a0069761508..2da5b3b117a1 100644
> --- a/arch/x86/include/asm/sgx.h
> +++ b/arch/x86/include/asm/sgx.h
> @@ -73,6 +73,10 @@ 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_NO_UPDATE:		EUPDATESVN could not update the CPUSVN because the
> + *				current SVN was not newer than CPUSVN. This is the most
> + *				common error code returned by EUPDATESVN.
>   * %SGX_UNMASKED_EVENT:		An unmasked event, e.g. INTR, was received
>   */
>  enum sgx_return_code {
> @@ -81,6 +85,8 @@ enum sgx_return_code {
>  	SGX_CHILD_PRESENT		= 13,
>  	SGX_INVALID_EINITTOKEN		= 16,
>  	SGX_PAGE_NOT_MODIFIABLE		= 20,
> +	SGX_INSUFFICIENT_ENTROPY	= 29,
> +	SGX_NO_UPDATE				= 31,
>  	SGX_UNMASKED_EVENT		= 128,
>  };
>  

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

[Huang, Kai]

On Wed, 2025-08-06 at 11:11 +0300, Elena Reshetova wrote:
> 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.
> 
> Add the method to perform ENCLS[EUPDATESVN]. However, until the
> follow up patch that wires calling sgx_update_svn() from
> sgx_inc_usage_count(), this code is not reachable.

Please check the text wrap.

> 
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> ---
>  arch/x86/include/asm/sgx.h      | 31 +++++++-------
>  arch/x86/kernel/cpu/sgx/encls.h |  5 +++
>  arch/x86/kernel/cpu/sgx/main.c  | 73 +++++++++++++++++++++++++++++++++
>  3 files changed, 94 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
> index 2da5b3b117a1..0e13678f9cbd 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,
>  };
>  
>  /**
> 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 3a5cbd1c170e..d8c42524b590 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -16,6 +16,7 @@
>  #include <linux/vmalloc.h>
>  #include <asm/msr.h>
>  #include <asm/sgx.h>
> +#include <asm/archrandom.h>
>  #include "driver.h"
>  #include "encl.h"
>  #include "encls.h"
> @@ -917,6 +918,78 @@ int sgx_set_attribute(unsigned long *allowed_attributes,
>  }
>  EXPORT_SYMBOL_GPL(sgx_set_attribute);
>  
> +/* Counter to count the active SGX users */
> +static int sgx_usage_count;
> +
> +/**
> + * sgx_update_svn() - Attempt to call ENCLS[EUPDATESVN].

Add a newline would make the comment more readable.

> + * This instruction attempts to update CPUSVN to the
> + * currently loaded microcode update SVN and generate new
> + * cryptographic assets.

Ditto here.

> + * Return:
> + * 0: Success or not supported
> + * -EAGAIN: Can be safely retried, failure is due to lack of
> + *  entropy in RNG.
> + * -EIO: Unexpected error, retries are not advisable.
> + */

You may want to make the description of the return code vertically
aligned.  And looking at the k-doc documentation, the format of the return
codes could be improved:

	* Return:
	* * %0:		- Success or not supported
	* * %-EAGAIN:	- ...
	* * %-EIO:	- ...

Please see "Return values" part of:

https://docs.kernel.org/doc-guide/kernel-doc.html

Or you can switch to use normal comment.  It's a static function anyway.
	

> +static int __maybe_unused sgx_update_svn(void)
> +{
> +	int ret;
> +
> +	/*
> +	 * If EUPDATESVN is not available, it is ok to
> +	 * silently skip it to comply with legacy behavior.
> +	 */
> +	if (!cpu_feature_enabled(X86_FEATURE_SGX_EUPDATESVN))
> +		return 0;
> +
> +	/*
> +	 * EPC is guaranteed to be empty when there are no users.
> +	 * Ensure we are on our first user before proceeding further.
> +	 */
> +	WARN(sgx_usage_count != 1, "Elevated usage count when calling EUPDATESVN\n");

I am not sure whether this is needed.  Wouldn't the ENCLS_WARN() at the
end catch this case and the user is able to figure out what went wrong
from the error code?

Besides that, in _this_ patch, what prevents sgx_usage_count from being
concurrently updated is still unknown.  It's kinda weird to just use it
here w/o seeing the actual mutex.

> +
> +	for (int i = 0; i < RDRAND_RETRY_LOOPS; i++) {
> +		ret = __eupdatesvn();
> +
> +		/* Stop on success or unexpected errors: */
> +		if (ret != SGX_INSUFFICIENT_ENTROPY)
> +			break;
> +	}
> +
> +	switch (ret) {
> +	/*
> +	 * SVN successfully updated.
> +	 * Let users know when the update was successful.
> +	 */

This is ugly.  I would just put the comment inside the 'case'.

> +	case 0:
> +		pr_info("SVN updated successfully\n");
> +		return 0;
> +	/*
> +	 * SVN update failed since the current SVN is
> +	 * not newer than CPUSVN. This is the most
> +	 * common case and indicates no harm.
> +	 */
> +	case SGX_NO_UPDATE:
> +		return 0;
> +	/*
> +	 * SVN update failed due to lack of entropy in DRNG.
> +	 * Indicate to userspace that it should retry.
> +	 */
> +	case SGX_INSUFFICIENT_ENTROPY:
> +		return -EAGAIN;
> +	default:
> +		break;
> +	}

Ditto for all the comments above.

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

[Huang, Kai]

On Wed, 2025-08-06 at 11:11 +0300, Elena Reshetova wrote:
> == Background ==
> 
> ENCLS[EUPDATESVN] is a new SGX instruction [1] which allows enclave
> attestation to include information about updated microcode SVN without a
> reboot. Before an EUPDATESVN operation can be successful, all SGX memory
> (aka. EPC) must be marked as “unused” in the SGX hardware metadata
> (aka.EPCM). This requirement ensures that no compromised enclave can
> survive the EUPDATESVN procedure and provides an opportunity to generate
> new cryptographic assets.
> 
> == Solution ==
> 
> Attempt to execute ENCLS[EUPDATESVN] every time the first file descriptor
> is obtained via sgx_(vepc_)open(). In the most common case the microcode
> SVN is already up-to-date, and the operation succeeds without updating SVN.

A newline here would be helpful.

> Note: while in such cases the underlying CR_BASE_KEY is regenrated, it does
		      ^ case, since it's just one case, right?

CR_BASE_KEY comes out of blue.  And the odd is the SDM actually uses
CR_BASE_PK AFAICT, so it could bring some confusion.

Perhaps just "crypto-assets" in general?

> not affect enclaves' visible keys obtained via EGETKEY instruction.
> 
> If it 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's DRNG (RDSEED) and therefore the *open() can
> be safely retried to allow normal enclave operation.

please check text wrap.

RE: [PATCH v11 1/5] x86/sgx: Introduce functions to count the sgx_(vepc_)open()

[Reshetova, Elena]

> -----Original Message-----
> From: Huang, Kai <kai.huang@intel.com>
> Sent: Thursday, August 7, 2025 2:39 AM
> To: Reshetova, Elena <elena.reshetova@intel.com>; Hansen, Dave
> <dave.hansen@intel.com>
> Cc: seanjc@google.com; mingo@kernel.org; Scarlata, Vincent R
> <vincent.r.scarlata@intel.com>; x86@kernel.org; jarkko@kernel.org;
> Annapurve, Vishal <vannapurve@google.com>; linux-kernel@vger.kernel.org;
> Mallick, Asit K <asit.k.mallick@intel.com>; Aktas, Erdem
> <erdemaktas@google.com>; Cai, Chong <chongc@google.com>; Bondarevska,
> Nataliia <bondarn@google.com>; linux-sgx@vger.kernel.org; Raynor, Scott
> <scott.raynor@intel.com>
> Subject: Re: [PATCH v11 1/5] x86/sgx: Introduce functions to count the
> sgx_(vepc_)open()
> 
> 
> (sorry for back-and-forth and not saying those before, but since I found
> some issues in this version so I feel I should still point out.)

Thank you Kai for your review! Later is better than never ))

> 
> On Wed, 2025-08-06 at 11:11 +0300, Elena Reshetova wrote:
> > Currently SGX does not have a global counter to count the
> > active users from userspace or hypervisor.
> 
> First, the text wrap of this is not consistent with other lines.  It
> breaks too aggressively AFAICT.  I personally set textwidth=72, but I've
> seen other people suggesting to wrap at 75 characters.  But this looks too
> aggressive and not consistent with  other lines.
> 
> I also observed this problem in other patches too.  Could you check all
> changelogs and re-wrap the text if needed?

Ok, I can do the 75 wrap, I think this is considered standard. 

> 
> Back to technical:
> 
> "virtual EPC" is also opened from the userspace, so using "hypervisor"
> doesn't look quite right to me.
> 
> Also, it would be better to mention the "why" first, so people don't need
> to find out the reason after reading these "how"s.
> 
> How about:
> 
> Currently, when SGX is compromised and the microcode update fix is
> applied, the machine needs to be rebooted to invalidate old SGX crypto-
> assets and make SGX be in an updated safe state.  It's not friendly for
> the cloud.
> 
> To avoid having to reboot, a new ENCLS[EUPDATESVN] is introduced to update
> SGX environment at runtime.  This process needs to be done when there's no
> SGX user to make sure no compromised enclaves can survive from the update
> and allow the system to regenerate crypto-assets etc.
> 
> For now there's no counter to track the active SGX users of host enclave
> and virtual EPC.  Introduce such counter mechanism so that the EUPDATESVN
> can be done only when there's no SGX users.


Thank you! I am fine with this description, will use it. 

> 
> > Define placeholder
> > functions sgx_inc/dec_usage_count() that are used to increment
> > and decrement such a counter. Also, wire the call sites for
> > these functions.
> >
> 
> [...]
> 
> > For the latter, in order to introduce the
> > counting of active sgx users on top of clean functions that
> > allocate vepc structures
> >
> 
> It's not just "vepc structures" only, right?
> 
> Encapsulate the current sgx_(vepc_)open() to __sgx_(vepc_)open() to make
> the new sgx_(vepc_)open() easy to read.

Sure, will change to this wording. 

> 
> > , covert existing sgx_(vepc_)open() to __sgx_(vepc_)open().
>     ^
>     convert ?
> 
> >
> > The definition of the counter itself and the actual implementation
> > of these two functions comes next.
> 			 ^
> 			 come next ?

Yes, will fix.

> 
> > The counter will be used by
> > the driver that would be attempting to call EUPDATESVN SGX instruction
> > only when incrementing from zero.
> 
> This can be removed if my paragraphs are used (which already mentioned
> this).

Agree. 

> 
> >
> > Note: the sgx_inc_usage_count() prototype is defined to return
> > int for the cleanliness of the follow-up patches despite always
> > returning zero in this patch. When the EUPDATESVN SGX instruction
> > will be enabled in the follow-up patch, the sgx_inc_usage_count()
> 			 ^
> 			 follow-up patches?
> 
> > will start to return the actual return code.
> 
> Could this paragraph be shorter, like below?
> 
> The EUPDATESVN, which may fail, will be done in sgx_inc_usage_count().
> Make it return 'int' to make subsequent patches which implement
> EUPDATESVN
> easier to review.  For now it always returns success.

Sure, will change. 

> 
> 
> [...]
> 
> > diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> > index 308dbbae6c6e..cf149b9f4916 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();
> >  }
> >
> >
> 
> [...]
> 
> > --- a/arch/x86/kernel/cpu/sgx/virt.c
> > +++ b/arch/x86/kernel/cpu/sgx/virt.c
> > @@ -255,10 +255,11 @@ static int sgx_vepc_release(struct inode *inode,
> struct file *file)
> >  	xa_destroy(&vepc->page_array);
> >  	kfree(vepc);
> >
> > +	sgx_dec_usage_count();
> >  	return 0;
> >  }
> 
> Given we have __sgx_(vepc_)open(), I think it makes more sense to have
> __sgx_(encl_|vepc_)release() counterpart?

Is it worth it? In case of *_open() variants there are quite error handling
under different cases, but for release as you can see it is just a one-line
addition. Not sure it is worth adding the wrappers just for that. 
But I can change it if people think it would look better this way. 

Best Regards,
Elena.

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

[Reshetova, Elena]

> -----Original Message-----
> From: Huang, Kai <kai.huang@intel.com>
> Sent: Thursday, August 7, 2025 2:40 AM
> To: Reshetova, Elena <elena.reshetova@intel.com>; Hansen, Dave
> <dave.hansen@intel.com>
> Cc: seanjc@google.com; mingo@kernel.org; Scarlata, Vincent R
> <vincent.r.scarlata@intel.com>; x86@kernel.org; jarkko@kernel.org;
> Annapurve, Vishal <vannapurve@google.com>; linux-kernel@vger.kernel.org;
> Mallick, Asit K <asit.k.mallick@intel.com>; Aktas, Erdem
> <erdemaktas@google.com>; Cai, Chong <chongc@google.com>; Bondarevska,
> Nataliia <bondarn@google.com>; linux-sgx@vger.kernel.org;
> dave.hansen@linux.intel.com; Raynor, Scott <scott.raynor@intel.com>
> Subject: Re: [PATCH v11 2/5] x86/cpufeatures: Add
> X86_FEATURE_SGX_EUPDATESVN feature flag
> 
> On Wed, 2025-08-06 at 11:11 +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.
> 
> You may need to pay attention to text wrap, anyway ...

Yes, will fix in all patches next. 

> >
> > Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>
> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> 
> Reviewed-by: Kai Huang <kai.huang@intel.com>
> 

Thank you!

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

[Reshetova, Elena]

> -----Original Message-----
> From: Huang, Kai <kai.huang@intel.com>
> Sent: Thursday, August 7, 2025 2:50 AM
> To: Reshetova, Elena <elena.reshetova@intel.com>; Hansen, Dave
> <dave.hansen@intel.com>
> Cc: seanjc@google.com; mingo@kernel.org; Scarlata, Vincent R
> <vincent.r.scarlata@intel.com>; x86@kernel.org; jarkko@kernel.org;
> Annapurve, Vishal <vannapurve@google.com>; linux-kernel@vger.kernel.org;
> Mallick, Asit K <asit.k.mallick@intel.com>; Aktas, Erdem
> <erdemaktas@google.com>; Cai, Chong <chongc@google.com>; Bondarevska,
> Nataliia <bondarn@google.com>; linux-sgx@vger.kernel.org; Raynor, Scott
> <scott.raynor@intel.com>
> Subject: Re: [PATCH v11 3/5] x86/sgx: Define error codes for use by
> ENCLS[EUPDATESVN]
> 
> On Wed, 2025-08-06 at 11:11 +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.
> 
> I would also add some text to explain why other error codes are not
> defined:
> 
> EUPDATESVN will be called when no active SGX user is guaranteed.  Only add
> the error codes that can legally happen.  E.g., it could also fail due to
> "SGX not ready" when there's SGX users but it wouldn't happen in this
> implementation.

Sure, will add in the next iteration, thanks!

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

[Reshetova, Elena]

> -----Original Message-----
> From: Huang, Kai <kai.huang@intel.com>
> Sent: Thursday, August 7, 2025 2:42 AM
> To: Reshetova, Elena <elena.reshetova@intel.com>; Hansen, Dave
> <dave.hansen@intel.com>
> Cc: seanjc@google.com; mingo@kernel.org; Scarlata, Vincent R
> <vincent.r.scarlata@intel.com>; x86@kernel.org; jarkko@kernel.org;
> Annapurve, Vishal <vannapurve@google.com>; linux-kernel@vger.kernel.org;
> Mallick, Asit K <asit.k.mallick@intel.com>; Aktas, Erdem
> <erdemaktas@google.com>; Cai, Chong <chongc@google.com>; Bondarevska,
> Nataliia <bondarn@google.com>; linux-sgx@vger.kernel.org; Raynor, Scott
> <scott.raynor@intel.com>
> Subject: Re: [PATCH v11 3/5] x86/sgx: Define error codes for use by
> ENCLS[EUPDATESVN]
> 
> On Wed, 2025-08-06 at 11:11 +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>
> > ---
> >  arch/x86/include/asm/sgx.h | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
> > index 6a0069761508..2da5b3b117a1 100644
> > --- a/arch/x86/include/asm/sgx.h
> > +++ b/arch/x86/include/asm/sgx.h
> > @@ -73,6 +73,10 @@ 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_NO_UPDATE:		EUPDATESVN could not update the
> CPUSVN because the
> > + *				current SVN was not newer than CPUSVN. This
> is the most
> > + *				common error code returned by EUPDATESVN.
> >   * %SGX_UNMASKED_EVENT:		An unmasked event, e.g. INTR, was
> received
> >   */
> >  enum sgx_return_code {
> > @@ -81,6 +85,8 @@ enum sgx_return_code {
> >  	SGX_CHILD_PRESENT		= 13,
> >  	SGX_INVALID_EINITTOKEN		= 16,
> >  	SGX_PAGE_NOT_MODIFIABLE		= 20,
> > +	SGX_INSUFFICIENT_ENTROPY	= 29,
> > +	SGX_NO_UPDATE				= 31,
> 
> It seems the tap/writespace is broken, i.e., this error code is not
> aligned with others.

Not sure how this happened, will fix. 

> 
> >  	SGX_UNMASKED_EVENT		= 128,
> >  };
> >

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

[Reshetova, Elena]

> -----Original Message-----
> From: Huang, Kai <kai.huang@intel.com>
> Sent: Thursday, August 7, 2025 3:15 AM
> To: Reshetova, Elena <elena.reshetova@intel.com>; Hansen, Dave
> <dave.hansen@intel.com>
> Cc: seanjc@google.com; mingo@kernel.org; Scarlata, Vincent R
> <vincent.r.scarlata@intel.com>; x86@kernel.org; jarkko@kernel.org;
> Annapurve, Vishal <vannapurve@google.com>; linux-kernel@vger.kernel.org;
> Mallick, Asit K <asit.k.mallick@intel.com>; Aktas, Erdem
> <erdemaktas@google.com>; Cai, Chong <chongc@google.com>; Bondarevska,
> Nataliia <bondarn@google.com>; linux-sgx@vger.kernel.org; Raynor, Scott
> <scott.raynor@intel.com>
> Subject: Re: [PATCH v11 4/5] x86/sgx: Implement ENCLS[EUPDATESVN]
> 
> On Wed, 2025-08-06 at 11:11 +0300, Elena Reshetova wrote:
> > 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.
> >
> > Add the method to perform ENCLS[EUPDATESVN]. However, until the
> > follow up patch that wires calling sgx_update_svn() from
> > sgx_inc_usage_count(), this code is not reachable.
> 
> Please check the text wrap.
> 

Yes, will do. 

> >
> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> > ---
> >  arch/x86/include/asm/sgx.h      | 31 +++++++-------
> >  arch/x86/kernel/cpu/sgx/encls.h |  5 +++
> >  arch/x86/kernel/cpu/sgx/main.c  | 73
> +++++++++++++++++++++++++++++++++
> >  3 files changed, 94 insertions(+), 15 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
> > index 2da5b3b117a1..0e13678f9cbd 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,
> >  };
> >
> >  /**
> > 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 3a5cbd1c170e..d8c42524b590 100644
> > --- a/arch/x86/kernel/cpu/sgx/main.c
> > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/vmalloc.h>
> >  #include <asm/msr.h>
> >  #include <asm/sgx.h>
> > +#include <asm/archrandom.h>
> >  #include "driver.h"
> >  #include "encl.h"
> >  #include "encls.h"
> > @@ -917,6 +918,78 @@ int sgx_set_attribute(unsigned long
> *allowed_attributes,
> >  }
> >  EXPORT_SYMBOL_GPL(sgx_set_attribute);
> >
> > +/* Counter to count the active SGX users */
> > +static int sgx_usage_count;
> > +
> > +/**
> > + * sgx_update_svn() - Attempt to call ENCLS[EUPDATESVN].
> 
> Add a newline would make the comment more readable.

Ok

> 
> > + * This instruction attempts to update CPUSVN to the
> > + * currently loaded microcode update SVN and generate new
> > + * cryptographic assets.
> 
> Ditto here.

Ok

> 
> > + * Return:
> > + * 0: Success or not supported
> > + * -EAGAIN: Can be safely retried, failure is due to lack of
> > + *  entropy in RNG.
> > + * -EIO: Unexpected error, retries are not advisable.
> > + */
> 
> You may want to make the description of the return code vertically
> aligned.  And looking at the k-doc documentation, the format of the return
> codes could be improved:
> 
> 	* Return:
> 	* * %0:		- Success or not supported
> 	* * %-EAGAIN:	- ...
> 	* * %-EIO:	- ...
> 
> Please see "Return values" part of:
> 
> https://docs.kernel.org/doc-guide/kernel-doc.html
> 
> Or you can switch to use normal comment.  It's a static function anyway.

Ok, will fix the formatting issues. 

> 
> 
> > +static int __maybe_unused sgx_update_svn(void)
> > +{
> > +	int ret;
> > +
> > +	/*
> > +	 * If EUPDATESVN is not available, it is ok to
> > +	 * silently skip it to comply with legacy behavior.
> > +	 */
> > +	if (!cpu_feature_enabled(X86_FEATURE_SGX_EUPDATESVN))
> > +		return 0;
> > +
> > +	/*
> > +	 * EPC is guaranteed to be empty when there are no users.
> > +	 * Ensure we are on our first user before proceeding further.
> > +	 */
> > +	WARN(sgx_usage_count != 1, "Elevated usage count when calling
> EUPDATESVN\n");
> 
> I am not sure whether this is needed.  Wouldn't the ENCLS_WARN() at the
> end catch this case and the user is able to figure out what went wrong
> from the error code?

Dave has made a suggestion to include this check, so I have added it. 

> 
> Besides that, in _this_ patch, what prevents sgx_usage_count from being
> concurrently updated is still unknown.  It's kinda weird to just use it
> here w/o seeing the actual mutex.

In this patch it is fully useless, because sgx_usage_count is never incremented
from zero and this function is also never called. But I didn’t want to move this
addition to the following patch since it would look as one-add to this function. 

> 
> > +
> > +	for (int i = 0; i < RDRAND_RETRY_LOOPS; i++) {
> > +		ret = __eupdatesvn();
> > +
> > +		/* Stop on success or unexpected errors: */
> > +		if (ret != SGX_INSUFFICIENT_ENTROPY)
> > +			break;
> > +	}
> > +
> > +	switch (ret) {
> > +	/*
> > +	 * SVN successfully updated.
> > +	 * Let users know when the update was successful.
> > +	 */
> 
> This is ugly.  I would just put the comment inside the 'case'.

Ok, will move. 

> 
> > +	case 0:
> > +		pr_info("SVN updated successfully\n");
> > +		return 0;
> > +	/*
> > +	 * SVN update failed since the current SVN is
> > +	 * not newer than CPUSVN. This is the most
> > +	 * common case and indicates no harm.
> > +	 */
> > +	case SGX_NO_UPDATE:
> > +		return 0;
> > +	/*
> > +	 * SVN update failed due to lack of entropy in DRNG.
> > +	 * Indicate to userspace that it should retry.
> > +	 */
> > +	case SGX_INSUFFICIENT_ENTROPY:
> > +		return -EAGAIN;
> > +	default:
> > +		break;
> > +	}
> 
> Ditto for all the comments above.

Ok, will move inside. 

Best Regards,
Elena.

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

[Reshetova, Elena]

> -----Original Message-----
> From: Huang, Kai <kai.huang@intel.com>
> Sent: Thursday, August 7, 2025 3:24 AM
> To: Reshetova, Elena <elena.reshetova@intel.com>; Hansen, Dave
> <dave.hansen@intel.com>
> Cc: seanjc@google.com; mingo@kernel.org; Scarlata, Vincent R
> <vincent.r.scarlata@intel.com>; x86@kernel.org; jarkko@kernel.org;
> Annapurve, Vishal <vannapurve@google.com>; linux-kernel@vger.kernel.org;
> Mallick, Asit K <asit.k.mallick@intel.com>; Aktas, Erdem
> <erdemaktas@google.com>; Cai, Chong <chongc@google.com>; Bondarevska,
> Nataliia <bondarn@google.com>; linux-sgx@vger.kernel.org; Raynor, Scott
> <scott.raynor@intel.com>
> Subject: Re: [PATCH v11 5/5] x86/sgx: Enable automatic SVN updates for SGX
> enclaves
> 
> On Wed, 2025-08-06 at 11:11 +0300, Elena Reshetova wrote:
> > == Background ==
> >
> > ENCLS[EUPDATESVN] is a new SGX instruction [1] which allows enclave
> > attestation to include information about updated microcode SVN without a
> > reboot. Before an EUPDATESVN operation can be successful, all SGX memory
> > (aka. EPC) must be marked as “unused” in the SGX hardware metadata
> > (aka.EPCM). This requirement ensures that no compromised enclave can
> > survive the EUPDATESVN procedure and provides an opportunity to generate
> > new cryptographic assets.
> >
> > == Solution ==
> >
> > Attempt to execute ENCLS[EUPDATESVN] every time the first file descriptor
> > is obtained via sgx_(vepc_)open(). In the most common case the microcode
> > SVN is already up-to-date, and the operation succeeds without updating SVN.
> 
> A newline here would be helpful.

OK

> 
> > Note: while in such cases the underlying CR_BASE_KEY is regenrated, it does
> 		      ^ case, since it's just one case, right?
> 
> CR_BASE_KEY comes out of blue.  And the odd is the SDM actually uses
> CR_BASE_PK AFAICT, so it could bring some confusion.
> 
> Perhaps just "crypto-assets" in general?

Sure, will fix both. 

> 
> > not affect enclaves' visible keys obtained via EGETKEY instruction.
> >
> > If it 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's DRNG (RDSEED) and therefore the *open() can
> > be safely retried to allow normal enclave operation.
> 
> please check text wrap.

Yes, will do. 

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

[Dave Hansen]

On 8/8/25 03:59, Reshetova, Elena wrote:
>> I am not sure whether this is needed.  Wouldn't the ENCLS_WARN() at the
>> end catch this case and the user is able to figure out what went wrong
>> from the error code?
> Dave has made a suggestion to include this check, so I have added it. 

If it's important enough to write a comment for it, it's important
enough to write a line of code for it.

Re: [PATCH v11 0/5] Enable automatic SVN updates for SGX enclaves

[Jarkko Sakkinen]

On Wed, Aug 06, 2025 at 11:11:51AM +0300, Elena Reshetova wrote:
> Changes since v10 following reviews by Dave:
> 
>  - merge patch 1 and 2
>  - patch 1: clarify the comment about the function prototype
>  - patch 3: clarify the description of SGX_NO_UPDATE
>   error code, move the definition of EUPDATESVN enum leaf
>   to patch 4
>  - patch 4: clarify commit, adjust sgx_update_svn() function
>   according to feedback
> 
> Changes since v9 following reviews by Kai:
> 
>  - postpone the definition of sgx_inc_usage_count
>    until patch 6
>  - clarify the commit message in patch 6
>  - minor fixes
> 
> Note: I didn't merge patch 1 and 2 since it goes against
> previous suggestion made by Jarkko.
> 
> Changes since v8 following reviews by Dave and Jarkko:
> 
>  - fix the sgx_inc/dec_usage_count() to not do any
>  actual counting until the later patch when adequate
>  mutex is introduced as suggested by Dave
>  - add an additional patch (patch 1) to redefine
>  existing sgx_(vepc_)open into wrappers to allow
>  the follow up patch to introduce the sgx_inc/dec_usage_count()
>  functions into sgx_(vepc_)open cleanly as suggested
>  by Jarkko. 
> 
> Changes since v7 following reviews by Dave:
> 
>  - change sgx_usage_count to be a normal int type
>  and greatly simplify the sgx_inc_usage_count func.
>  This results in requiring a mutex for each sgx_(vepc_)open
>  but given that the mutex is held a short amount of
>  time it should be ok perf-wise.
> 
> Changes since v6 following reviews by Kai,Jarkko
> and Dave:
> 
>  - fix sgx_update_svn function description
>  - add maybe_unused for sgx_update_svn in patch 4
>  to silence the warning, remove it in patch 5
>  - add note to patch 1 explaining why the prototype
>  sgx_inc_usage_count returns int and not void
>  - fix the order of return code checks in 
>  sgx_update_svn
>  - cosmetic fixes
>  
> Note: I didn't change the sgx_inc/dec_usage_count
> to statics because they are called from a number of
> different code locations and also rely on a static
> sgx_usage_count, which lives naturally in main.c.  
>  
> Changes since v5 following reviews by Ingo, Kai,
> Jarkko and Dave:
> 
>  - rebase on x86 tip
>  - patch 1 is fixed to do correct unwinding in
>  case of errors
>  - patch 2: add feature flag to cpuid-deps.c
>  - patch 3: remove unused SGX_EPC_NOT_READY error code
>  - patch 4: fix x86 feature check, return -EAGAIN
>  on SGX_INSUFFICIENT_ENTROPY and -EIO on other non-
>  expected errors. Comments/style are also fixed.
>  - patch 5: rewrite commit message, add comments inline
> 
> 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.16.0_rc7 & sgx selftests.
> Also tested on a Kaby Lake machine without EUPDATESVN support.
> 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?explicitVersion=true
> [2] https://lore.kernel.org/all/20220520103904.1216-1-cathy.zhang@intel.com/
> 
> Elena Reshetova (5):
>   x86/sgx: Introduce functions 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               | 37 ++++++----
>  arch/x86/kernel/cpu/cpuid-deps.c         |  1 +
>  arch/x86/kernel/cpu/scattered.c          |  1 +
>  arch/x86/kernel/cpu/sgx/driver.c         | 19 ++++-
>  arch/x86/kernel/cpu/sgx/encl.c           |  1 +
>  arch/x86/kernel/cpu/sgx/encls.h          |  5 ++
>  arch/x86/kernel/cpu/sgx/main.c           | 91 ++++++++++++++++++++++++
>  arch/x86/kernel/cpu/sgx/sgx.h            |  3 +
>  arch/x86/kernel/cpu/sgx/virt.c           | 20 +++++-
>  tools/arch/x86/include/asm/cpufeatures.h |  1 +
>  11 files changed, 163 insertions(+), 17 deletions(-)
> 
> -- 
> 2.45.2
> 
> 

LGTM

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko

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

[Huang, Kai]

On Fri, 2025-08-08 at 10:59 +0000, Reshetova, Elena wrote:
> > > +	/*
> > > +	 * EPC is guaranteed to be empty when there are no users.
> > > +	 * Ensure we are on our first user before proceeding further.
> > > +	 */
> > > +	WARN(sgx_usage_count != 1, "Elevated usage count when calling
> > EUPDATESVN\n");
> > 
> > I am not sure whether this is needed.  Wouldn't the ENCLS_WARN() at the
> > end catch this case and the user is able to figure out what went wrong
> > from the error code?
> 
> Dave has made a suggestion to include this check, so I have added it. 

Sorry I didn't read careful enough and missed that.

> 
> > 
> > Besides that, in _this_ patch, what prevents sgx_usage_count from being
> > concurrently updated is still unknown.  It's kinda weird to just use it
> > here w/o seeing the actual mutex.
> 
> In this patch it is fully useless, because sgx_usage_count is never incremented
> from zero and this function is also never called. But I didn’t want to move this
> addition to the following patch since it would look as one-add to this function.

Sure np.

Re: [PATCH v11 1/5] x86/sgx: Introduce functions to count the sgx_(vepc_)open()

[Huang, Kai]

On Fri, 2025-08-08 at 10:47 +0000, Reshetova, Elena wrote:
> > 
> > > diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> > > index 308dbbae6c6e..cf149b9f4916 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();
> > >   }
> > > 
> > > 
> > 
> > [...]
> > 
> > > --- a/arch/x86/kernel/cpu/sgx/virt.c
> > > +++ b/arch/x86/kernel/cpu/sgx/virt.c
> > > @@ -255,10 +255,11 @@ static int sgx_vepc_release(struct inode *inode,
> > struct file *file)
> > >   	xa_destroy(&vepc->page_array);
> > >   	kfree(vepc);
> > > 
> > > +	sgx_dec_usage_count();
> > >   	return 0;
> > >   }
> > 
> > Given we have __sgx_(vepc_)open(), I think it makes more sense to have
> > __sgx_(encl_|vepc_)release() counterpart?
> 
> Is it worth it? In case of *_open() variants there are quite error handling
> under different cases, but for release as you can see it is just a one-line
> addition. Not sure it is worth adding the wrappers just for that. 
> But I can change it if people think it would look better this way.

Either way is fine to me.  Feel free to ignore.

RE: [PATCH v11 0/5] Enable automatic SVN updates for SGX enclaves

[Reshetova, Elena]

> >
> 
> LGTM
> 
> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> 
> BR, Jarkko

Thank you very much for your review, Jarkko!

Re: [PATCH v11 0/5] Enable automatic SVN updates for SGX enclaves

[Jarkko Sakkinen]

On Mon, Aug 11, 2025 at 07:21:15AM +0000, Reshetova, Elena wrote:
> > >
> > 
> > LGTM
> > 
> > Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> > 
> > BR, Jarkko
> 
> Thank you very much for your review, Jarkko!

np :-)

BR, Jarkko