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.
