Re: [PATCH v3 2/2] x86/sgx: Implement EUPDATESVN and opportunistically call it during first EPC page alloc

[Sean Christopherson <seanjc@google.com>]

On Tue, Apr 15, 2025, Elena Reshetova wrote:
> +/* This lock is held to prevent new EPC pages from being created
> + * during the execution of ENCLS[EUPDATESVN].
> + */
> +static DEFINE_SPINLOCK(sgx_epc_eupdatesvn_lock);
> +
>  static atomic_long_t sgx_nr_used_pages = ATOMIC_LONG_INIT(0);
>  static unsigned long sgx_nr_total_pages;
>  
> @@ -444,6 +449,7 @@ static struct sgx_epc_page *__sgx_alloc_epc_page_from_node(int nid)
>  {
>  	struct sgx_numa_node *node = &sgx_numa_nodes[nid];
>  	struct sgx_epc_page *page = NULL;
> +	bool ret;
>  
>  	spin_lock(&node->lock);
>  
> @@ -452,12 +458,33 @@ static struct sgx_epc_page *__sgx_alloc_epc_page_from_node(int nid)
>  		return NULL;
>  	}
>  
> +	if (!atomic_long_inc_not_zero(&sgx_nr_used_pages)) {

FWIW, the entire automatic scheme has terrible behavior when KVM is running SGX
capable guests.  KVM will allocate EPC when the virtual EPC is first touched by
the guest, and won't free the EPC pages until the VM is terminated.  And IIRC,
userspace (QEMU) typically preallocates the virtual EPC to ensure the VM doesn't
need to be killed later on due to lack of EPC.

I.e. running an SGX capable VM, even if there are no active enclaves in said VM,
will prevent SVN updates.

Unfortunately, I can't think of a sane way around that, because while it would
be easy enough to force interception of ECREATE, there's no definitive ENCLS leaf
that KVM can intercept to detect when an enclave is destroyed (interception
EREMOVE would have terrible performance implications).

That said, handling this deep in the bowels of EPC page allocation seems
unnecessary.  The only way for there to be no active EPC pages is if there are
no enclaves.  As above, virtual EPC usage is already all but guaranteed to hit
false positives, so I don't think there's anything gained by trying to update
the SVN based on EPC allocations.

So rather than react to EPC allocations, why not hook sgx_open() and sgx_vepc_open()?
Then you're hooking a relative slow path (I assume EUPDATESVN is not fast), error
codes can be returned to userspace (if you really want the kernel to freak out if
EUPDATESVN unexpected fails), and you don't _have_ to use a spinlock, e.g. if
EUPDATESVN takes a long time, using a mutex might be desirable.

> +bool sgx_updatesvn(void)
> +{
> +	int retry = 10;
> +	int ret;
> +
> +	lockdep_assert_held(&sgx_epc_eupdatesvn_lock);
> +
> +	if (!(cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN))
> +		return true;

Checking CPUID features inside the spinlock is asinine.  They can't change, barring
a misbehaving hypervisor.  This should be a "cache once during boot" sorta thing.

> +
> +	/*
> +	 * Do not execute ENCLS[EUPDATESVN] if running in a VM since
> +	 * microcode updates are only meaningful to be applied on the host.

I don't think the kernel should check HYPERVISOR.  The SDM doesn't actually
say anything about EUPDATESVN, but unless it's explicitly special cased, I doubt
XuCode cares whether ENCLS was executed in Root vs. Non-Root.

It's the hypervisor's responsibility to enumerate SGX_CPUID_EUPDATESVN or not.
E.g. if the kernel is running in a "passthrough" type setup, it would be perfectly
fine to do EUPDATESVN from a guest kernel.  EUPDATESVN could even be proxied,
e.g. by intercepting ECREATE to track EPC usage