On Fri, May 23, 2025 at 06:57:50PM +0300, Jarkko Sakkinen wrote:
> On Thu, May 22, 2025 at 12:21:37PM +0300, Elena Reshetova wrote:
> > All running enclaves and cryptographic assets (such as internal SGX
> > encryption keys) are assumed to be compromised whenever an SGX-related
> > microcode update occurs. To mitigate this assumed compromise the new
> > supervisor SGX instruction ENCLS[EUPDATESVN] can generate fresh
> > cryptographic assets.
> >
> > Before executing EUPDATESVN, all SGX memory must be marked as unused.
> > This requirement ensures that no potentially compromised enclave
> > survives the update and allows the system to safely regenerate
> > cryptographic assets.
> >
> > Add the method to perform ENCLS[EUPDATESVN].
> >
> > Signed-off-by: Elena Reshetova <elena.reshet...@intel.com>
> > ---
> > arch/x86/kernel/cpu/sgx/encls.h | 5 +++
> > arch/x86/kernel/cpu/sgx/main.c | 67 +++++++++++++++++++++++++++++++++
> > 2 files changed, 72 insertions(+)
> >
> > diff --git a/arch/x86/kernel/cpu/sgx/encls.h
> > b/arch/x86/kernel/cpu/sgx/encls.h
> > index 99004b02e2ed..d9160c89a93d 100644
> > --- a/arch/x86/kernel/cpu/sgx/encls.h
> > +++ b/arch/x86/kernel/cpu/sgx/encls.h
> > @@ -233,4 +233,9 @@ static inline int __eaug(struct sgx_pageinfo *pginfo,
> > void *addr)
> > return __encls_2(EAUG, pginfo, addr);
> > }
> >
> > +/* Attempt to 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 a018b01b8736..109d40c89fe8 100644
> > --- a/arch/x86/kernel/cpu/sgx/main.c
> > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > @@ -16,6 +16,7 @@
> > #include <linux/vmalloc.h>
> > #include <asm/msr.h>
> > #include <asm/sgx.h>
> > +#include <asm/archrandom.h>
> > #include "driver.h"
> > #include "encl.h"
> > #include "encls.h"
> > @@ -920,6 +921,72 @@ EXPORT_SYMBOL_GPL(sgx_set_attribute);
> > /* Counter to count the active SGX users */
> > static atomic64_t sgx_usage_count;
> >
> > +/**
> > + * sgx_updatesvn() - Attempt to call ENCLS[EUPDATESVN].
> > + * This instruction attempts to update CPUSVN to the
> > + * currently loaded microcode update SVN and generate new
> > + * cryptographic assets. Must be called when EPC is empty.
> > + * Most of the time, there will be no update and that's OK.
> > + * If the failure is due to SGX_INSUFFICIENT_ENTROPY, the
> > + * operation can be safely retried. In other failure cases,
> > + * the retry should not be attempted.
> > + *
> > + * Return:
> > + * 0: Success or not supported
> > + * -EAGAIN: Can be safely retried, failure is due to lack of
> > + * entropy in RNG.
> > + * -EIO: Unexpected error, retries are not advisable.
> > + */
> > +static int sgx_update_svn(void)
> > +{
> > + int ret;
> > +
> > + /*
> > + * If EUPDATESVN is not available, it is ok to
> > + * silently skip it to comply with legacy behavior.
> > + */
> > + if (!cpu_feature_enabled(X86_FEATURE_SGX_EUPDATESVN))
> > + return 0;
> > +
> > + for (int i = 0; i < RDRAND_RETRY_LOOPS; i++) {
> > + ret = __eupdatesvn();
> > +
> > + /* Stop on success or unexpected errors: */
> > + if (ret != SGX_INSUFFICIENT_ENTROPY)
> > + break;
> > + }
> > +
> > + /*
> > + * SVN was already up-to-date. This is the most
> > + * common case.
> > + */
> > + if (ret == SGX_NO_UPDATE)
> > + return 0;
> > +
> > + /*
> > + * SVN update failed due to lack of entropy in DRNG.
> > + * Indicate to userspace that it should retry.
> > + */
> > + if (ret == SGX_INSUFFICIENT_ENTROPY)
> > + return -EAGAIN;
> > +
> > + if (!ret) {
> > + /*
> > + * SVN successfully updated.
> > + * Let users know when the update was successful.
> > + */
> > + pr_info("SVN updated successfully\n");
> > + return 0;
> > + }
> > +
> > + /*
> > + * EUPDATESVN was called when EPC is empty, all other error
> > + * codes are unexpected.
> > + */
> > + ENCLS_WARN(ret, "EUPDATESVN");
> > + return -EIO;
> > +}
>
> Even if unlikely() was not used I still don't agree with the order i.e.,
> dealing with the success case in the middle. So I stand with my earlier
> suggestion, except unlikely() (since that was a problem for David, not
> going to fight over it).
Oops s/David/Dave/, sorry.
BR, Jarkko
