| Message ID | 20220315010300.10199-10-cathy.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support microcode updates affecting SGX | expand |
On 2022-03-15 02:02, Cathy Zhang wrote: > EUPDATESVN is the SGX instruction which allows enclave attestation > to include information about updated microcode without a reboot. > > Microcode updates which affect SGX require two phases: > > 1. Do the main microcode update > 2. Make the new CPUSVN available for enclave attestation via > EUPDATESVN. > > Before a EUPDATESVN can succeed, all enclave pages (EPC) must be > marked as unused in the SGX metadata (EPCM). This operation destroys > all preexisting SGX enclave data and metadata. This is by design and > mitigates the impact of vulnerabilities that may have compromised > enclaves or the SGX hardware itself prior to the update. > > Signed-off-by: Cathy Zhang <cathy.zhang@intel.com> > > --- > Changes since v1: > - Remove the sysfs file svnupdate. (Thomas Gleixner, Dave Hansen) > - Let late microcode load path call ENCLS[EUPDATESVN] procedure > directly. (Borislav Petkov) > - Redefine update_cpusvn_intel() to return void instead of int. > --- > arch/x86/include/asm/microcode.h | 5 +++++ > arch/x86/include/asm/sgx.h | 5 +++++ > arch/x86/kernel/cpu/common.c | 9 +++++++++ > arch/x86/kernel/cpu/sgx/main.c | 12 ++++++++++++ > 4 files changed, 31 insertions(+) > > diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h > index d6bfdfb0f0af..1ba66b9fe198 100644 > --- a/arch/x86/include/asm/microcode.h > +++ b/arch/x86/include/asm/microcode.h > @@ -3,6 +3,7 @@ > #define _ASM_X86_MICROCODE_H > > #include <asm/cpu.h> > +#include <asm/sgx.h> > #include <linux/earlycpio.h> > #include <linux/initrd.h> > > @@ -137,4 +138,8 @@ static inline void load_ucode_ap(void) { } > static inline void reload_early_microcode(void) { } > #endif > > +#ifndef update_cpusvn_intel > +static inline void update_cpusvn_intel(void) {} > +#endif > + > #endif /* _ASM_X86_MICROCODE_H */ > diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h > index d5942d0848ec..d0f2832a57b3 100644 > --- a/arch/x86/include/asm/sgx.h > +++ b/arch/x86/include/asm/sgx.h > @@ -412,4 +412,9 @@ int sgx_virt_einit(void __user *sigstruct, void __user *token, > int sgx_set_attribute(unsigned long *allowed_attributes, > unsigned int attribute_fd); > > +#ifdef CONFIG_X86_SGX > +void update_cpusvn_intel(void); > +#define update_cpusvn_intel update_cpusvn_intel > +#endif > + > #endif /* _ASM_X86_SGX_H */ > diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c > index 64deb7727d00..514e621f04c3 100644 > --- a/arch/x86/kernel/cpu/common.c > +++ b/arch/x86/kernel/cpu/common.c > @@ -59,6 +59,7 @@ > #include <asm/cpu_device_id.h> > #include <asm/uv/uv.h> > #include <asm/sigframe.h> > +#include <asm/sgx.h> > > #include "cpu.h" > > @@ -2165,6 +2166,14 @@ void microcode_check(void) > > perf_check_microcode(); > > + /* > + * SGX related microcode update requires EUPDATESVN to update CPUSVN, which > + * will destroy all enclaves to ensure EPC is not in use. If SGX is configured > + * and EUPDATESVN is supported, call the EUPDATESVN procecure. > + */ > + if (IS_ENABLED(CONFIG_X86_SGX) && (cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN)) > + update_cpusvn_intel(); > + > /* Reload CPUID max function as it might've changed. */ > info.cpuid_level = cpuid_eax(0); > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > index 123818fa2386..d86745d8cc7d 100644 > --- a/arch/x86/kernel/cpu/sgx/main.c > +++ b/arch/x86/kernel/cpu/sgx/main.c > @@ -1380,3 +1380,15 @@ static int sgx_updatesvn(void) > > return ret; > } > + > +void update_cpusvn_intel(void) > +{ > + sgx_lock_epc(); > + if (sgx_zap_pages()) Doing this automatically and unconditionally during a microcode update seems undesirable. This requires the userland tooling that is coordinating the microcode update to be aware of any SGX enclaves that are running and possibly coordinate sequencing with the processes containing those enclaves. This coupling does not exist today. -- Jethro Beekman | Fortanix > + goto out; > + > + sgx_updatesvn(); > + > +out: > + sgx_unlock_epc(); > +}
On 2022-03-16 10:46, Jethro Beekman wrote: > On 2022-03-15 02:02, Cathy Zhang wrote: >> EUPDATESVN is the SGX instruction which allows enclave attestation >> to include information about updated microcode without a reboot. >> >> Microcode updates which affect SGX require two phases: >> >> 1. Do the main microcode update >> 2. Make the new CPUSVN available for enclave attestation via >> EUPDATESVN. >> >> Before a EUPDATESVN can succeed, all enclave pages (EPC) must be >> marked as unused in the SGX metadata (EPCM). This operation destroys >> all preexisting SGX enclave data and metadata. This is by design and >> mitigates the impact of vulnerabilities that may have compromised >> enclaves or the SGX hardware itself prior to the update. >> >> Signed-off-by: Cathy Zhang <cathy.zhang@intel.com> >> >> --- >> Changes since v1: >> - Remove the sysfs file svnupdate. (Thomas Gleixner, Dave Hansen) >> - Let late microcode load path call ENCLS[EUPDATESVN] procedure >> directly. (Borislav Petkov) >> - Redefine update_cpusvn_intel() to return void instead of int. >> --- >> arch/x86/include/asm/microcode.h | 5 +++++ >> arch/x86/include/asm/sgx.h | 5 +++++ >> arch/x86/kernel/cpu/common.c | 9 +++++++++ >> arch/x86/kernel/cpu/sgx/main.c | 12 ++++++++++++ >> 4 files changed, 31 insertions(+) >> >> diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h >> index d6bfdfb0f0af..1ba66b9fe198 100644 >> --- a/arch/x86/include/asm/microcode.h >> +++ b/arch/x86/include/asm/microcode.h >> @@ -3,6 +3,7 @@ >> #define _ASM_X86_MICROCODE_H >> >> #include <asm/cpu.h> >> +#include <asm/sgx.h> >> #include <linux/earlycpio.h> >> #include <linux/initrd.h> >> >> @@ -137,4 +138,8 @@ static inline void load_ucode_ap(void) { } >> static inline void reload_early_microcode(void) { } >> #endif >> >> +#ifndef update_cpusvn_intel >> +static inline void update_cpusvn_intel(void) {} >> +#endif >> + >> #endif /* _ASM_X86_MICROCODE_H */ >> diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h >> index d5942d0848ec..d0f2832a57b3 100644 >> --- a/arch/x86/include/asm/sgx.h >> +++ b/arch/x86/include/asm/sgx.h >> @@ -412,4 +412,9 @@ int sgx_virt_einit(void __user *sigstruct, void __user *token, >> int sgx_set_attribute(unsigned long *allowed_attributes, >> unsigned int attribute_fd); >> >> +#ifdef CONFIG_X86_SGX >> +void update_cpusvn_intel(void); >> +#define update_cpusvn_intel update_cpusvn_intel >> +#endif >> + >> #endif /* _ASM_X86_SGX_H */ >> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c >> index 64deb7727d00..514e621f04c3 100644 >> --- a/arch/x86/kernel/cpu/common.c >> +++ b/arch/x86/kernel/cpu/common.c >> @@ -59,6 +59,7 @@ >> #include <asm/cpu_device_id.h> >> #include <asm/uv/uv.h> >> #include <asm/sigframe.h> >> +#include <asm/sgx.h> >> >> #include "cpu.h" >> >> @@ -2165,6 +2166,14 @@ void microcode_check(void) >> >> perf_check_microcode(); >> >> + /* >> + * SGX related microcode update requires EUPDATESVN to update CPUSVN, which >> + * will destroy all enclaves to ensure EPC is not in use. If SGX is configured >> + * and EUPDATESVN is supported, call the EUPDATESVN procecure. >> + */ >> + if (IS_ENABLED(CONFIG_X86_SGX) && (cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN)) >> + update_cpusvn_intel(); >> + >> /* Reload CPUID max function as it might've changed. */ >> info.cpuid_level = cpuid_eax(0); >> >> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c >> index 123818fa2386..d86745d8cc7d 100644 >> --- a/arch/x86/kernel/cpu/sgx/main.c >> +++ b/arch/x86/kernel/cpu/sgx/main.c >> @@ -1380,3 +1380,15 @@ static int sgx_updatesvn(void) >> >> return ret; >> } >> + >> +void update_cpusvn_intel(void) >> +{ >> + sgx_lock_epc(); >> + if (sgx_zap_pages()) > > Doing this automatically and unconditionally during a microcode update seems undesirable. This requires the userland tooling that is coordinating the microcode update to be aware of any SGX enclaves that are running and possibly coordinate sequencing with the processes containing those enclaves. This coupling does not exist today. Also, a microcode update may not affect SGX security at all and doing the EUPDATESVN procedure may not be required for this particular update. This case is called out specifically in the EUPDATESVN documentation. -- Jethro Beekman | Fortanix
On 3/16/22 02:46, Jethro Beekman wrote: >> +void update_cpusvn_intel(void) +{ + sgx_lock_epc(); + if >> (sgx_zap_pages()) > Doing this automatically and unconditionally during a microcode > update seems undesirable. This requires the userland tooling that is > coordinating the microcode update to be aware of any SGX enclaves > that are running and possibly coordinate sequencing with the > processes containing those enclaves. This coupling does not exist > today. "Today" in what? If a microcode update changes SGX behavior and bumps CPUSVN, it's fundamentally incompatible with letting enclaves continue to run. They might as well be killed. But, seriously, if you can't handle enclaves being killed every few months, don't use SGX. The architecture doesn't allow data to be persistent like normal x86. It's fundamental to the architecture. You can also think of this as a shiny new SGX *testing* feature: one that keeps enclave owners from becoming complacent and forgetting about what the SGX architecture provides.
On 3/16/22 03:24, Jethro Beekman wrote: >> Doing this automatically and unconditionally during a microcode >> update seems undesirable. This requires the userland tooling that >> is coordinating the microcode update to be aware of any SGX >> enclaves that are running and possibly coordinate sequencing with >> the processes containing those enclaves. This coupling does not >> exist today. > Also, a microcode update may not affect SGX security at all and doing > the EUPDATESVN procedure may not be required for this particular > update. This case is called out specifically in the EUPDATESVN > documentation. I don't think Intel provides the metadata for the kernel to tell if an update requires an EUPDATESVN procedure or not. If this is inconvenient for you, I'd suggest reporting this to the folks at Intel who can fix it. It doesn't seem like something which they are motivated to fix.
On 2022-03-16 16:42, Dave Hansen wrote: > On 3/16/22 02:46, Jethro Beekman wrote: >>> +void update_cpusvn_intel(void) +{ + sgx_lock_epc(); + if >>> (sgx_zap_pages()) >> Doing this automatically and unconditionally during a microcode >> update seems undesirable. This requires the userland tooling that is >> coordinating the microcode update to be aware of any SGX enclaves >> that are running and possibly coordinate sequencing with the >> processes containing those enclaves. This coupling does not exist >> today. > > "Today" in what? In distros. > If a microcode update changes SGX behavior and bumps CPUSVN, it's > fundamentally incompatible with letting enclaves continue to run. They > might as well be killed. It's not "fundamentally incompatible". It works fine today and will continue to work fine in CPUs that have EUPDATESVN support. Yes your enclaves are probably vulnerable to some attacks, but people run vulnerable software intentionally all the time. > But, seriously, if you can't handle enclaves being killed every few > months, don't use SGX. The architecture doesn't allow data to be I don't think anyone is making a claim that there are enclaves that wouldn't be able handle this? Being able to deal with something is not the same as wanting to deal with something. My laptop can handle running out of battery in the middle of writing some new code. That doesn't mean I want my laptop to arbitrarily turn off at uncontrollable times? > persistent like normal x86. It's fundamental to the architecture. You > can also think of this as a shiny new SGX *testing* feature: one that > keeps enclave owners from becoming complacent and forgetting about what > the SGX architecture provides. Great idea, why not expand on this and just randomly call EREMOVE at timed intervals? -- Jethro Beekman | Fortanix
On 2022-03-16 16:47, Dave Hansen wrote: > On 3/16/22 03:24, Jethro Beekman wrote: >>> Doing this automatically and unconditionally during a microcode >>> update seems undesirable. This requires the userland tooling that >>> is coordinating the microcode update to be aware of any SGX >>> enclaves that are running and possibly coordinate sequencing with >>> the processes containing those enclaves. This coupling does not >>> exist today. >> Also, a microcode update may not affect SGX security at all and doing >> the EUPDATESVN procedure may not be required for this particular >> update. This case is called out specifically in the EUPDATESVN >> documentation. > > I don't think Intel provides the metadata for the kernel to tell if an > update requires an EUPDATESVN procedure or not. If this is inconvenient > for you, I'd suggest reporting this to the folks at Intel who can fix > it. It doesn't seem like something which they are motivated to fix. Whether or not metadata is currently made available is orthogonal to creating a mechanism by which userspace can indidate that a particular microcode update shouldn't trigger the EUPDATESVN procedure. -- Jethro Beekman | Fortanix
Hi Jethro, Thanks for helping review! > -----Original Message----- > From: Jethro Beekman <jethro@fortanix.com> > Sent: Friday, March 18, 2022 5:06 PM > To: Hansen, Dave <dave.hansen@intel.com>; Zhang, Cathy > <cathy.zhang@intel.com>; linux-sgx@vger.kernel.org; linux-Hi > kernel@vger.kernel.org > Cc: Raj, Ashok <ashok.raj@intel.com> > Subject: Re: [RFC PATCH v2 09/10] x86/cpu: Call ENCLS[EUPDATESVN] > procedure in microcode update > > On 2022-03-16 16:47, Dave Hansen wrote: > > On 3/16/22 03:24, Jethro Beekman wrote: > >>> Doing this automatically and unconditionally during a microcode > >>> update seems undesirable. This requires the userland tooling that > >>> is coordinating the microcode update to be aware of any SGX > >>> enclaves that are running and possibly coordinate sequencing with > >>> the processes containing those enclaves. This coupling does not > >>> exist today. > >> Also, a microcode update may not affect SGX security at all and doing > >> the EUPDATESVN procedure may not be required for this particular > >> update. This case is called out specifically in the EUPDATESVN > >> documentation. > > > > I don't think Intel provides the metadata for the kernel to tell if an > > update requires an EUPDATESVN procedure or not. If this is inconvenient > > for you, I'd suggest reporting this to the folks at Intel who can fix > > it. It doesn't seem like something which they are motivated to fix. > > Whether or not metadata is currently made available is orthogonal to > creating a mechanism by which userspace can indidate that a particular > microcode update shouldn't trigger the EUPDATESVN procedure. I'm not sure if you have noticed the discussions in v1: https://lore.kernel.org/all/1742be9e-c18e-28c9-75c8-144bf1f6a311@intel.com/T/#m18e6fecd8c9c517c68cb4d62e53f24909abd50a7 We remove the sysfs which allows userspace to decide if and when to trigger the EUPDATESVN procedure. Please comment if you have other suggestion. > > -- > Jethro Beekman | Fortanix
diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h index d6bfdfb0f0af..1ba66b9fe198 100644 --- a/arch/x86/include/asm/microcode.h +++ b/arch/x86/include/asm/microcode.h @@ -3,6 +3,7 @@ #define _ASM_X86_MICROCODE_H #include <asm/cpu.h> +#include <asm/sgx.h> #include <linux/earlycpio.h> #include <linux/initrd.h> @@ -137,4 +138,8 @@ static inline void load_ucode_ap(void) { } static inline void reload_early_microcode(void) { } #endif +#ifndef update_cpusvn_intel +static inline void update_cpusvn_intel(void) {} +#endif + #endif /* _ASM_X86_MICROCODE_H */ diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h index d5942d0848ec..d0f2832a57b3 100644 --- a/arch/x86/include/asm/sgx.h +++ b/arch/x86/include/asm/sgx.h @@ -412,4 +412,9 @@ int sgx_virt_einit(void __user *sigstruct, void __user *token, int sgx_set_attribute(unsigned long *allowed_attributes, unsigned int attribute_fd); +#ifdef CONFIG_X86_SGX +void update_cpusvn_intel(void); +#define update_cpusvn_intel update_cpusvn_intel +#endif + #endif /* _ASM_X86_SGX_H */ diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c index 64deb7727d00..514e621f04c3 100644 --- a/arch/x86/kernel/cpu/common.c +++ b/arch/x86/kernel/cpu/common.c @@ -59,6 +59,7 @@ #include <asm/cpu_device_id.h> #include <asm/uv/uv.h> #include <asm/sigframe.h> +#include <asm/sgx.h> #include "cpu.h" @@ -2165,6 +2166,14 @@ void microcode_check(void) perf_check_microcode(); + /* + * SGX related microcode update requires EUPDATESVN to update CPUSVN, which + * will destroy all enclaves to ensure EPC is not in use. If SGX is configured + * and EUPDATESVN is supported, call the EUPDATESVN procecure. + */ + if (IS_ENABLED(CONFIG_X86_SGX) && (cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN)) + update_cpusvn_intel(); + /* Reload CPUID max function as it might've changed. */ info.cpuid_level = cpuid_eax(0); diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c index 123818fa2386..d86745d8cc7d 100644 --- a/arch/x86/kernel/cpu/sgx/main.c +++ b/arch/x86/kernel/cpu/sgx/main.c @@ -1380,3 +1380,15 @@ static int sgx_updatesvn(void) return ret; } + +void update_cpusvn_intel(void) +{ + sgx_lock_epc(); + if (sgx_zap_pages()) + goto out; + + sgx_updatesvn(); + +out: + sgx_unlock_epc(); +}
EUPDATESVN is the SGX instruction which allows enclave attestation to include information about updated microcode without a reboot. Microcode updates which affect SGX require two phases: 1. Do the main microcode update 2. Make the new CPUSVN available for enclave attestation via EUPDATESVN. Before a EUPDATESVN can succeed, all enclave pages (EPC) must be marked as unused in the SGX metadata (EPCM). This operation destroys all preexisting SGX enclave data and metadata. This is by design and mitigates the impact of vulnerabilities that may have compromised enclaves or the SGX hardware itself prior to the update. Signed-off-by: Cathy Zhang <cathy.zhang@intel.com> --- Changes since v1: - Remove the sysfs file svnupdate. (Thomas Gleixner, Dave Hansen) - Let late microcode load path call ENCLS[EUPDATESVN] procedure directly. (Borislav Petkov) - Redefine update_cpusvn_intel() to return void instead of int. --- arch/x86/include/asm/microcode.h | 5 +++++ arch/x86/include/asm/sgx.h | 5 +++++ arch/x86/kernel/cpu/common.c | 9 +++++++++ arch/x86/kernel/cpu/sgx/main.c | 12 ++++++++++++ 4 files changed, 31 insertions(+)