| Message ID | 20250321123938.802763-5-elena.reshetova@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Enable automatic SVN updates for SGX enclaves | expand |
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. > [2] https://lore.kernel.org/all/20220520103904.1216-1-cathy.zhang@intel.com/T/#medb89e6a916337b4f9e68c736a295ba0ae99ac90 Link: > > Co-developed-by: Cathy Zhang <cathy.zhang@intel.com> > Signed-off-by: Cathy Zhang <cathy.zhang@intel.com> > Co-developed-by: Elena Reshetova <elena.reshetova@intel.com> > Signed-off-by: Elena Reshetova <elena.reshetova@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. > + > + 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. > + > + 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. > + } > +} > 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. > +void sgx_updatesvn(void); > + > #endif /* _X86_SGX_H */ > -- > 2.45.2 > BR, Jarkko
> 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.zhang@intel.com/T/#medb89e6a916337b4f9e68c736a295ba0ae99ac90 > > Link: ? Not sure what you mean by it. > > > > > Co-developed-by: Cathy Zhang <cathy.zhang@intel.com> > > Signed-off-by: Cathy Zhang <cathy.zhang@intel.com> > > Co-developed-by: Elena Reshetova <elena.reshetova@intel.com> > > Signed-off-by: Elena Reshetova <elena.reshetova@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.
On Mon, Mar 24, 2025 at 12:26:38PM +0000, Reshetova, Elena wrote: > > > 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.zhang@intel.com/T/#medb89e6a916337b4f9e68c736a295ba0ae99ac90 > > > > Link: > > ? Not sure what you mean by it. > > > > > > > > > Co-developed-by: Cathy Zhang <cathy.zhang@intel.com> > > > Signed-off-by: Cathy Zhang <cathy.zhang@intel.com> > > > Co-developed-by: Elena Reshetova <elena.reshetova@intel.com> > > > Signed-off-by: Elena Reshetova <elena.reshetova@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; This is at least definite removal. We don't log for zero codes. > > > + 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. Maybe pr_debug() is them wrong level if they are meant for sysadmins? I mean these should not happen in normal behavior like ever? As pr_debug() I don't really grab this. > > > > > > > > + } > > > +} > > > 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! NP, despite all the complains this are really just minor details that we need to nail :-) I'm sure we get them right within round or two... > > Best Regards, > Elena. BR, Jarkko
> On Mon, Mar 24, 2025 at 12:26:38PM +0000, Reshetova, Elena wrote: > > > > > 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.zhang@intel.com/T/#medb89e6a916337b4f9e68c736a295ba0ae99ac90 > > > > > > Link: > > > > ? Not sure what you mean by it. > > > > > > > > > > > > > Co-developed-by: Cathy Zhang <cathy.zhang@intel.com> > > > > Signed-off-by: Cathy Zhang <cathy.zhang@intel.com> > > > > Co-developed-by: Elena Reshetova <elena.reshetova@intel.com> > > > > Signed-off-by: Elena Reshetova <elena.reshetova@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; > > This is at least definite removal. We don't log for zero codes. Ok. > > > > > + 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. > > Maybe pr_debug() is them wrong level if they are meant for sysadmins? > > I mean these should not happen in normal behavior like ever? As > pr_debug() I don't really grab this. SGX_NO_UPDATE will absolutely happen normally all the time. Since EUPDATESVN is executed every time EPC is empty, this is the most common code you will get back (because microcode updates are rare). Others yes, that would indicate some error condition. So, what is the pr_level that you would suggest? > > > > > > > > > > > > > > + } > > > > +} > > > > 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! > > NP, despite all the complains this are really just minor details > that we need to nail :-) I'm sure we get them right within round > or two... Sure, happy to do the nailing )) Best Regards, Elena
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; + + 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; + + 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; + } +} 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); +void sgx_updatesvn(void); + #endif /* _X86_SGX_H */