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: <DM8PR11MB5750CC5686E413A88CF41D7DE79CA@DM8PR11MB5750.namprd11.prod.outlook.com>
Date: Mon, 19 May 2025 11:41:35 +0000
From: "Reshetova, Elena" <elena.reshetova@...el.com>
To: "Huang, Kai" <kai.huang@...el.com>, "Hansen, Dave" <dave.hansen@...el.com>
CC: "Raynor, Scott" <scott.raynor@...el.com>, "linux-sgx@...r.kernel.org"
	<linux-sgx@...r.kernel.org>, "Scarlata, Vincent R"
	<vincent.r.scarlata@...el.com>, "x86@...nel.org" <x86@...nel.org>,
	"jarkko@...nel.org" <jarkko@...nel.org>, "Annapurve, Vishal"
	<vannapurve@...gle.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "Mallick, Asit K" <asit.k.mallick@...el.com>,
	"Aktas, Erdem" <erdemaktas@...gle.com>, "Cai, Chong" <chongc@...gle.com>,
	"bondarn@...gle.com" <bondarn@...gle.com>, "seanjc@...gle.com"
	<seanjc@...gle.com>, "dionnaglaze@...gle.com" <dionnaglaze@...gle.com>
Subject: RE: [PATCH v5 4/5] x86/sgx: Implement ENCLS[EUPDATESVN]

> 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@...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
> > + * 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);

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ