linux-kernel - RE: [PATCH v5 4/5] x86/sgx: Implement ENCLS[EUPDATESVN]

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: <DM8PR11MB57509295C87F7FB54B773107E79FA@DM8PR11MB5750.namprd11.prod.outlook.com>
Date: Tue, 20 May 2025 06:31:46 +0000
From: "Reshetova, Elena" <elena.reshetova@...el.com>
To: Jarkko Sakkinen <jarkko@...nel.org>
CC: "Hansen, Dave" <dave.hansen@...el.com>, "seanjc@...gle.com"
	<seanjc@...gle.com>, "Huang, Kai" <kai.huang@...el.com>,
	"linux-sgx@...r.kernel.org" <linux-sgx@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"x86@...nel.org" <x86@...nel.org>, "Mallick, Asit K"
	<asit.k.mallick@...el.com>, "Scarlata, Vincent R"
	<vincent.r.scarlata@...el.com>, "Cai, Chong" <chongc@...gle.com>, "Aktas,
 Erdem" <erdemaktas@...gle.com>, "Annapurve, Vishal" <vannapurve@...gle.com>,
	"dionnaglaze@...gle.com" <dionnaglaze@...gle.com>, "bondarn@...gle.com"
	<bondarn@...gle.com>, "Raynor, Scott" <scott.raynor@...el.com>
Subject: RE: [PATCH v5 4/5] x86/sgx: Implement ENCLS[EUPDATESVN]

 > +/**
> > + * sgx_updatesvn() - Attempt to call ENCLS[EUPDATESVN]
> > + * If EPC is empty, this instruction attempts to update CPUSVN to the
> > + * currently loaded microcode update SVN and generate new
> > + * cryptographic assets.sgx_updatesvn() Most of the time, there will
> 
> Is there something wrong here in the text? It looks malformed.

Yes, sorry, looks like copy-paste error I missed in the comment. 
Will fix. 

> 
> > + * be no update and that's OK.
> > + *
> > + * Return:
> > + * 0: Success, not supported or run out of entropy
> > + */
> > +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 (!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 either was up-to-date or SVN update failed due
> > +	 * to lack of entropy. In both cases, we want to return
> > +	 * 0 in order not to break sgx_(vepc_)open. We dont expect
> > +	 * SGX_INSUFFICIENT_ENTROPY error unless underlying RDSEED
> > +	 * is under heavy pressure.
> > +	 */
> > +	if ((ret == SGX_NO_UPDATE) || (ret == SGX_INSUFFICIENT_ENTROPY))
> 
> 	if (ret == SGX_NO_UPDATE || ret == SGX_INSUFFICIENT_ENTROPY)

Ok, but I will have to change this anyhow since we seems to trend that we want
to return -EBUSY when SGX_INSUFFICIENT_ENTROPY and do not
proceed with open() call. 

> 
> > +		return 0;
> > +
> > +	if (!ret) {
> > +		/*
> > +		 * SVN successfully updated.
> > +		 * Let users know when the update was successful.
> > +		 */
> 
> This comment is like as useless as an inline comment can ever possibly
> be. Please, remove it.

It is actually not quite so useless because this is the rare case we know
the EUPDATESVN actually executed and hence the pr_info also below.
Without this, there will be no way for sysadmin to trace whenever CPU
SVN was upgraded or not (Sean mentioned that this is already pretty
opaque to users). 

> 
> > +		pr_info("SVN updated successfully\n");
> 
> Let's not add this either in the scope of this patch set.

See above. 

> 
> > +		return 0;
> > +	}
> 
> Since you parse error codes already, I don't understand why deal with
> the success case in the middle of doing that.
> 
> More consistent would be (not also the use of unlikely()):
> 
> 	if (ret == SGX_NO_UPDATE || ret == SGX_INSUFFICIENT_ENTROPY)
> 		return 0;
> 
> 	/*
> 	 * EUPDATESVN was called when EPC is empty, all other error
> 	 * codes are unexpected.
> 	 */
> 	if (unlikely(ret)) {
> 		ENCLS_WARN(ret, "EUPDATESVN");
> 		return ret;
> 	}
> 
> 	return 0;
> }
> 
> This is how I would rewrite the tail of this function.

I think everyone already re-wrote this function at least once and no one is
happy with the version from previous person )) 
Let me try another version again, taking into account changes in return codes
discussed in this thread also. 

Best Regards,
Elena.