linux-kernel - Re: [PATCH v7 5/5] x86/sgx: Enable automatic SVN updates for SGX enclaves

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: <461a33d3-f91a-4dd3-bb97-048670530b25@intel.com>
Date: Fri, 11 Jul 2025 10:54:06 -0700
From: Dave Hansen <dave.hansen@...el.com>
To: Elena Reshetova <elena.reshetova@...el.com>
Cc: jarkko@...nel.org, seanjc@...gle.com, kai.huang@...el.com,
 mingo@...nel.org, 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 v7 5/5] x86/sgx: Enable automatic SVN updates for SGX
 enclaves

On 7/11/25 09:50, Elena Reshetova 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.
> 
> == Patch Contents ==
> 
> 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.
> 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.

The effort to add paragraphs would be appreciated.

>  int sgx_inc_usage_count(void)
>  {
> -	atomic64_inc(&sgx_usage_count);
> -	return 0;
> +	int ret;
...

For what it does sgx_inc_usage_count() is seriously hard to parse and
complicated. Three logical atomic ops *and* a spinlock? Wouldn't this
suffice?

Just make 'sgx_usage_count' a normal integer and always guard it with a
the existing lock:

int sgx_inc_usage_count(void)
{
	int ret;

	guard(mutex)(&sgx_svn_lock);

	if (sgx_usage_count++ == 0)
		return sgx_update_svn();

	return 0;
}

Yeah, it makes the fast path a spinlock instead of an atomic_inc, but
it's still the same number of atomic ops in the end.

Isn't that a billion times more readable? It barely even needs commenting.

What am I missing?