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

["Huang, Kai" <kai.huang@intel.com>]

On Fri, 2025-04-18 at 07:55 -0700, Sean Christopherson wrote:
> 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()?

The only thing I don't like about this is we need to hook both of them.

> Then you're hooking a relative slow path (I assume EUPDATESVN is not fast), 
> 

I was expecting the SGX_NO_UPDATE case should be fast, i.e., some sort of simple
compare and return, but looking at the pseudo code it still re-generates the
CR_BASE_KEY etc.

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

Seems reasonable to me.

> 
> > +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.

Agreed.

> 
> > +
> > +	/*
> > +	 * 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.

The spec is here:

[1] https://cdrdv2.intel.com/v1/dl/getContent/648682?explicitVersion=true

And from the pseudo code it doesn't distinguish 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

I am open to this HYPERVISOR check, because I don't think we currently support
any kind of "passthrough" setup? And depending on what kinda of "passthrough"
types, the things that the hypervisor traps can vary.

Anyway, I agree it's not necessary to have this check, because currently it is
not possible for a guest to see the EUPDATESVN in CPUID.