Re: [PATCH v3 2/5] x86/sgx: Reject concurrent ioctls on single enclave

[Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>]

On Thu, Aug 29, 2019 at 05:17:03PM -0700, Sean Christopherson wrote:
> Except for ENCLAVE_SET_ATTRIBUTE, all SGX ioctls() must be executed
> serially to successfully initialize an enclave, e.g. the kernel already
> strictly requires ECREATE->EADD->EINIT, and concurrent EADDs will result
> in an unstable MRENCLAVE.  Explicitly enforce serialization by returning
> EINVAL if userspace attempts an ioctl while a different ioctl for the
> same enclave is in progress.
> 
> The primary beneficiary of explicit serialization is sgx_encl_grow() as
> it no longer has to deal with dropping and reacquiring encl->lock when
> a new VA page needs to be allocated.  Eliminating the lock juggling in
> sgx_encl_grow() paves the way for fixing a lock ordering bug in
> ENCLAVE_ADD_PAGE without having to also juggle mm->mmap_sem.
> 
> Serializing ioctls also fixes a race between ENCLAVE_CREATE and
> ENCLAVE_SET_ATTRIBUTE, as the latter does not take encl->lock, e.g.
> concurrent updates to allowed_attributes could result in a stale
> value.  The race could also be fixed by taking encl->lock or making
> allowed_attributes atomic, but both add unnecessary overhead with this
> change applied.
> 
> Note, manipulating the list of VA pages (encl->va_pages) during EADD
> must be done while holding encl->lock, the list of VA pages is visible
> to the reclaimer after the first EADD completes.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

Took a few rounds but I think it is now right :-)

Was tricky to squash between three patches but I think I got that one
right too.

Can you check driver, provisioning and reclaimer patches and see if
you see anything weird?

/Jarkko