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

[Huang, Kai]

On Mon, 2025-05-19 at 10:24 +0300, Elena Reshetova wrote:
> The SGX attestation architecture assumes a compromise
> of all running enclaves and cryptographic assets
> (like internal SGX encryption keys) whenever a microcode
> update affects SGX. To mitigate the impact of this presumed
> compromise, a new supervisor SGX instruction: ENCLS[EUPDATESVN],
> is introduced to update SGX microcode version and generate
> new cryptographic assets in runtime after SGX microcode update.
> 
> EUPDATESVN requires that SGX memory to be marked as "unused"
> before it will succeed. This ensures that no compromised enclave
> can survive the process and provides an opportunity to generate
> new cryptographic assets.
> 
> Add the method to perform ENCLS[EUPDATESVN].
> 
> Signed-off-by: Elena Reshetova <elena.reshetova@xxxxxxxxx>
> ---
>  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);