| Message ID | 20250415115213.291449-3-elena.reshetova@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Enable automatic SVN updates for SGX enclaves | expand |
On Tue, 2025-04-15 at 14:51 +0300, Elena Reshetova wrote: > SGX architecture introduced a new instruction called EUPDATESVN "a new ENCLS leaf function 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/ When Jarkko suggested to use "Link" here: https://lore.kernel.org/lkml/Z983ZaTaWNqFUpYS@kernel.org/ I think he meant something like below: This patch is based on ... [1] Link: https://lore.kernel.org/all/20220520103904.1216-1-cathy.zhang@intel.com/ [1] > > 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 | 82 ++++++++++++++++++++++++++++++++- > arch/x86/kernel/cpu/sgx/sgx.h | 1 + > 4 files changed, 114 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) I am not sure whether this should be a new X86_FEATURE bit, just like other SGX bits: X86_FEATURE_SGX1 X86_FEATURE_SGX2 X86_FEATURE_SGX_EDECCSSA .. reported in CPUID.0x12.0x0:EAX. But this is never going to be exposed to VMs, i.e., KVM should never need to use it, so perhaps fine to just put it here. [...] > > +/* This lock is held to prevent new EPC pages from being created > + * during the execution of ENCLS[EUPDATESVN]. > + */ /* * This lock ... * ... */ > +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)) { > + spin_lock(&sgx_epc_eupdatesvn_lock); > + /* > + * Only call sgx_updatesvn() once the first enclave's > + * page is allocated from EPC > + */ The VMs can pre-populate EPC w/o having any enclave being created inside. How about just: /* * Make sure SVN is up-to-date before any EPC page is * allocated for any enclave. */ > + if (atomic_long_read(&sgx_nr_used_pages) == 0) { > + ret = sgx_updatesvn(); > + if (!ret) { > + /* > + * sgx_updatesvn() returned unknown error, smth > + * must be broken, do not allocate a page from EPC > + */ Strictly speaking, sgx_updatesvn() returns true or false, but not "unknown error". Reading below, it could also fail due to running out of entropy, which is still legally possible to happen IMHO. Maybe just: /* * Updating SVN failed. SGX might be broken, * or running out of entropy happened. Do not * allocate EPC page since it is not safe to use * SGX anymore if it was the former. If it was * due to running out of entropy, the further * call of EPC allocation will try * sgx_updatesvn() again. */ > + spin_unlock(&sgx_epc_eupdatesvn_lock); > + spin_unlock(&node->lock); > + return NULL; > + } > + } > + atomic_long_inc(&sgx_nr_used_pages); > + spin_unlock(&sgx_epc_eupdatesvn_lock); > + } > + > page = list_first_entry(&node->free_page_list, struct sgx_epc_page, list); > list_del_init(&page->list); > page->flags = 0; > > spin_unlock(&node->lock); > - atomic_long_inc(&sgx_nr_used_pages); > > return page; > } > @@ -970,3 +997,56 @@ 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 Not sure how to interpret "EPC is ready". Assume it means "EPC is empty", in which case should we just say so? It is consistent with what said in the changelog anyway. > + * loaded microcode update SVN and generate new cryptographic assets. > + * > + * Return: > + * True: success or EUPDATESVN can be safely retried next time > + * False: Unexpected error occurred Hmm.. IIUC it could fail with running out of entropy but this is still legally possible to happen. And it is safe to retry. > + */ > +bool sgx_updatesvn(void) > +{ > + int retry = 10; "10" is a bit out of blue. We can use RDRAND_RETRY_LOOPS in <asm/archrandom.h> instead: #define RDRAND_RETRY_LOOPS 10 > + int ret; > + > + lockdep_assert_held(&sgx_epc_eupdatesvn_lock); > + > + if (!(cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN)) > + return true; > + > + /* > + * 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 true; > + > + do { > + ret = __eupdatesvn(); > + if (ret != SGX_INSUFFICIENT_ENTROPY) > + break; > + The blank line here is not needed. > + } 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: > + ENCLS_WARN(ret, "EUPDATESVN"); > + break; I don't think we should use ENCLS_WARN() for SGX_INSUFFICIENT_ENTROPY, since IIUC it is still legally possible to happen after the above retry. Also, it doesn't seem SGX_EPC_NOT_READY and SGX_EPC_PAGE_CONFLICT are needed since IIUC the only possible error is out of entropy. > + case SGX_NO_UPDATE: > + break; > + default: > + ENCLS_WARN(ret, "EUPDATESVN"); > + return false; > + } > + > + return true; > + Please remove this blank line. > +} > diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h > index d2dad21259a8..d7d631c973d0 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); > +bool sgx_updatesvn(void); Seems sgx_updatesvn() is only called by __sgx_alloc_epc_page_from_node(). Can it be made static?
> On Tue, 2025-04-15 at 14:51 +0300, Elena Reshetova wrote: > > SGX architecture introduced a new instruction called EUPDATESVN > > "a new ENCLS leaf function EUPDATESVN"? Yes, you are right, better wording, will fix. > > > 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/ > > When Jarkko suggested to use "Link" here: > > https://lore.kernel.org/lkml/Z983ZaTaWNqFUpYS@kernel.org/ > > I think he meant something like below: > > This patch is based on ... [1] > > Link: https://lore.kernel.org/all/20220520103904.1216-1- > cathy.zhang@intel.com/ > [1] Oh, I see, I didn’t understand this. Can fix in the next version. > > > > > 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 | 82 > ++++++++++++++++++++++++++++++++- > > arch/x86/kernel/cpu/sgx/sgx.h | 1 + > > 4 files changed, 114 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) > > I am not sure whether this should be a new X86_FEATURE bit, just like other > SGX > bits: > > X86_FEATURE_SGX1 > X86_FEATURE_SGX2 > X86_FEATURE_SGX_EDECCSSA > > .. reported in CPUID.0x12.0x0:EAX. > > But this is never going to be exposed to VMs, i.e., KVM should never need to > use > it, so perhaps fine to just put it here. I am fine either way. I can change this if people consider it is a better fit. > > [...] > > > > > > +/* This lock is held to prevent new EPC pages from being created > > + * during the execution of ENCLS[EUPDATESVN]. > > + */ > > /* > * This lock ... > * ... > */ Thank you for catching this! I thought that I fixed all comments to this Format but obvious I missed this one. Sad that checkpatch doesn’t complain on this ( > > > +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)) { > > + spin_lock(&sgx_epc_eupdatesvn_lock); > > + /* > > + * Only call sgx_updatesvn() once the first enclave's > > + * page is allocated from EPC > > + */ > > The VMs can pre-populate EPC w/o having any enclave being created inside. > How > about just: > > /* > * Make sure SVN is up-to-date before any EPC page is > * allocated for any enclave. > */ > Right, agree with this wording change, good point! > > + if (atomic_long_read(&sgx_nr_used_pages) == 0) { > > + ret = sgx_updatesvn(); > > + if (!ret) { > > + /* > > + * sgx_updatesvn() returned unknown error, > smth > > + * must be broken, do not allocate a page > from EPC > > + */ > > Strictly speaking, sgx_updatesvn() returns true or false, but not "unknown > error". Yes, will fix the wording. > > Reading below, it could also fail due to running out of entropy, which is still > legally possible to happen IMHO. Actually, no in this case, we only return false from sgx_updatesvn in case unknown error happens as agreed previously. In case we run out of entropy it should be safe to retry later and we dont prevent per current code EPC page allocation. > > Maybe just: > /* > * Updating SVN failed. SGX might be broken, > * or running out of entropy happened. Do > not > * allocate EPC page since it is not safe to > use > * SGX anymore if it was the former. If it was > * due to running out of entropy, the further > * call of EPC allocation will try > * sgx_updatesvn() again. > */ I agree with this except that the current code doesn’t prevent EPC allocation on any other error return than unknown error. The question is whenever we want to change the behaviour to require it? > > > + spin_unlock(&sgx_epc_eupdatesvn_lock); > > + spin_unlock(&node->lock); > > + return NULL; > > + } > > + } > > + atomic_long_inc(&sgx_nr_used_pages); > > + spin_unlock(&sgx_epc_eupdatesvn_lock); > > + } > > + > > page = list_first_entry(&node->free_page_list, struct sgx_epc_page, > list); > > list_del_init(&page->list); > > page->flags = 0; > > > > spin_unlock(&node->lock); > > - atomic_long_inc(&sgx_nr_used_pages); > > > > return page; > > } > > @@ -970,3 +997,56 @@ 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 > > Not sure how to interpret "EPC is ready". Assume it means "EPC is empty", in > which case should we just say so? Yes, correct, it means EPC is empty, will fix. > > It is consistent with what said in the changelog anyway. > > > + * loaded microcode update SVN and generate new cryptographic assets. > > + * > > + * Return: > > + * True: success or EUPDATESVN can be safely retried next time > > + * False: Unexpected error occurred > > Hmm.. IIUC it could fail with running out of entropy but this is still legally > possible to happen. And it is safe to retry. Yes, in this case we get back SGX_INSUFFICIENT_ENTROPY currently we return "true" here and do not prevent EPC allocations of the page in this case, which means we will start populate EPC and can next time retry only when EPC is empty again. > > > + */ > > +bool sgx_updatesvn(void) > > +{ > > + int retry = 10; > > "10" is a bit out of blue. > > We can use RDRAND_RETRY_LOOPS in <asm/archrandom.h> instead: > > #define RDRAND_RETRY_LOOPS 10 I can change it to this value. > > > + int ret; > > + > > + lockdep_assert_held(&sgx_epc_eupdatesvn_lock); > > + > > + if (!(cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN)) > > + return true; > > + > > + /* > > + * 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 true; > > + > > + do { > > + ret = __eupdatesvn(); > > + if (ret != SGX_INSUFFICIENT_ENTROPY) > > + break; > > + > > The blank line here is not needed. Will fix > > > + } 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: > > + ENCLS_WARN(ret, "EUPDATESVN"); > > + break; > > I don't think we should use ENCLS_WARN() for SGX_INSUFFICIENT_ENTROPY, > since > IIUC it is still legally possible to happen after the above retry. > > Also, it doesn't seem SGX_EPC_NOT_READY and SGX_EPC_PAGE_CONFLICT > are needed > since IIUC the only possible error is out of entropy. Well, in case we have a kernel bug, and we think EPC is empty and it is safe to execute EUPDATESVN, while it is not the case, we can still get back the SGX_EPC_NOT_READY and SGX_EPC_PAGE_CONFLICT from HW, right? Which means we probably should warn about such buggy cases. And maybe we should also prevent page allocation from EPC in this case also similarly as for unknown error? > > > + case SGX_NO_UPDATE: > > + break; > > + default: > > + ENCLS_WARN(ret, "EUPDATESVN"); > > + return false; > > + } > > + > > + return true; > > + > > Please remove this blank line. Sure, thanks for catching! > > > +} > > diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h > > index d2dad21259a8..d7d631c973d0 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); > > +bool sgx_updatesvn(void); > > Seems sgx_updatesvn() is only called by __sgx_alloc_epc_page_from_node(). > Can > it be made static? Yes, will do.
On Tue, Apr 15, 2025 at 02:51:22PM +0300, 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. "Ice Lake introduced ENCLS[EUPDATESVN], which allows to to update the microcode version for an online system" ? Now you are saying it allows updating SVN which is null information. > > 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 | 82 ++++++++++++++++++++++++++++++++- > arch/x86/kernel/cpu/sgx/sgx.h | 1 + > 4 files changed, 114 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..c21f4f6226b0 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; > > @@ -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)) { > + 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) { > + ret = sgx_updatesvn(); > + if (!ret) { > + /* > + * sgx_updatesvn() returned unknown error, smth > + * must be broken, do not allocate a page from EPC > + */ > + spin_unlock(&sgx_epc_eupdatesvn_lock); > + spin_unlock(&node->lock); > + return NULL; > + } > + } > + atomic_long_inc(&sgx_nr_used_pages); > + spin_unlock(&sgx_epc_eupdatesvn_lock); > + } > + > page = list_first_entry(&node->free_page_list, struct sgx_epc_page, list); > list_del_init(&page->list); > page->flags = 0; > > spin_unlock(&node->lock); > - atomic_long_inc(&sgx_nr_used_pages); > > return page; > } > @@ -970,3 +997,56 @@ 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. > + * > + * Return: > + * True: success or EUPDATESVN can be safely retried next time > + * False: Unexpected error occurred > + */ > +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; > + > + /* > + * 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 true; > + > + 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: > + ENCLS_WARN(ret, "EUPDATESVN"); > + break; > + case SGX_NO_UPDATE: > + break; > + default: > + ENCLS_WARN(ret, "EUPDATESVN"); > + return false; I'm lost why only ENCLS_WARN() is practiced here. For spurious error code at minimum EPC allocation should be turned off really. Something is likely working incorrectly in the driver. > + } > + > + return true; > + > +} > diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h > index d2dad21259a8..d7d631c973d0 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); > +bool sgx_updatesvn(void); > > #endif /* _X86_SGX_H */ > -- > 2.45.2 > BR, Jarkko
> > > > > Reading below, it could also fail due to running out of entropy, which is still > > legally possible to happen IMHO. > > Actually, no in this case, we only return false from sgx_updatesvn in case unknown > error happens as agreed previously. In case we run out of entropy it should be safe > to retry later and we dont prevent per current code EPC page allocation. > > > > > Maybe just: > > /* > > * Updating SVN failed. SGX might be broken, > > * or running out of entropy happened. Do > > not > > * allocate EPC page since it is not safe to > > use > > * SGX anymore if it was the former. If it was > > * due to running out of entropy, the further > > * call of EPC allocation will try > > * sgx_updatesvn() again. > > */ > > I agree with this except that the current code doesn’t prevent EPC allocation on any > other error return than unknown error. The question is whenever we want to > change the behaviour to require it? [...] > > > + * Return: > > > + * True: success or EUPDATESVN can be safely retried next time > > > + * False: Unexpected error occurred > > > > Hmm.. IIUC it could fail with running out of entropy but this is still legally > > possible to happen. And it is safe to retry. > > Yes, in this case we get back SGX_INSUFFICIENT_ENTROPY currently we > return "true" here and do not prevent EPC allocations of the page in this > case, which means we will start populate EPC and can next time retry only > when EPC is empty again. [...] > > > + switch (ret) { > > > + case 0: > > > + pr_info("EUPDATESVN: success\n"); > > > + break; > > > + case SGX_EPC_NOT_READY: > > > + case SGX_INSUFFICIENT_ENTROPY: > > > + case SGX_EPC_PAGE_CONFLICT: > > > + ENCLS_WARN(ret, "EUPDATESVN"); > > > + break; > > > > I don't think we should use ENCLS_WARN() for SGX_INSUFFICIENT_ENTROPY, > > since > > IIUC it is still legally possible to happen after the above retry. > > > > Also, it doesn't seem SGX_EPC_NOT_READY and SGX_EPC_PAGE_CONFLICT > > are needed > > since IIUC the only possible error is out of entropy. > > Well, in case we have a kernel bug, and we think EPC is empty and it is safe > to execute EUPDATESVN, while it is not the case, we can still get back the > SGX_EPC_NOT_READY and SGX_EPC_PAGE_CONFLICT from HW, right? Right, but as you said, in case we have a kernel bug. Which means it is not expected and we should just use ENCLS_WARN() for them. IMHO we can even add WARN_ON_ONCE(atomic_long_read(&sgx_nr_used_pages) != 0); to sgx_updatesvn(), e.g., right after lockdep_assert_held(&sgx_epc_eupdatesvn_lock); to assert sgx_updatesvn() is called when EPC is empty, thus SGX_EPC_NOT_READY and SGX_EPC_PAGE_CONFLICT are not possible to happen. > Which means we probably should warn about such buggy cases. Yes. > And maybe we should also prevent page allocation from EPC in this case also > similarly as for unknown error? Yes. I don't see any reason we should continue to allow SGX to work in case of SGX_EPC_NOT_READY or SGX_EPC_PAGE_CONFLICT. IIUC, we also agreed in the last round discussion that we should: "I guess the best action would be make sgx_alloc_epc_page() return consistently -ENOMEM, if the unexpected happens." SGX_EPC_NOT_READY and SGX_EPC_PAGE_CONFLICT are indeed unexpected to me. So my suggestion would be: I think the sgx_updatesvn() should just return true when EUPDATESVN returns 0 or SGX_NO_UPDATE, and return false for all other error codes. And it should ENCLS_WARN() for all other error codes, except SGX_INSUFFICIENT_ENTROPY because it can still legally happen. Something like: do { ret = __eupdatesvn(); if (ret != SGX_INSUFFICIENT_ENTROPY) break; } while (--retry); if (!ret || ret == SGX_NO_UPDATE) { /* * SVN successfully updated, or it was already up-to-date. * Let users know when the update was successful. */ if (!ret) pr_info("SVN updated successfully\n"); return true; } /* * EUPDATESVN was called when EPC is empty, all other error * codes are unexcepted except running out of entropy. */ if (ret != SGX_INSUFFICIENT_ENTROPY) ENCLS_WARN(ret, "EUPDATESVN"); return false; In __sgx_alloc_epc_page_from_node(), it should fail to allocate EPC page and return -ENOMEM when sgx_updatesvn() returns false. We should only allow EPC to be allocated when we know the SVN is already up-to-date. Any further call of EPC allocation will trigger sgx_updatesvn() again. If it was failed due to unexpected error, then it should continue to fail, guaranteeing "sgx_alloc_epc_page() return consistently -ENOMEM, if the unexpected happens". If it was failed due to running out of entropy, it then may fail again, or it will just succeed and then SGX can continue to work.
On Thu, Apr 17, 2025, Kai Huang wrote: > I think the sgx_updatesvn() should just return true when EUPDATESVN returns 0 or > SGX_NO_UPDATE, and return false for all other error codes. And it should > ENCLS_WARN() for all other error codes, except SGX_INSUFFICIENT_ENTROPY because > it can still legally happen. > > Something like: > > do { > ret = __eupdatesvn(); > if (ret != SGX_INSUFFICIENT_ENTROPY) > break; > } while (--retry); This can be: do { ret = __eupdatesvn(); } while (ret == SGX_INSUFFICIENT_ENTROPY && --retry) To make it super obvious that retry is only relevant to lack of entropy. > if (!ret || ret == SGX_NO_UPDATE) { > /* > * SVN successfully updated, or it was already up-to-date. > * Let users know when the update was successful. > */ > if (!ret) > pr_info("SVN updated successfully\n"); > return true; Returning true/false is confusing since the vast majority of the SGX code uses '0' for success. A lot of cleverness went into splicing SGX's error codes into the kernel's -ernno; it would be a shame to ignore that :-) E.g. this looks wrong at first (and second glance) ret = sgx_updatesvn(); if (!ret) { /* * sgx_updatesvn() returned unknown error, smth * must be broken, do not allocate a page from EPC */ spin_unlock(&sgx_epc_eupdatesvn_lock); spin_unlock(&node->lock); return NULL; } > } > > /* > * EUPDATESVN was called when EPC is empty, all other error > * codes are unexcepted except running out of entropy. > */ > if (ret != SGX_INSUFFICIENT_ENTROPY) > ENCLS_WARN(ret, "EUPDATESVN"); > > return false; > > > In __sgx_alloc_epc_page_from_node(), it should fail to allocate EPC page and > return -ENOMEM when sgx_updatesvn() returns false. We should only allow EPC to No, it should return a meaningful error code, not -ENOMEM. And if that's the behavior you want, then __sgx_alloc_epc_page() should be updated to bail immediately. The current code assuming -ENOMEM is the only failure scenario: do { page = __sgx_alloc_epc_page_from_node(nid); if (page) return page; nid = next_node_in(nid, sgx_numa_mask); } while (nid != nid_start); That should be something like: do { page = __sgx_alloc_epc_page_from_node(nid); if (!IS_ERR(page) || PTR_ERR(page) != -ENOMEM) return page; nid = next_node_in(nid, sgx_numa_mask); } while (nid != nid_start); > be allocated when we know the SVN is already up-to-date. > > Any further call of EPC allocation will trigger sgx_updatesvn() again. If it > was failed due to unexpected error, then it should continue to fail, > guaranteeing "sgx_alloc_epc_page() return consistently -ENOMEM, if the > unexpected happens". If it was failed due to running out of entropy, it then > may fail again, or it will just succeed and then SGX can continue to work. Side topic, the function comment for __sgx_alloc_epc_page() is stale/wrong. It returns ERR_PTR(-ENOMEM), not NULL, on failure. /** * __sgx_alloc_epc_page() - Allocate an EPC page * * Iterate through NUMA nodes and reserve ia free EPC page to the caller. Start * from the NUMA node, where the caller is executing. * * Return: * - an EPC page: A borrowed EPC pages were available. * - NULL: Out of EPC pages. */
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()? Then you're hooking a relative slow path (I assume EUPDATESVN is not fast), 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. > +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. > + > + /* > + * 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. 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
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..c21f4f6226b0 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; @@ -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)) { + 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) { + ret = sgx_updatesvn(); + if (!ret) { + /* + * sgx_updatesvn() returned unknown error, smth + * must be broken, do not allocate a page from EPC + */ + spin_unlock(&sgx_epc_eupdatesvn_lock); + spin_unlock(&node->lock); + return NULL; + } + } + atomic_long_inc(&sgx_nr_used_pages); + spin_unlock(&sgx_epc_eupdatesvn_lock); + } + page = list_first_entry(&node->free_page_list, struct sgx_epc_page, list); list_del_init(&page->list); page->flags = 0; spin_unlock(&node->lock); - atomic_long_inc(&sgx_nr_used_pages); return page; } @@ -970,3 +997,56 @@ 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. + * + * Return: + * True: success or EUPDATESVN can be safely retried next time + * False: Unexpected error occurred + */ +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; + + /* + * 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 true; + + 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: + ENCLS_WARN(ret, "EUPDATESVN"); + break; + case SGX_NO_UPDATE: + break; + default: + ENCLS_WARN(ret, "EUPDATESVN"); + return false; + } + + return true; + +} diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h index d2dad21259a8..d7d631c973d0 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); +bool 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 | 82 ++++++++++++++++++++++++++++++++- arch/x86/kernel/cpu/sgx/sgx.h | 1 + 4 files changed, 114 insertions(+), 16 deletions(-)