| Message ID | 20250328125859.73803-3-elena.reshetova@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Enable automatic SVN updates for SGX enclaves | expand |
On Fri, Mar 28, 2025 at 02:57:41PM +0200, Elena Reshetova wrote: > SGX architecture introduced a new instruction called EUPDATESVN > 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. > > This patch is based on previous submision by Cathy Zhang > https://lore.kernel.org/all/20220520103904.1216-1-cathy.zhang@intel.com/ > > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com> > --- > arch/x86/include/asm/sgx.h | 41 +++++++++++++-------- > arch/x86/kernel/cpu/sgx/encls.h | 6 ++++ > arch/x86/kernel/cpu/sgx/main.c | 63 ++++++++++++++++++++++++++++++++- > arch/x86/kernel/cpu/sgx/sgx.h | 1 + > 4 files changed, 95 insertions(+), 16 deletions(-) > > diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h > index 6a0069761508..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, > }; > > /** > @@ -73,6 +76,11 @@ enum sgx_encls_function { > * public key does not match IA32_SGXLEPUBKEYHASH. > * %SGX_PAGE_NOT_MODIFIABLE: The EPC page cannot be modified because it > * is in the PENDING or MODIFIED state. > + * %SGX_INSUFFICIENT_ENTROPY: Insufficient entropy in RNG. > + * %SGX_EPC_NOT_READY: EPC is not ready for SVN update. > + * %SGX_NO_UPDATE: EUPDATESVN was successful, but CPUSVN was not > + * updated because current SVN was not newer than > + * CPUSVN. > * %SGX_UNMASKED_EVENT: An unmasked event, e.g. INTR, was received > */ > enum sgx_return_code { > @@ -81,6 +89,9 @@ enum sgx_return_code { > SGX_CHILD_PRESENT = 13, > SGX_INVALID_EINITTOKEN = 16, > SGX_PAGE_NOT_MODIFIABLE = 20, > + SGX_INSUFFICIENT_ENTROPY = 29, > + SGX_EPC_NOT_READY = 30, > + SGX_NO_UPDATE = 31, > SGX_UNMASKED_EVENT = 128, > }; > > 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..24563110811d 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,49 @@ 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 retry = 10; > + int ret; > + > + lockdep_assert_held(&sgx_epc_eupdatesvn_lock); > + > + if (!(cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN)) > + return; > + > + /* Do not execute ENCLS[EUPDATESVN] if running in a VM since > + * microcode updates are only meaningful to be applied on the host. > + */ > + if (boot_cpu_has(X86_FEATURE_HYPERVISOR)) > + return; > + > + do { > + ret = __eupdatesvn(); > + if (ret != SGX_INSUFFICIENT_ENTROPY) > + break; > + > + } while (--retry); > + > + switch (ret) { > + case 0: > + pr_info("EUPDATESVN: success\n"); > + break; > + case SGX_EPC_NOT_READY: > + case SGX_INSUFFICIENT_ENTROPY: > + case SGX_EPC_PAGE_CONFLICT: > + pr_err("EUPDATESVN: error %d\n", ret); > + break; > + case SGX_NO_UPDATE: > + break; > + default: > + pr_err("EUPDATESVN: unknown error %d\n", ret); > + break; > + } Overall, I think you're right in that "inversion" does make sense, now that other stuff is better aligned. At least when there is spurious error, I think ioctl's should stop responding and driver should not do anything useful anymore. I.e., it should go out-of-service. I don't think the driver should tear-down, just stop servicing VM's and responding ioctl's. Possibly thish should be also right action for other errors than "insufficient entropy" but I'm open for comments for this. BR, Jarkko
On Fri, Mar 28, 2025 at 07:50:43PM +0200, Jarkko Sakkinen wrote: > On Fri, Mar 28, 2025 at 02:57:41PM +0200, Elena Reshetova wrote: > > SGX architecture introduced a new instruction called EUPDATESVN > > 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. > > > > This patch is based on previous submision by Cathy Zhang > > https://lore.kernel.org/all/20220520103904.1216-1-cathy.zhang@intel.com/ > > > > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com> > > --- > > arch/x86/include/asm/sgx.h | 41 +++++++++++++-------- > > arch/x86/kernel/cpu/sgx/encls.h | 6 ++++ > > arch/x86/kernel/cpu/sgx/main.c | 63 ++++++++++++++++++++++++++++++++- > > arch/x86/kernel/cpu/sgx/sgx.h | 1 + > > 4 files changed, 95 insertions(+), 16 deletions(-) > > > > diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h > > index 6a0069761508..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, > > }; > > > > /** > > @@ -73,6 +76,11 @@ enum sgx_encls_function { > > * public key does not match IA32_SGXLEPUBKEYHASH. > > * %SGX_PAGE_NOT_MODIFIABLE: The EPC page cannot be modified because it > > * is in the PENDING or MODIFIED state. > > + * %SGX_INSUFFICIENT_ENTROPY: Insufficient entropy in RNG. > > + * %SGX_EPC_NOT_READY: EPC is not ready for SVN update. > > + * %SGX_NO_UPDATE: EUPDATESVN was successful, but CPUSVN was not > > + * updated because current SVN was not newer than > > + * CPUSVN. > > * %SGX_UNMASKED_EVENT: An unmasked event, e.g. INTR, was received > > */ > > enum sgx_return_code { > > @@ -81,6 +89,9 @@ enum sgx_return_code { > > SGX_CHILD_PRESENT = 13, > > SGX_INVALID_EINITTOKEN = 16, > > SGX_PAGE_NOT_MODIFIABLE = 20, > > + SGX_INSUFFICIENT_ENTROPY = 29, > > + SGX_EPC_NOT_READY = 30, > > + SGX_NO_UPDATE = 31, > > SGX_UNMASKED_EVENT = 128, > > }; > > > > 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..24563110811d 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,49 @@ 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 retry = 10; > > + int ret; > > + > > + lockdep_assert_held(&sgx_epc_eupdatesvn_lock); > > + > > + if (!(cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN)) > > + return; > > + > > + /* Do not execute ENCLS[EUPDATESVN] if running in a VM since > > + * microcode updates are only meaningful to be applied on the host. > > + */ > > + if (boot_cpu_has(X86_FEATURE_HYPERVISOR)) > > + return; > > + > > + do { > > + ret = __eupdatesvn(); > > + if (ret != SGX_INSUFFICIENT_ENTROPY) > > + break; > > + > > + } while (--retry); > > + > > + switch (ret) { > > + case 0: > > + pr_info("EUPDATESVN: success\n"); > > + break; > > + case SGX_EPC_NOT_READY: > > + case SGX_INSUFFICIENT_ENTROPY: > > + case SGX_EPC_PAGE_CONFLICT: > > + pr_err("EUPDATESVN: error %d\n", ret); > > + break; > > + case SGX_NO_UPDATE: > > + break; > > + default: > > + pr_err("EUPDATESVN: unknown error %d\n", ret); > > + break; > > + } > > Overall, I think you're right in that "inversion" does make sense, > now that other stuff is better aligned. > > At least when there is spurious error, I think ioctl's should stop > responding and driver should not do anything useful anymore. I.e., > it should go out-of-service. > > I don't think the driver should tear-down, just stop servicing > VM's and responding ioctl's. > > Possibly thish should be also right action for other errors than > "insufficient entropy" but I'm open for comments for this. Or actually actually I take one step back with my suggestions because this really should be a question for which I don't have the definitive answer. The current code works like this: if anything that we don't like happens, we re-iterate. Should some of the "exceptional conditions" have a different recovery or not? BR, Jarkko
> > On Fri, Mar 28, 2025 at 07:50:43PM +0200, Jarkko Sakkinen wrote: > > On Fri, Mar 28, 2025 at 02:57:41PM +0200, Elena Reshetova wrote: > > > SGX architecture introduced a new instruction called EUPDATESVN > > > 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. > > > > > > This patch is based on previous submision by Cathy Zhang > > > https://lore.kernel.org/all/20220520103904.1216-1- > cathy.zhang@intel.com/ > > > > > > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com> > > > --- > > > arch/x86/include/asm/sgx.h | 41 +++++++++++++-------- > > > arch/x86/kernel/cpu/sgx/encls.h | 6 ++++ > > > arch/x86/kernel/cpu/sgx/main.c | 63 > ++++++++++++++++++++++++++++++++- > > > arch/x86/kernel/cpu/sgx/sgx.h | 1 + > > > 4 files changed, 95 insertions(+), 16 deletions(-) > > > > > > diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h > > > index 6a0069761508..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, > > > }; > > > > > > /** > > > @@ -73,6 +76,11 @@ enum sgx_encls_function { > > > * public key does not match > IA32_SGXLEPUBKEYHASH. > > > * %SGX_PAGE_NOT_MODIFIABLE: The EPC page cannot be modified > because it > > > * is in the PENDING or MODIFIED state. > > > + * %SGX_INSUFFICIENT_ENTROPY: Insufficient entropy in RNG. > > > + * %SGX_EPC_NOT_READY: EPC is not ready for SVN update. > > > + * %SGX_NO_UPDATE: EUPDATESVN was successful, but > CPUSVN was not > > > + * updated because current SVN was not newer > than > > > + * CPUSVN. > > > * %SGX_UNMASKED_EVENT: An unmasked event, e.g. > INTR, was received > > > */ > > > enum sgx_return_code { > > > @@ -81,6 +89,9 @@ enum sgx_return_code { > > > SGX_CHILD_PRESENT = 13, > > > SGX_INVALID_EINITTOKEN = 16, > > > SGX_PAGE_NOT_MODIFIABLE = 20, > > > + SGX_INSUFFICIENT_ENTROPY = 29, > > > + SGX_EPC_NOT_READY = 30, > > > + SGX_NO_UPDATE = 31, > > > SGX_UNMASKED_EVENT = 128, > > > }; > > > > > > 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..24563110811d 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,49 @@ 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 retry = 10; > > > + int ret; > > > + > > > + lockdep_assert_held(&sgx_epc_eupdatesvn_lock); > > > + > > > + if (!(cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN)) > > > + return; > > > + > > > + /* Do not execute ENCLS[EUPDATESVN] if running in a VM since > > > + * microcode updates are only meaningful to be applied on the host. > > > + */ > > > + if (boot_cpu_has(X86_FEATURE_HYPERVISOR)) > > > + return; > > > + > > > + do { > > > + ret = __eupdatesvn(); > > > + if (ret != SGX_INSUFFICIENT_ENTROPY) > > > + break; > > > + > > > + } while (--retry); > > > + > > > + switch (ret) { > > > + case 0: > > > + pr_info("EUPDATESVN: success\n"); > > > + break; > > > + case SGX_EPC_NOT_READY: > > > + case SGX_INSUFFICIENT_ENTROPY: > > > + case SGX_EPC_PAGE_CONFLICT: > > > + pr_err("EUPDATESVN: error %d\n", ret); > > > + break; > > > + case SGX_NO_UPDATE: > > > + break; > > > + default: > > > + pr_err("EUPDATESVN: unknown error %d\n", ret); > > > + break; > > > + } > > > > Overall, I think you're right in that "inversion" does make sense, > > now that other stuff is better aligned. > > > > At least when there is spurious error, I think ioctl's should stop > > responding and driver should not do anything useful anymore. I.e., > > it should go out-of-service. > > > > I don't think the driver should tear-down, just stop servicing > > VM's and responding ioctl's. > > > > Possibly thish should be also right action for other errors than > > "insufficient entropy" but I'm open for comments for this. > > Or actually actually I take one step back with my suggestions > because this really should be a question for which I don't have > the definitive answer. > > The current code works like this: if anything that we don't > like happens, we re-iterate. > > Should some of the "exceptional conditions" have a different > recovery or not? None of these exceptional conditions are fatal or present an immediate danger to the system security. So, allowing the re-tries seems logical in this case. In case re-tries also fail, the system admin will have an option of gracefully shutting down all enclaves and doing either a full reboot (if SVN is the only concern) or other necessary actions like taking the physical node out of use, etc. Does this sound reasonable? Best Regards, Elena.
On Mon, Mar 31, 2025 at 07:26:45AM +0000, Reshetova, Elena wrote: > > > > + default: > > > > + pr_err("EUPDATESVN: unknown error %d\n", ret); > > > > + break; > > > > + } > > > > > > Overall, I think you're right in that "inversion" does make sense, > > > now that other stuff is better aligned. > > > > > > At least when there is spurious error, I think ioctl's should stop > > > responding and driver should not do anything useful anymore. I.e., > > > it should go out-of-service. > > > > > > I don't think the driver should tear-down, just stop servicing > > > VM's and responding ioctl's. > > > > > > Possibly thish should be also right action for other errors than > > > "insufficient entropy" but I'm open for comments for this. > > > > Or actually actually I take one step back with my suggestions > > because this really should be a question for which I don't have > > the definitive answer. > > > > The current code works like this: if anything that we don't > > like happens, we re-iterate. > > > > Should some of the "exceptional conditions" have a different > > recovery or not? > > None of these exceptional conditions are fatal or present an > immediate danger to the system security. So, allowing the re-tries > seems logical in this case. In case re-tries also fail, the system > admin will have an option of gracefully shutting down all enclaves > and doing either a full reboot (if SVN is the only concern) or other > necessary actions like taking the physical node out of use, etc. > > Does this sound reasonable? Uknown error I don't think would hold that premise. > > Best Regards, > Elena. > BR, Jarkko >
diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h index 6a0069761508..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, }; /** @@ -73,6 +76,11 @@ enum sgx_encls_function { * public key does not match IA32_SGXLEPUBKEYHASH. * %SGX_PAGE_NOT_MODIFIABLE: The EPC page cannot be modified because it * is in the PENDING or MODIFIED state. + * %SGX_INSUFFICIENT_ENTROPY: Insufficient entropy in RNG. + * %SGX_EPC_NOT_READY: EPC is not ready for SVN update. + * %SGX_NO_UPDATE: EUPDATESVN was successful, but CPUSVN was not + * updated because current SVN was not newer than + * CPUSVN. * %SGX_UNMASKED_EVENT: An unmasked event, e.g. INTR, was received */ enum sgx_return_code { @@ -81,6 +89,9 @@ enum sgx_return_code { SGX_CHILD_PRESENT = 13, SGX_INVALID_EINITTOKEN = 16, SGX_PAGE_NOT_MODIFIABLE = 20, + SGX_INSUFFICIENT_ENTROPY = 29, + SGX_EPC_NOT_READY = 30, + SGX_NO_UPDATE = 31, SGX_UNMASKED_EVENT = 128, }; 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..24563110811d 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,49 @@ 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 retry = 10; + int ret; + + lockdep_assert_held(&sgx_epc_eupdatesvn_lock); + + if (!(cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN)) + return; + + /* Do not execute ENCLS[EUPDATESVN] if running in a VM since + * microcode updates are only meaningful to be applied on the host. + */ + if (boot_cpu_has(X86_FEATURE_HYPERVISOR)) + return; + + do { + ret = __eupdatesvn(); + if (ret != SGX_INSUFFICIENT_ENTROPY) + break; + + } while (--retry); + + switch (ret) { + case 0: + pr_info("EUPDATESVN: success\n"); + break; + case SGX_EPC_NOT_READY: + case SGX_INSUFFICIENT_ENTROPY: + case SGX_EPC_PAGE_CONFLICT: + pr_err("EUPDATESVN: error %d\n", ret); + break; + case SGX_NO_UPDATE: + break; + default: + pr_err("EUPDATESVN: unknown error %d\n", ret); + break; + } + +} diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h index d2dad21259a8..12ae49e78959 100644 --- a/arch/x86/kernel/cpu/sgx/sgx.h +++ b/arch/x86/kernel/cpu/sgx/sgx.h @@ -103,5 +103,6 @@ static inline int __init sgx_vepc_init(void) #endif void sgx_update_lepubkeyhash(u64 *lepubkeyhash); +void sgx_updatesvn(void); #endif /* _X86_SGX_H */
SGX architecture introduced a new instruction called EUPDATESVN 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. This patch is based on previous submision by Cathy Zhang https://lore.kernel.org/all/20220520103904.1216-1-cathy.zhang@intel.com/ Signed-off-by: Elena Reshetova <elena.reshetova@intel.com> --- arch/x86/include/asm/sgx.h | 41 +++++++++++++-------- arch/x86/kernel/cpu/sgx/encls.h | 6 ++++ arch/x86/kernel/cpu/sgx/main.c | 63 ++++++++++++++++++++++++++++++++- arch/x86/kernel/cpu/sgx/sgx.h | 1 + 4 files changed, 95 insertions(+), 16 deletions(-)