On Thu, 2019-08-08 at 08:55 -0700, Sean Christopherson wrote: > On Thu, Aug 08, 2019 at 06:52:29PM +0300, Jarkko Sakkinen wrote: > > On Wed, Aug 07, 2019 at 05:12:53PM -0700, Sean Christopherson wrote: > > > Move the taking of the enclave's lock outside of sgx_encl_grow() in > > > preparation for adding sgx_encl_shrink(), which will decrement the > > > number of enclave pages and free any allocated VA page. When freeing > > > a VA page, the enclave's lock needs to be held for the entire time > > > between adding the VA page to the enclave's list and freeing the VA > > > page so as to prevent it from being used by reclaim, e.g. to avoid a > > > use-after-free scenario. > > > > > > Because sgx_encl_grow() can temporarily drop encl->lock, calling it > > > with encl->lock held adds a subtle dependency on the ordering of > > > checks against encl->flags, e.g. checking for SGX_ENCL_CREATED prior > > > to calling sgx_encl_grow() could lead to a TOCTOU on ECREATE. Avoid > > > this by passing in the disallowed flags to sgx_encl_grow() so that the > > > the dependency is clear. > > > > > > Retaking encl->lock in the failure paths is a bit ugly, but the > > > alternative is to have sgx_encl_grow() drop encl->lock in all failure > > > paths, which is arguably worse since the caller has to know which > > > paths do/don't drop the lock. > > > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx> > > > > Would be cleaner to check the flags just before the call. Otherwise, > > no problems with this. > > That's not sufficient in the case that sgx_encl_grow() drops encl->lock > to allocate an EPC page, as the flags need to be rechecked after the > lock is reacquired. I'm not a huge fan of the code either, but it was > the least ugly solution I could come up with and kinda fit since > sgx_encl_grow() was already checking SGX_ENCL_DEAD. Right, I was too hazy. We'll go what you proposed. /Jarkko
