Re: [PATCH for_v22 10/11] x86/sgx: Call sgx_encl_grow() with the enclave's lock held

[Sean Christopherson <sean.j.christopherson@intel.com>]

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@intel.com>
> 
> 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.