Re: [RFC PATCH 3/4] x86/sgx: Obtain backing storage page with enclave mutex held

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Dave,

On 4/28/2022 2:58 PM, Dave Hansen wrote:
> On 4/28/22 13:11, Reinette Chatre wrote:
>> @@ -252,6 +252,7 @@ static void sgx_reclaimer_write(struct sgx_epc_page *epc_page,
>>  	sgx_encl_ewb(epc_page, backing);
>>  	encl_page->epc_page = NULL;
>>  	encl->secs_child_cnt--;
>> +	sgx_encl_put_backing(backing, true);
> 
> Could you also talk a bit about why this needed to move?  It's a bit
> harder to make sense of the refcounting when the get and put are done in
> different functions.

This needed to move to address the following scenario described in the
changelog:

sgx_reclaim_pages() {                    sgx_vma_fault() {
...                                      ...
/* write data to backing store */
sgx_reclaimer_write();
                                         mutex_lock(&encl->lock);
                                         __sgx_encl_eldu() {
                                         ...
                                         /* page not dirty -
                                          * contents may not be
                                          * up to date
                                         */
                                         sgx_encl_get_backing();
                                         ...
                                         }
                                         ...
/* set page dirty */
sgx_encl_put_backing();
...
                                         mutex_unlock(&encl->lock);
}                                        }

Before this change the backing store page was modified within
sgx_reclaimer_write() that essentially does:

sgx_reclaimer_write() {
    mutex_lock(&encl->lock);
    /* write encrypted data to backing store */
    mutex_unlock(&encl->lock);
}

The reclaimer followed the sgx_reclaimer_write() call with a
call to sgx_encl_put_backing() where the pages have their
dirty bits set. sgx_encl_put_backing() was thus done without
the enclave mutex held. If that page is faulted in at that
time the page fault handler may thus attempt to load
the page from the backing store between its contents being
changed and it being marked as dirty.

After the change the page fault handler would not be able to
load the page from the backing store before it is marked as
dirty.

I can improve the flow in the changelog to be clear on the
reclaimer's mutex usage. Perhaps something like:

sgx_reclaim_pages() {                    sgx_vma_fault() {
...                                      ...

  sgx_reclaimer_write() {
     mutex_lock(&encl->lock);
     /* write to backing store */
     mutex_unlock(&encl->lock);
  }
                                         mutex_lock(&encl->lock);
                                         __sgx_encl_eldu() {
                                         ...
                                         /* page not dirty -
                                          * contents may not be
                                          * up to date
                                         */
                                         sgx_encl_get_backing();
                                         ...
                                         }
                                         ...
/* set page dirty */
sgx_encl_put_backing();
...
                                         mutex_unlock(&encl->lock);
}                                        }



Reinette