On Mon, Apr 29, 2024 at 04:04:24PM +0300, Jarkko Sakkinen wrote:
On Mon Apr 29, 2024 at 1:43 PM EEST, Dmitrii Kuvaiskii wrote:
Two enclave threads may try to access the same non-present enclave page simultaneously (e.g., if the SGX runtime supports lazy allocation). The threads will end up in sgx_encl_eaug_page(), racing to acquire the enclave lock. The winning thread will perform EAUG, set up the page table entry, and insert the page into encl->page_array. The losing thread will then get -EBUSY on xa_insert(&encl->page_array) and proceed to error handling path.
And that path removes page. Not sure I got gist of this tbh.
Well, this is not about a redundant EREMOVE performed. This is about the enclave page becoming inaccessible due to a bug triggered with a data race.
Consider some enclave page not yet added to the enclave. The enclave performs a memory access to it at the same time on CPU1 and CPU2. Since the page does not yet have a corresponding PTE, the #PF handler on both CPUs calls sgx_vma_fault(). Scenario proceeds as follows:
/* * Fault on CPU1 */ sgx_vma_fault() {
xa_load(&encl->page_array) == NULL ->
sgx_encl_eaug_page() {
... /* * Fault on CPU2 */ sgx_vma_fault() {
xa_load(&encl->page_array) == NULL ->
sgx_encl_eaug_page() {
...
mutex_lock(&encl->lock); /* * alloc encl_page */ /* * alloc EPC page */ epc_page = sgx_alloc_epc_page(...); /* * add page_to enclave's xarray */ xa_insert(&encl->page_array, ...); /* * add page to enclave via EAUG * (page is in pending state) */ /* * add PTE entry */ vmf_insert_pfn(...);
mutex_unlock(&encl->lock); return VM_FAULT_NOPAGE; } } mutex_lock(&encl->lock); /* * alloc encl_page */ /* * alloc EPC page */ epc_page = sgx_alloc_epc_page(...); /* * add page_to enclave's xarray, * this fails with -EBUSY */ xa_insert(&encl->page_array, ...);
err_out_shrink: sgx_encl_free_epc_page(epc_page) { /* * remove page via EREMOVE */ /* * free EPC page */ sgx_free_epc_page(epc_page); }
mutex_unlock(&encl->lock); return VM_FAULT_SIGBUS; } }
CPU2 added the enclave page (in pending state) to the enclave and installed the PTE. The kernel gives control back to the user space, without raising a signal. The user space on CPU2 retries the memory access and induces a page fault, but now with the SGX bit set in the #PF error code. The #PF handler calls do_user_addr_fault(), which calls access_error() and ultimately raises a SIGSEGV. The userspace SIGSEGV handler is supposed to perform EACCEPT, after which point the enclave page becomes accessible.
CPU1 however jumps to the error handling path because the page was already inserted into the enclave's xarray. This error handling path EREMOVEs the page and also raises a SIGBUS signal to user space. The PTE entry is not removed.
After CPU1 performs EREMOVE, this enclave page becomes perpetually inaccessible (until an SGX_IOC_ENCLAVE_REMOVE_PAGES ioctl). This is because the page is marked accessible in the PTE entry but is not EAUGed. Because of this combination, the #PF handler sees the SGX bit set in the #PF error code and does not call sgx_vma_fault() but instead raises a SIGSEGV. The userspace SIGSEGV handler cannot perform EACCEPT because the page was not EAUGed. Thus, the user space is stuck with the inaccessible page.
Also note that in the scenario, CPU1 raises a SIGBUS signal to user space unnecessarily. This signal is spurious because a page-access retry on CPU2 will also raise the SIGBUS signal. That said, this side effect is less severe because it affects only user space. Therefore, it could be circumvented in user space alone, but it seems reasonable to fix it in this patch.
This error handling path contains two bugs: (1) SIGBUS is sent to userspace even though the enclave page is correctly installed by another thread, and (2) sgx_encl_free_epc_page() is called that performs EREMOVE even though the enclave page was never intended to be removed. The first bug is less severe because it impacts only the user space; the second bug is more severe because it also impacts the OS state by ripping the page (added by the winning thread) from the enclave.
Fix these two bugs (1) by returning VM_FAULT_NOPAGE to the generic Linux fault handler so that no signal is sent to userspace, and (2) by replacing sgx_encl_free_epc_page() with sgx_free_epc_page() so that no EREMOVE is performed.
What is the collateral damage caused by ENCLS[EREMOVE]?
As explained above, the damage is that the SGX driver leaves the enclave page metadata in an inconsistent state: on the one hand, the PTE entry is installed which forces the generic Linux fault handler to raise SIGSEGV, and on the other hand, the page is not in a correct state to be EACCEPTed (i.e., EAUG was not performed on this page).
Fixes: 5a90d2c3f5ef ("x86/sgx: Support adding of pages to an initialized enclave") Cc: stable@vger.kernel.org Reported-by: Marcelina Kościelnicka mwk@invisiblethingslab.com Suggested-by: Reinette Chatre reinette.chatre@intel.com Signed-off-by: Dmitrii Kuvaiskii dmitrii.kuvaiskii@intel.com
arch/x86/kernel/cpu/sgx/encl.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c index 279148e72459..41f14b1a3025 100644 --- a/arch/x86/kernel/cpu/sgx/encl.c +++ b/arch/x86/kernel/cpu/sgx/encl.c @@ -382,8 +382,11 @@ static vm_fault_t sgx_encl_eaug_page(struct vm_area_struct *vma, * If ret == -EBUSY then page was created in another flow while * running without encl->lock */
- if (ret)
- if (ret) {
if (ret == -EBUSY)vmret = VM_FAULT_NOPAGE;- }
pginfo.secs = (unsigned long)sgx_get_epc_virt_addr(encl->secs.epc_page); pginfo.addr = encl_page->desc & PAGE_MASK; @@ -419,7 +422,7 @@ static vm_fault_t sgx_encl_eaug_page(struct vm_area_struct *vma, err_out_shrink: sgx_encl_shrink(encl, va_page); err_out_epc:
- sgx_encl_free_epc_page(epc_page);
- sgx_free_epc_page(epc_page);
This ignores check for the page being reclaimer tracked, i.e. it does changes that have been ignored in the commit message.
Indeed, sgx_encl_free_epc_page() performs the following check:
WARN_ON_ONCE(page->flags & SGX_EPC_PAGE_RECLAIMER_TRACKED);
However, the EPC page is allocated in sgx_encl_eaug_page() and has zeroed-out flags in all error-handling paths. In other words, the page is marked as reclaimable only in the happy path of sgx_encl_eaug_page(). Therefore, in the particular code path that I changed this "page reclaimer tracked" condition is always false, and the warning is never printed.
Do you want me to explain this in the commit message?
---
(Below questions are from follow-up emails, I add them to this reply email to have all discussions in one place.)
What is the collateral damage caused by ENCLS[EREMOVE]?
Have you measured cost of eremove on an empty page?
I tried to lookup for a thread from lore because I have a faint memory that it was concluded that its cost irrelevant. Please correct if I'm wrong.
Also pseudocode for EREMOVE supports this as it just returns without actually doing anything.
I have not measured the cost of EREMOVE on an empty page. This cost may be negligible. But as stated above, my patch does not get rid of EREMOVE simply for performance reasons. My patch removes a data race that leads to a forever-inaccessible enclave page.
-- Dmitrii Kuvaiskii