| Message ID | 20220401142409.26215-10-cathy.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support microcode updates affecting SGX | expand |
On Fri, Apr 01, 2022 at 10:24:08PM +0800, Cathy Zhang wrote: > @@ -2086,6 +2087,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)) Stick all that above... > + update_cpusvn_intel(); ...inside this function so that you have only the call here. Inside the function you can do all the ifdeffery, checking and commenting and so on. Just like perf_check_microcode() does. Thx.
On 4/1/22 07:24, Cathy Zhang wrote: > +#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 74bcb6841a4b..5867f5a78d13 100644 > --- a/arch/x86/include/asm/sgx.h > +++ b/arch/x86/include/asm/sgx.h > @@ -409,4 +409,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 7b8382c11788..f53fd877ba0d 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" > > @@ -2086,6 +2087,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(); In addition to what Borislav said, these #ifdefs are not how we do things. The update_cpusvn_intel() shouldn't be declared in two different headers. Just imagine what happens if some code happens to include the microcode header *then* the sgx.h header. Please define both the 'static inline' stub *and* the declaration in sgx.h. Then you won't even need the #ifndef trickery. Please also add an "sgx_" to its name prefix while you're at it. That comment is also not great. A reader will likely have *ZERO* idea what EUPDATESVN is or what a CPUSVN is. Try to focus on what the code *means* rather than just repeat the if() conditions in the comment.
Hi Boris, Thanks for helping review! > -----Original Message----- > From: Borislav Petkov <bp@alien8.de> > Sent: Friday, April 1, 2022 10:30 PM > To: Zhang, Cathy <cathy.zhang@intel.com> > Cc: linux-sgx@vger.kernel.org; x86@kernel.org; jarkko@kernel.org; Chatre, > Reinette <reinette.chatre@intel.com>; Hansen, Dave > <dave.hansen@intel.com>; Raj, Ashok <ashok.raj@intel.com> > Subject: Re: [RFC PATCH v3 09/10] x86/cpu: Call ENCLS[EUPDATESVN] > procedure in microcode update > > On Fri, Apr 01, 2022 at 10:24:08PM +0800, Cathy Zhang wrote: > > @@ -2086,6 +2087,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)) > > Stick all that above... The comment is re-written as follows: /* * SGX attestation incorporates the microcode versions of all processors * on the system and is affected by microcode updates. So, update SGX * attestation metric (called CPUSVN) to ensure enclaves attest to the * new version after microcode update. */ > > > + update_cpusvn_intel(); > > ...inside this function so that you have only the call here. Inside the function > you can do all the ifdeffery, checking and commenting and so on. Just like > perf_check_microcode() does. > Thanks for showing me the example! I've referred. > Thx. > > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette
Hi Dave, Thanks for your comments and suggestions! > -----Original Message----- > From: Hansen, Dave <dave.hansen@intel.com> > Sent: Friday, April 1, 2022 11:42 PM > To: Zhang, Cathy <cathy.zhang@intel.com>; linux-sgx@vger.kernel.org; > x86@kernel.org > Cc: jarkko@kernel.org; Chatre, Reinette <reinette.chatre@intel.com>; Raj, > Ashok <ashok.raj@intel.com> > Subject: Re: [RFC PATCH v3 09/10] x86/cpu: Call ENCLS[EUPDATESVN] > procedure in microcode update > > On 4/1/22 07:24, Cathy Zhang wrote: > > +#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 74bcb6841a4b..5867f5a78d13 100644 > > --- a/arch/x86/include/asm/sgx.h > > +++ b/arch/x86/include/asm/sgx.h > > @@ -409,4 +409,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 7b8382c11788..f53fd877ba0d > 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" > > > > @@ -2086,6 +2087,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(); > > In addition to what Borislav said, these #ifdefs are not how we do things. > > The update_cpusvn_intel() shouldn't be declared in two different headers. > Just imagine what happens if some code happens to include the microcode > header *then* the sgx.h header. > > Please define both the 'static inline' stub *and* the declaration in sgx.h. > Then you won't even need the #ifndef trickery. Please also add an "sgx_" to > its name prefix while you're at it. Moved to sgx.h and added "sgx_". > > That comment is also not great. A reader will likely have *ZERO* idea what > EUPDATESVN is or what a CPUSVN is. Try to focus on what the code > *means* rather than just repeat the if() conditions in the comment. Yes, I see. The update comment is shown in my reply to Boris. Please take a look.
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 74bcb6841a4b..5867f5a78d13 100644 --- a/arch/x86/include/asm/sgx.h +++ b/arch/x86/include/asm/sgx.h @@ -409,4 +409,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 7b8382c11788..f53fd877ba0d 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" @@ -2086,6 +2087,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 784780314762..20800c543110 100644 --- a/arch/x86/kernel/cpu/sgx/main.c +++ b/arch/x86/kernel/cpu/sgx/main.c @@ -1409,3 +1409,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(+)