> On Fri, Mar 21, 2025 at 02:34:43PM +0200, Elena Reshetova wrote: > > SGX architecture introduced a new instruction called EUPDATESVN [1] > > to Ice Lake. It allows updating security SVN version, given that EPC > > is completely empty. The latter is required for security reasons > > in order to reason that enclave security posture is as secure as the > > security SVN version of the TCB that created it. > > > > Additionally it is important to ensure that while ENCLS[EUPDATESVN] > > runs, no concurrent page creation happens in EPC, because it might > > result in #GP delivered to the creator. Legacy SW might not be prepared > > to handle such unexpected #GPs and therefore this patch introduces > > a locking mechanism to ensure no concurrent EPC allocations can happen. > > > > It is also ensured that ENCLS[EUPDATESVN] is not called when running > > in a VM since it does not have a meaning in this context (microcode > > updates application is limited to the host OS) and will create > > unnecessary load. > > > > The implementation of ENCLS[EUPDATESVN] is based on previous > submision in [2] > > > > [1] https://cdrdv2.intel.com/v1/dl/getContent/648682?explicitVersion=true > > I don't think for Intel opcodes a link is needed. We know where to look > them up.
Ok, I can drop this reference. It is just a whitepaper explaining to readers the background and operation of EUPDATESVN. It is not part of standard SDM, so I thought I would put a link. > > > [2] https://lore.kernel.org/all/20220520103904.1216-1- > cathy.zh...@intel.com/T/#medb89e6a916337b4f9e68c736a295ba0ae99ac90 > > Link: ? Not sure what you mean by it. > > > > > Co-developed-by: Cathy Zhang <cathy.zh...@intel.com> > > Signed-off-by: Cathy Zhang <cathy.zh...@intel.com> > > Co-developed-by: Elena Reshetova <elena.reshet...@intel.com> > > Signed-off-by: Elena Reshetova <elena.reshet...@intel.com> > > --- > > arch/x86/include/asm/sgx.h | 33 +++++++++-------- > > arch/x86/kernel/cpu/sgx/encls.h | 6 +++ > > arch/x86/kernel/cpu/sgx/main.c | 65 > ++++++++++++++++++++++++++++++++- > > arch/x86/kernel/cpu/sgx/sgx.h | 2 + > > 4 files changed, 90 insertions(+), 16 deletions(-) > > > > diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h > > index 8ba39bbf4e91..5caf5c31ebc6 100644 > > --- a/arch/x86/include/asm/sgx.h > > +++ b/arch/x86/include/asm/sgx.h > > @@ -26,23 +26,26 @@ > > #define SGX_CPUID_EPC_SECTION 0x1 > > /* The bitmask for the EPC section type. */ > > #define SGX_CPUID_EPC_MASK GENMASK(3, 0) > > +/* EUPDATESVN presence indication */ > > +#define SGX_CPUID_EUPDATESVN BIT(10) > > > > enum sgx_encls_function { > > - ECREATE = 0x00, > > - EADD = 0x01, > > - EINIT = 0x02, > > - EREMOVE = 0x03, > > - EDGBRD = 0x04, > > - EDGBWR = 0x05, > > - EEXTEND = 0x06, > > - ELDU = 0x08, > > - EBLOCK = 0x09, > > - EPA = 0x0A, > > - EWB = 0x0B, > > - ETRACK = 0x0C, > > - EAUG = 0x0D, > > - EMODPR = 0x0E, > > - EMODT = 0x0F, > > + ECREATE = 0x00, > > + EADD = 0x01, > > + EINIT = 0x02, > > + EREMOVE = 0x03, > > + EDGBRD = 0x04, > > + EDGBWR = 0x05, > > + EEXTEND = 0x06, > > + ELDU = 0x08, > > + EBLOCK = 0x09, > > + EPA = 0x0A, > > + EWB = 0x0B, > > + ETRACK = 0x0C, > > + EAUG = 0x0D, > > + EMODPR = 0x0E, > > + EMODT = 0x0F, > > + EUPDATESVN = 0x18, > > }; > > > > /** > > diff --git a/arch/x86/kernel/cpu/sgx/encls.h > b/arch/x86/kernel/cpu/sgx/encls.h > > index 99004b02e2ed..3d83c76dc91f 100644 > > --- a/arch/x86/kernel/cpu/sgx/encls.h > > +++ b/arch/x86/kernel/cpu/sgx/encls.h > > @@ -233,4 +233,10 @@ static inline int __eaug(struct sgx_pageinfo *pginfo, > void *addr) > > return __encls_2(EAUG, pginfo, addr); > > } > > > > +/* Update CPUSVN at runtime. */ > > +static inline int __eupdatesvn(void) > > +{ > > + return __encls_ret_1(EUPDATESVN, ""); > > +} > > + > > #endif /* _X86_ENCLS_H */ > > diff --git a/arch/x86/kernel/cpu/sgx/main.c > b/arch/x86/kernel/cpu/sgx/main.c > > index b61d3bad0446..698921229094 100644 > > --- a/arch/x86/kernel/cpu/sgx/main.c > > +++ b/arch/x86/kernel/cpu/sgx/main.c > > @@ -32,6 +32,11 @@ static DEFINE_XARRAY(sgx_epc_address_space); > > static LIST_HEAD(sgx_active_page_list); > > static DEFINE_SPINLOCK(sgx_reclaimer_lock); > > > > +/* 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; > > > > @@ -457,7 +462,17 @@ static struct sgx_epc_page > *__sgx_alloc_epc_page_from_node(int nid) > > page->flags = 0; > > > > spin_unlock(&node->lock); > > - atomic_long_inc(&sgx_nr_used_pages); > > + > > + if (!atomic_long_inc_not_zero(&sgx_nr_used_pages)) { > > + spin_lock(&sgx_epc_eupdatesvn_lock); > > + /* Only call sgx_updatesvn() once the first enclave's > > + * page is allocated from EPC > > + */ > > + if (atomic_long_read(&sgx_nr_used_pages) == 0) > > + sgx_updatesvn(); > > + atomic_long_inc(&sgx_nr_used_pages); > > + spin_unlock(&sgx_epc_eupdatesvn_lock); > > + } > > > > return page; > > } > > @@ -970,3 +985,51 @@ static int __init sgx_init(void) > > } > > > > device_initcall(sgx_init); > > + > > +/** > > + * sgx_updatesvn() - Issue ENCLS[EUPDATESVN] > > + * If EPC is ready, this instruction will update CPUSVN to the currently > > + * loaded microcode update SVN and generate new cryptographic assets. > > + */ > > +void sgx_updatesvn(void) > > +{ > > + int ret; > > + int retry = 10; > > Reverse declaration order. Sure, will fix. > > > + > > + lockdep_assert_held(&sgx_epc_eupdatesvn_lock); > > + > > + /* Do not execute EUPDATESVN if instruction is unavalible or running > in a VM */ > > + if (!(cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN) || > > + boot_cpu_has(X86_FEATURE_HYPERVISOR)) > > + return; > > > if (!(cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN)) > return; > > /* ... */ > if (boot_cpu_has(X86_FEATURE_HYPERVISOR)) > return; > > The first check really does not need a comment. The second would benefit > from explaining why bail out inside a VM. Sure, I can change. The reason why we dont want the execution in a VM is explained in the commit message, I can duplicate it in the code comment also. > > > > > > > + > > + do { > > + ret = __eupdatesvn(); > > + if (ret != SGX_INSUFFICIENT_ENTROPY) > > + break; > > + > > + } while (--retry); > > + > > + switch (ret) { > > + case 0: > > + pr_debug("EUPDATESVN was successful!\n"); > > + break; > > + case SGX_NO_UPDATE: > > + pr_debug("EUPDATESVN was successful, but CPUSVN was not > updated, " > > + "because current SVN was not newer than > CPUSVN.\n"); > > + break; > > + case SGX_EPC_NOT_READY: > > + pr_debug("EPC is not ready for SVN update."); > > + break; > > + case SGX_INSUFFICIENT_ENTROPY: > > + pr_debug("CPUSVN update is failed due to Insufficient > entropy in RNG, " > > + "please try it later.\n"); > > + break; > > + case SGX_EPC_PAGE_CONFLICT: > > + pr_debug("CPUSVN update is failed due to concurrency > violation, please " > > + "stop running any other ENCLS leaf and try it > later.\n"); > > + break; > > + default: > > + break; > > Remove pr_debug() statements. This I am not sure it is good idea. I think it would be useful for system admins to have a way to see that update either happened or not. It is true that you can find this out by requesting a new SGX attestation quote (and see if newer SVN is used), but it is not the faster way. > > > + } > > +} > > diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h > > index d2dad21259a8..92c9e226f1b5 100644 > > --- a/arch/x86/kernel/cpu/sgx/sgx.h > > +++ b/arch/x86/kernel/cpu/sgx/sgx.h > > @@ -104,4 +104,6 @@ static inline int __init sgx_vepc_init(void) > > > > void sgx_update_lepubkeyhash(u64 *lepubkeyhash); > > > > I don't think we need a newline in-between. Sure, will fix. Thank you very much for your prompt review Jarkko! Best Regards, Elena.
