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

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

 > +/**
> > + * 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.

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

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.

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.

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);

> 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);

> > 
> > > +
> > > +	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.

 > >
> > > 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.

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.