linux-kernel - Re: [RFC PATCH v2 09/10] x86/cpu: Call ENCLS[EUPDATESVN] procedure in microcode update

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e1682971-4821-8e57-3232-de7a72086991@fortanix.com>
Date:   Wed, 16 Mar 2022 11:24:34 +0100
From:   Jethro Beekman <jethro@...tanix.com>
To:     Cathy Zhang <cathy.zhang@...el.com>, linux-sgx@...r.kernel.org,
        linux-kernel@...r.kernel.org
Cc:     dave.hansen@...el.com, ashok.raj@...el.com
Subject: Re: [RFC PATCH v2 09/10] x86/cpu: Call ENCLS[EUPDATESVN] procedure in
 microcode update

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@...el.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

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4490 bytes)