linux-kernel - Re: [PATCH v3 2/2] x86/sgx: Implement EUPDATESVN and opportunistically call it during first EPC page alloc

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: <aAJfJ38wt2bnguhg@google.com>
Date: Fri, 18 Apr 2025 07:18:46 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Kai Huang <kai.huang@...el.com>
Cc: Elena Reshetova <elena.reshetova@...el.com>, Dave Hansen <dave.hansen@...el.com>, 
	"linux-sgx@...r.kernel.org" <linux-sgx@...r.kernel.org>, Vincent Scarlata <vincent.r.scarlata@...el.com>, 
	"x86@...nel.org" <x86@...nel.org>, "jarkko@...nel.org" <jarkko@...nel.org>, 
	Vishal Annapurve <vannapurve@...gle.com>, Chong Cai <chongc@...gle.com>, 
	Asit K Mallick <asit.k.mallick@...el.com>, Erdem Aktas <erdemaktas@...gle.com>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "bondarn@...gle.com" <bondarn@...gle.com>, 
	"dionnaglaze@...gle.com" <dionnaglaze@...gle.com>, Scott Raynor <scott.raynor@...el.com>
Subject: Re: [PATCH v3 2/2] x86/sgx: Implement EUPDATESVN and
 opportunistically call it during first EPC page alloc

On Thu, Apr 17, 2025, Kai Huang wrote:
> I think the sgx_updatesvn() should just return true when EUPDATESVN returns 0 or
> SGX_NO_UPDATE, and return false for all other error codes.  And it should
> ENCLS_WARN() for all other error codes, except SGX_INSUFFICIENT_ENTROPY because
> it can still legally happen.
> 
> Something like:
> 
> 	do {
> 		ret = __eupdatesvn();
> 		if (ret != SGX_INSUFFICIENT_ENTROPY)
> 			break;
> 	} while (--retry);

This can be:

	do {
		ret = __eupdatesvn();
	} while (ret == SGX_INSUFFICIENT_ENTROPY && --retry)

To make it super obvious that retry is only relevant to lack of entropy.

> 	if (!ret || ret == SGX_NO_UPDATE) {
> 		/*
> 		 * SVN successfully updated, or it was already up-to-date.
> 		 * Let users know when the update was successful.
> 		 */
> 		if (!ret)
> 			pr_info("SVN updated successfully\n");
> 		return true;

Returning true/false is confusing since the vast majority of the SGX code uses
'0' for success.  A lot of cleverness went into splicing SGX's error codes into
the kernel's -ernno; it would be a shame to ignore that :-)

E.g. this looks wrong at first (and second glance)

			ret = sgx_updatesvn();
			if (!ret) {
				/*
				 * sgx_updatesvn() returned unknown error, smth
				 * must be broken, do not allocate a page from EPC
				 */
				spin_unlock(&sgx_epc_eupdatesvn_lock);
				spin_unlock(&node->lock);
				return NULL;
			}

> 	}
> 
> 	/*
> 	 * EUPDATESVN was called when EPC is empty, all other error
> 	 * codes are unexcepted except running out of entropy.
> 	 */
> 	if (ret != SGX_INSUFFICIENT_ENTROPY)
> 		ENCLS_WARN(ret, "EUPDATESVN");
> 
> 	return false;
> 		
> 
> In __sgx_alloc_epc_page_from_node(), it should fail to allocate EPC page and
> return -ENOMEM when sgx_updatesvn() returns false.  We should only allow EPC to

No, it should return a meaningful error code, not -ENOMEM.  And if that's the
behavior you want, then __sgx_alloc_epc_page() should be updated to bail immediately.
The current code assuming -ENOMEM is the only failure scenario:

	do {
		page = __sgx_alloc_epc_page_from_node(nid);
		if (page)
			return page;

		nid = next_node_in(nid, sgx_numa_mask);
	} while (nid != nid_start);

That should be something like:

	do {
		page = __sgx_alloc_epc_page_from_node(nid);
		if (!IS_ERR(page) || PTR_ERR(page) != -ENOMEM)
			return page;

		nid = next_node_in(nid, sgx_numa_mask);
	} while (nid != nid_start);

> be allocated when we know the SVN is already up-to-date.
> 
> Any further call of EPC allocation will trigger sgx_updatesvn() again.  If it
> was failed due to unexpected error, then it should continue to fail,
> guaranteeing "sgx_alloc_epc_page() return consistently -ENOMEM, if the
> unexpected happens".  If it was failed due to running out of entropy, it then
> may fail again, or it will just succeed and then SGX can continue to work.


Side topic, the function comment for __sgx_alloc_epc_page() is stale/wrong.  It
returns ERR_PTR(-ENOMEM), not NULL, on failure.

/**
 * __sgx_alloc_epc_page() - Allocate an EPC page
 *
 * Iterate through NUMA nodes and reserve ia free EPC page to the caller. Start
 * from the NUMA node, where the caller is executing.
 *
 * Return:
 * - an EPC page:	A borrowed EPC pages were available.
 * - NULL:		Out of EPC pages.
 */