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.
