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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aCt3XZ0m40wuA9bU@kernel.org>
Date: Mon, 19 May 2025 21:24:29 +0300
From: Jarkko Sakkinen <jarkko@...nel.org>
To: Elena Reshetova <elena.reshetova@...el.com>
Cc: dave.hansen@...el.com, seanjc@...gle.com, kai.huang@...el.com,
	linux-sgx@...r.kernel.org, linux-kernel@...r.kernel.org,
	x86@...nel.org, asit.k.mallick@...el.com,
	vincent.r.scarlata@...el.com, chongc@...gle.com,
	erdemaktas@...gle.com, vannapurve@...gle.com,
	dionnaglaze@...gle.com, bondarn@...gle.com, scott.raynor@...el.com
Subject: Re: [PATCH v5 4/5] x86/sgx: Implement ENCLS[EUPDATESVN]

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