On Mon, Aug 26, 2019 at 05:11:27PM -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, but that > is less desirable as doing so would unnecessarily interfere with EPC >b page reclaim. > > Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx> I think the error code should be -EBUSY. Our implementation is fairly coherent at the moment that -EINVAL follows from bad input data. Getting -EINVAL from this would be a bit confusing. If atomic_t was used for the enclave flags (see my comment to 1/4), then I think we could implement this like: if (atomic_fetch_or(SGX_ENCL_IOCTL, &encl->flags) & SGX_ENCL_IOCTL) return -EIOCTL; /Jarkko
