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

Elena Reshetova posted 5 patches 1 month, 3 weeks ago
There is a newer version of this series
[PATCH v14 5/5] x86/sgx: Enable automatic SVN updates for SGX enclaves
Posted by Elena Reshetova 1 month, 3 weeks ago
== 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 crypto assets are regenerated, 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

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
---
 arch/x86/kernel/cpu/sgx/main.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 69ab28641e20..cff5c4d22ac2 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -934,7 +934,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;
 
@@ -992,14 +992,29 @@ 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)
 {
+	int ret;
+
+	guard(mutex)(&sgx_svn_lock);
+
+	if (!sgx_usage_count) {
+		ret = sgx_update_svn();
+		if (ret)
+			return ret;
+	}
+
+	sgx_usage_count++;
+
 	return 0;
 }
 
 void sgx_dec_usage_count(void)
 {
-	return;
+	sgx_usage_count--;
 }
 
 static int __init sgx_init(void)
-- 
2.45.2

Re: [PATCH v14 5/5] x86/sgx: Enable automatic SVN updates for SGX enclaves
Posted by Dave Hansen 1 month, 3 weeks ago
On 8/14/25 00:34, Elena Reshetova wrote:
> +/* Mutex to ensure no concurrent EPC accesses during EUPDATESVN */
> +static DEFINE_MUTEX(sgx_svn_lock);
> +
>  int sgx_inc_usage_count(void)
>  {
> +	int ret;
> +
> +	guard(mutex)(&sgx_svn_lock);
> +
> +	if (!sgx_usage_count) {
> +		ret = sgx_update_svn();
> +		if (ret)
> +			return ret;
> +	}
> +
> +	sgx_usage_count++;
> +
>  	return 0;
>  }
>  
>  void sgx_dec_usage_count(void)
>  {
> -	return;
> +	sgx_usage_count--;
>  }

How is a plain int-- safe?

Where's the locking?
Re: [PATCH v14 5/5] x86/sgx: Enable automatic SVN updates for SGX enclaves
Posted by Huang, Kai 1 month, 2 weeks ago
On Thu, 2025-08-14 at 09:50 -0700, Dave Hansen wrote:
> On 8/14/25 00:34, Elena Reshetova wrote:
> > +/* Mutex to ensure no concurrent EPC accesses during EUPDATESVN */
> > +static DEFINE_MUTEX(sgx_svn_lock);
> > +
> >  int sgx_inc_usage_count(void)
> >  {
> > +	int ret;
> > +
> > +	guard(mutex)(&sgx_svn_lock);
> > +
> > +	if (!sgx_usage_count) {
> > +		ret = sgx_update_svn();
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	sgx_usage_count++;
> > +
> >  	return 0;
> >  }
> >  
> >  void sgx_dec_usage_count(void)
> >  {
> > -	return;
> > +	sgx_usage_count--;
> >  }
> 
> How is a plain int-- safe?
> 
> Where's the locking?

Sorry I missed this during review too.
RE: [PATCH v14 5/5] x86/sgx: Enable automatic SVN updates for SGX enclaves
Posted by Reshetova, Elena 1 month, 2 weeks ago

> -----Original Message-----
> From: Huang, Kai <kai.huang@intel.com>
> Sent: Friday, August 15, 2025 12:41 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 v14 5/5] x86/sgx: Enable automatic SVN updates for SGX
> enclaves
> 
> On Thu, 2025-08-14 at 09:50 -0700, Dave Hansen wrote:
> > On 8/14/25 00:34, Elena Reshetova wrote:
> > > +/* Mutex to ensure no concurrent EPC accesses during EUPDATESVN */
> > > +static DEFINE_MUTEX(sgx_svn_lock);
> > > +
> > >  int sgx_inc_usage_count(void)
> > >  {
> > > +	int ret;
> > > +
> > > +	guard(mutex)(&sgx_svn_lock);
> > > +
> > > +	if (!sgx_usage_count) {
> > > +		ret = sgx_update_svn();
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > > +	sgx_usage_count++;
> > > +
> > >  	return 0;
> > >  }
> > >
> > >  void sgx_dec_usage_count(void)
> > >  {
> > > -	return;
> > > +	sgx_usage_count--;
> > >  }
> >
> > How is a plain int-- safe?
> >
> > Where's the locking?
> 
> Sorry I missed this during review too.

My line of thinking went that we don't actually
care or act on decrement (it is not a true ref counter)
and therefore, races here are ok. What I forgot is that
we loose basic atomicity also with plain int ((

I would personally like to go back to atomic (this is
what it is exactly for), but I can also just add another
mutex here. Preferences? 

Best Regards,
Elena.


Re: [PATCH v14 5/5] x86/sgx: Enable automatic SVN updates for SGX enclaves
Posted by Huang, Kai 1 month, 2 weeks ago
> > > > 
> > > >  void sgx_dec_usage_count(void)
> > > >  {
> > > > -	return;
> > > > +	sgx_usage_count--;
> > > >  }
> > > 
> > > How is a plain int-- safe?
> > > 
> > > Where's the locking?
> > 
> > Sorry I missed this during review too.
> 
> My line of thinking went that we don't actually
> care or act on decrement (it is not a true ref counter)
> and therefore, races here are ok. What I forgot is that
> we loose basic atomicity also with plain int ((
> 
> I would personally like to go back to atomic (this is
> what it is exactly for), but I can also just add another
> mutex here. Preferences? 

You don't need another mutex AFAICT, just use the one you already have.

The problem of the raw 'count--' is it is not multiple threads safe, e.g.,
IIUC, you could effectively lose one or more subtractions here, leading to
counter never reaching to 0.

From the perspective of functionality, to me there's no difference between
mutex vs atomic_t, so either would be fine.  But as shown in your v7 [*],
it appears atomic_t version is still slightly more complicated than the
mutex.

So since we are already here, I would say just use the mutex:

void sgx_dec_usage_count(void)
{
	guard(mutex)(&sgx_svn_lock);
	sgx_usage_count--;
}

Am I missing anything?

[*]
https://lore.kernel.org/linux-sgx/20250711165212.1354943-1-elena.reshetova@intel.com/T/#me3d9ca942330039a59e2dd6e1d14b81c6670f87a
RE: [PATCH v14 5/5] x86/sgx: Enable automatic SVN updates for SGX enclaves
Posted by Reshetova, Elena 1 month, 2 weeks ago

> -----Original Message-----
> From: Huang, Kai <kai.huang@intel.com>
> Sent: Monday, August 18, 2025 8:04 AM
> To: Reshetova, Elena <elena.reshetova@intel.com>; Hansen, Dave
> <dave.hansen@intel.com>
> Cc: linux-sgx@vger.kernel.org; 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>; seanjc@google.com; Raynor, Scott
> <scott.raynor@intel.com>
> Subject: Re: [PATCH v14 5/5] x86/sgx: Enable automatic SVN updates for SGX
> enclaves
> 
> > > > >
> > > > >  void sgx_dec_usage_count(void)
> > > > >  {
> > > > > -	return;
> > > > > +	sgx_usage_count--;
> > > > >  }
> > > >
> > > > How is a plain int-- safe?
> > > >
> > > > Where's the locking?
> > >
> > > Sorry I missed this during review too.
> >
> > My line of thinking went that we don't actually
> > care or act on decrement (it is not a true ref counter)
> > and therefore, races here are ok. What I forgot is that
> > we loose basic atomicity also with plain int ((
> >
> > I would personally like to go back to atomic (this is
> > what it is exactly for), but I can also just add another
> > mutex here. Preferences?
> 
> You don't need another mutex AFAICT, just use the one you already have.
> 
> The problem of the raw 'count--' is it is not multiple threads safe, e.g.,
> IIUC, you could effectively lose one or more subtractions here, leading to
> counter never reaching to 0.

Yes, this is what I call atomicity. 

> 
> From the perspective of functionality, to me there's no difference between
> mutex vs atomic_t, so either would be fine.  But as shown in your v7 [*],
> it appears atomic_t version is still slightly more complicated than the
> mutex.

Yes, but it was because originally we tried to avoid mutex/lock in
fast (non-zero increment part). If we drop this requirement and condition
everything with the mutex (as now), then we can go back to simpler handling
via atomic in inc() and in dec() it would be simple atomic_dec(). Smth like this:

int sgx_inc_usage_count(void)
{
	int ret;

	guard(mutex)(&sgx_svn_lock);

	if (atomic64_read(&sgx_usage_count)==0) {
		ret = sgx_update_svn();
		if (ret)
			return ret;
	}

	atomic64_inc(&sgx_usage_count);

 	return 0;
}

void sgx_dec_usage_count(void)
{
	atomic64_dec(&sgx_usage_count);
}

> So since we are already here, I would say just use the mutex:
> 
> void sgx_dec_usage_count(void)
> {
> 	guard(mutex)(&sgx_svn_lock);
> 	sgx_usage_count--;
> }
> 
> Am I missing anything?

The mutex just seems such an overkill for plain atomicity requirement here
in dec(). But sure, I can send the next version with mutex. 

Best Regards,
Elena.  
Re: [PATCH v14 5/5] x86/sgx: Enable automatic SVN updates for SGX enclaves
Posted by Huang, Kai 1 month, 3 weeks ago
On Thu, 2025-08-14 at 10:34 +0300, Reshetova, Elena 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.
> 
> Note: while in such cases the underlying crypto assets are regenerated, 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
> 
> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> 

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