Message-ID: <aJci8Zt9iSb3LurZ@kernel.org>
Date: Sat, 9 Aug 2025 13:29:05 +0300
From: Jarkko Sakkinen <jarkko@...nel.org>
To: Elena Reshetova <elena.reshetova@...el.com>
Cc: dave.hansen@...el.com, seanjc@...gle.com, kai.huang@...el.com,
	mingo@...nel.org, linux-sgx@...r.kernel.org,
	linux-kernel@...r.kernel.org, x86@...nel.org,
	asit.k.mallick@...el.com, vincent.r.scarlata@...el.com,
	chongc@...gle.com, erdemaktas@...gle.com, vannapurve@...gle.com,
	bondarn@...gle.com, scott.raynor@...el.com
Subject: Re: [PATCH v11 0/5] Enable automatic SVN updates for SGX enclaves

On Wed, Aug 06, 2025 at 11:11:51AM +0300, Elena Reshetova wrote:
> Changes since v10 following reviews by Dave:
> 
>  - merge patch 1 and 2
>  - patch 1: clarify the comment about the function prototype
>  - patch 3: clarify the description of SGX_NO_UPDATE
>   error code, move the definition of EUPDATESVN enum leaf
>   to patch 4
>  - patch 4: clarify commit, adjust sgx_update_svn() function
>   according to feedback
> 
> Changes since v9 following reviews by Kai:
> 
>  - postpone the definition of sgx_inc_usage_count
>    until patch 6
>  - clarify the commit message in patch 6
>  - minor fixes
> 
> Note: I didn't merge patch 1 and 2 since it goes against
> previous suggestion made by Jarkko.
> 
> Changes since v8 following reviews by Dave and Jarkko:
> 
>  - fix the sgx_inc/dec_usage_count() to not do any
>  actual counting until the later patch when adequate
>  mutex is introduced as suggested by Dave
>  - add an additional patch (patch 1) to redefine
>  existing sgx_(vepc_)open into wrappers to allow
>  the follow up patch to introduce the sgx_inc/dec_usage_count()
>  functions into sgx_(vepc_)open cleanly as suggested
>  by Jarkko. 
> 
> Changes since v7 following reviews by Dave:
> 
>  - change sgx_usage_count to be a normal int type
>  and greatly simplify the sgx_inc_usage_count func.
>  This results in requiring a mutex for each sgx_(vepc_)open
>  but given that the mutex is held a short amount of
>  time it should be ok perf-wise.
> 
> Changes since v6 following reviews by Kai,Jarkko
> and Dave:
> 
>  - fix sgx_update_svn function description
>  - add maybe_unused for sgx_update_svn in patch 4
>  to silence the warning, remove it in patch 5
>  - add note to patch 1 explaining why the prototype
>  sgx_inc_usage_count returns int and not void
>  - fix the order of return code checks in 
>  sgx_update_svn
>  - cosmetic fixes
>  
> Note: I didn't change the sgx_inc/dec_usage_count
> to statics because they are called from a number of
> different code locations and also rely on a static
> sgx_usage_count, which lives naturally in main.c.  
>  
> Changes since v5 following reviews by Ingo, Kai,
> Jarkko and Dave:
> 
>  - rebase on x86 tip
>  - patch 1 is fixed to do correct unwinding in
>  case of errors
>  - patch 2: add feature flag to cpuid-deps.c
>  - patch 3: remove unused SGX_EPC_NOT_READY error code
>  - patch 4: fix x86 feature check, return -EAGAIN
>  on SGX_INSUFFICIENT_ENTROPY and -EIO on other non-
>  expected errors. Comments/style are also fixed.
>  - patch 5: rewrite commit message, add comments inline
> 
> Changes since v4 following reviews by Dave and Jarkko:
> 
>  - breakdown the single patch into 4 patches as
>  suggested by Dave
>  - fix error unwinding in sgx_(vepc_)open as 
>  suggested by Jarkko
>  - numerous code improvements suggested by Dave
>  - numerous additional code comments and commit
>  message improvements as suggested by Dave
>  - switch to usage of atomic64_t for sgx_usage_count
>  to ensure overflows cannot happen as suggested by Dave
>  - do not return a error case when failing with
>  SGX_INSUFFICIENT_ENTROPY, fail silently as suggested
>  by Dave
> 
> Changes since v3 following reviews by Kai and Sean:
> 
>  - Change the overall approach to the one suggested
>   by Sean and do the EUPDATESVN execution during
>   sgx_open() and sgx_vepc_open().
>   Note, I do not try to do EUPDATESVN during the release()
>   flows since it doesnt save any noticable amount
>   of time during next open flow per underlying EUPDATESVN
>   microcode logic.
>  - In sgx_update_svn() remove the check that we are
>   running under VMM and expect the VMM to instead
>   expose correct CPUID
>  - Move the actual CPUID leaf check out of
>   sgx_update_svn() into sgx_init()
>  - Use existing RDRAND_RETRY_LOOPS define instead of 10
>  - Change the sgx_update_svn() to return 0 only in
>  success cases (or if unsupported)
>  - Various smaller cosmetic fixes
> 
> The still to be discussed question is what sgx_update_svn()
> should return in case of various failures. The current version
> follows suggestion by Kai and would return an error (and block
> sgx_(vepc_)open() in all cases, including running out of entropy.
> I think this might be the correct approach for SGX_INSUFFICIENT_ENTROPY
> since in such cases userspace can retry the open() and also
> will get an info about what is actually blocking the EUPDATEVSN
> (and can act on this). However, this is a change in existing API
> and therefore debatable and I would like to hear people's feedback.
> 
> Changes since v2 following review by Jarkko:
> 
>  - formatting of comments is fixed
>  - change from pr_error to ENCLS_WARN to communicate errors from
>  EUPDATESVN
>  - In case an unknown error is detected (must not happen per spec),
>  make page allocation from EPC fail in order to prevent EPC usage
> 
> Changes since v1 following review by Jarkko:
> 
>  - first and second patch are squashed together and a better
>    explanation of the change is added into the commit message
>  - third and fourth patch are also combined for better understanding
>    of error code purposes used in 4th patch
>  - implementation of sgx_updatesvn adjusted following Jarkko's 
>    suggestions
>  - minor fixes in both commit messages and code from the review
>  - dropping co-developed-by tag since the code now differs enough
>    from the original submission. However, the reference where the
>    original code came from and credits to original author is kept
> 
> Background
> ----------
> 
> In case an SGX vulnerability is discovered and TCB recovery
> for SGX is triggered, Intel specifies a process that must be
> followed for a given vulnerability. Steps to mitigate can vary
> based on vulnerability type, affected components, etc.
> In some cases, a vulnerability can be mitigated via a runtime
> recovery flow by shutting down all running SGX enclaves,
> clearing enclave page cache (EPC), applying a microcode patch
> that does not require a reboot (via late microcode loading) and
> restarting all SGX enclaves.
> 
> 
> Problem statement
> -------------------------
> Even when the above-described runtime recovery flow to mitigate the
> SGX vulnerability is followed, the SGX attestation evidence will
> still reflect the security SVN version being equal to the previous
> state of security SVN (containing vulnerability) that created
> and managed the enclave until the runtime recovery event. This
> limitation currently can be only overcome via a platform reboot,
> which negates all the benefits from the rebootless late microcode
> loading and not required in this case for functional or security
> purposes.
> 
> 
> Proposed solution
> -----------------
> 
> SGX architecture introduced  a new instruction called EUPDATESVN [1]
> 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.
> 
> This series enables opportunistic execution of EUPDATESVN upon first
> EPC page allocation for a first enclave to be run on the platform.
> 
> This series is partly based on the previous work done by Cathy Zhang
> [2], which attempted to enable forceful destruction of all SGX
> enclaves and execution of EUPDATESVN upon successful application of
> any microcode patch. This approach is determined as being too
> intrusive for the running SGX enclaves, especially taking into account
> that it would be performed upon *every* microcode patch application
> regardless if it changes the security SVN version or not (change to the
> security SVN version is a rare event).
> 
> Testing
> -------
> 
> Tested on EMR machine using kernel-6.16.0_rc7 & sgx selftests.
> Also tested on a Kaby Lake machine without EUPDATESVN support.
> If Google folks in CC can test on their side, it would be greatly appreciated.
> 
> 
> References
> ----------
> 
> [1] https://cdrdv2.intel.com/v1/dl/getContent/648682?explicitVersion=true
> [2] https://lore.kernel.org/all/20220520103904.1216-1-cathy.zhang@intel.com/
> 
> Elena Reshetova (5):
>   x86/sgx: Introduce functions to count the sgx_(vepc_)open()
>   x86/cpufeatures: Add X86_FEATURE_SGX_EUPDATESVN feature flag
>   x86/sgx: Define error codes for use by ENCLS[EUPDATESVN]
>   x86/sgx: Implement ENCLS[EUPDATESVN]
>   x86/sgx: Enable automatic SVN updates for SGX enclaves
> 
>  arch/x86/include/asm/cpufeatures.h       |  1 +
>  arch/x86/include/asm/sgx.h               | 37 ++++++----
>  arch/x86/kernel/cpu/cpuid-deps.c         |  1 +
>  arch/x86/kernel/cpu/scattered.c          |  1 +
>  arch/x86/kernel/cpu/sgx/driver.c         | 19 ++++-
>  arch/x86/kernel/cpu/sgx/encl.c           |  1 +
>  arch/x86/kernel/cpu/sgx/encls.h          |  5 ++
>  arch/x86/kernel/cpu/sgx/main.c           | 91 ++++++++++++++++++++++++
>  arch/x86/kernel/cpu/sgx/sgx.h            |  3 +
>  arch/x86/kernel/cpu/sgx/virt.c           | 20 +++++-
>  tools/arch/x86/include/asm/cpufeatures.h |  1 +
>  11 files changed, 163 insertions(+), 17 deletions(-)
> 
> -- 
> 2.45.2
> 
> 

LGTM

Reviewed-by: Jarkko Sakkinen <jarkko@...nel.org>

BR, Jarkko

Powered by blists - more mailing lists