Hi Dave,
On 5/4/2022 3:58 PM, Dave Hansen wrote:
> On 5/4/22 15:13, Reinette Chatre wrote:
>> + /*
>> + * VA page slot ID uses same bit as the flag so it is important
>> + * to ensure that the page is not already in backing store.
>> + */
>> + if (entry->epc_page &&
>> + (entry->desc & SGX_ENCL_PAGE_BEING_RECLAIMED)) {
>> + pr_debug("%s:%d encl %px addr 0x%lx desc 0x%lx (index %lu) being reclaimed\n",
>> + __func__, __LINE__, encl, addr, entry->desc,
>> + PFN_DOWN(entry->desc - entry->encl->base));
>> + reclaimed = 1;
>> + break;
>> + }
>
> I don't think this works as-is. As it stands,
> SGX_ENCL_PAGE_BEING_RECLAIMED is cleared *BEFORE* the EWB. It's
> possible for that check above to race with SGX_ENCL_PAGE_BEING_RECLAIMED
> being cleared. That would erroneously lead pcmd_page_in_use() to return
> false *just* before data is written to the PCMD page.
>
> static void sgx_encl_ewb()
> {
> encl_page->desc &= ~SGX_ENCL_PAGE_BEING_RECLAIMED;
> ...
> __sgx_encl_ewb()
> }
You are correct that SGX_ENCL_PAGE_BEING_RECLAIMED is cleared before
EWB. Please do note though that clearing that bit and running EWB
are all done in one section with the enclave mutex held the entire time.
The page fault handler cannot check for SGX_ENCL_PAGE_BEING_RECLAIMED
until the reclaimer releases the mutex.
These checks are thus not able to race. Here is a higher level view:
sgx_reclaim_pages() {
...
/*
* Reclaim ENCLAVE_A
*/
mutex_lock(&encl->lock);
/*
* Get a reference to ENCLAVE_A's
* shmem page where enclave page
* encrypted data will be stored
* as well as a reference to the
* enclave page's PCMD data page,
* PCMD_AB.
* Release mutex before writing
* any data to the shmem pages.
*/
sgx_encl_get_backing(...);
encl_page->desc |= SGX_ENCL_PAGE_BEING_RECLAIMED;
mutex_unlock(&encl->lock);
/*
* Fault ENCLAVE_B
*/
sgx_vma_fault() {
mutex_lock(&encl->lock);
/*
* Get reference to
* ENCLAVE_B's shmem page
* as well as PCMD_AB.
*/
sgx_encl_get_backing(...)
/*
* Load page back into
* enclave via ELDU.
*/
/*
* Release reference to
* ENCLAVE_B' shmem page and
* PCMD_AB.
*/
sgx_encl_put_backing(...);
/*
* PCMD_AB is found empty so
* it and ENCLAVE_B's shmem page
* are truncated.
*/
/* Truncate ENCLAVE_B backing page */
sgx_encl_truncate_backing_page();
/*
* pcmd_page_empty indicates that
* PCMD_AB is empty
*/
pcmd_page_in_use() {
/*
* Check SGX_ENCL_PAGE_BEING_RECLAIMED
* of pages sharing PCMD page.
* Will find that ENCLAVE_A's
* SGX_ENCL_PAGE_BEING_RECLAIMED is
* set and PCMD_AB page will NOT
* be truncated.
*/
}
/* sgx_encl_truncate_backing_page(); => not run */
mutex_unlock(&encl->lock);
...
}
mutex_lock(&encl->lock);
encl_page->desc &=
~SGX_ENCL_PAGE_BEING_RECLAIMED;
/*
* Write encrypted contents of
* ENCLAVE_A to ENCLAVE_A shmem
* page and its PCMD data to
* PCMD_AB.
*/
sgx_encl_put_backing(...)
/*
* Reference to PCMD_AB is
* dropped.
*/
mutex_unlock(&encl->lock);
}
>
> You might be able to move the ~SGX_ENCL_PAGE_BEING_RECLAIMED until after
> the __sgx_encl_ewb().
This will not change much since all is done with mutex held. At the current
location that bit has served its purpose and the code that follows will
replace it with the VA slot information.
> But, at that point, the pcmd_page_empty is a bit
> useless because it's stale.
I cannot see how pcmd_page_empty would be stale - could you please elaborate
on the flow where you see this happening?
> I guess it might act as a performance
> optimization because it allows avoiding the expensive pcmd_page_in_use()
> check.
>
> Also, one nit:
>
>> + /*
>> + * Address of enclave page using the first entry within the
>> + * PCMD page.
>> + */
>> + pcmd_first_page = PFN_PHYS(page_index & ~0x1FUL) + encl->base;
>
> That ~0x1FUL is pretty magic. I assume that's 5 bits because there are
> 32 PCMDs per page. This could probably be something resembling this
> instead:
>
> #define PCMDS_PER_PAGE (PAGE_SIZE/sizeof(struct sgx_pcmd))
> #define PCMD_FIRST_MASK GENMASK(PCMDS_PER_PAGE, 0)
>
Thank you, yes, that needs to improve.
> One other thing. The role of encl->lock here is very important.
> Without it, two concurrent page faults could do their individual
> memset(), each see !pcmd_page_empty, then decline to truncate the page.
Your argument is not clear to me. Yes, this would be an issue (and
that will not be the only issue) if the enclave mutex is not held
by the page fault handler ... but the enclave mutex _is_ held by the
page fault handler.
> Also, given the challenges here, I do think we should check the
> pcmd_page after truncate to ensure it is still all zero's.
Could you please elaborate the challenges? I do not think it would
help the issue at hand at all to add a check since this is done with
the mutex held while the reclaimer has a reference to the page but no
mutex ... as long as the page fault handler has that mutex it can write
and check the PCMD page all it want, the new changes would occur to the
PCMD page when it (the page fault handler) releases the mutex and the
reclaimer attempts to use the page it has a reference to.
Reinette
