Re: [PATCH 1/2] x86/sgx: Resolve EAUG race where losing thread returns SIGBUS

[Dmitrii Kuvaiskii]

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@xxxxxxxxxxxxxxx
> > Reported-by: Marcelina Kościelnicka <mwk@xxxxxxxxxxxxxxxxxxxxxx>
> > Suggested-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>
> > Signed-off-by: Dmitrii Kuvaiskii <dmitrii.kuvaiskii@xxxxxxxxx>
> > ---
> >  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;
> >  		goto err_out_shrink;
> > +	}
> >  
> >  	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