On Mon, Jul 06, 2020 at 06:38:47PM -0700, Sean Christopherson wrote: > On Sat, Jul 04, 2020 at 04:43:49AM +0300, Jarkko Sakkinen wrote: > > On Mon, Jun 29, 2020 at 08:27:19AM -0700, Sean Christopherson wrote: > > > On Sat, Jun 27, 2020 at 07:43:35PM +0200, Borislav Petkov wrote: > > > > And you could do similar sanity checks in the other ioctl functions. > > > > > > Ya, as above, SGX_ENCL_INITIALIZED can be checked here. > > > > > > SGX_ENCL_DEAD is actually already checked in in the top level sgx_ioctl(), > > > i.e. the check in sgx_encl_add_page() can technically be flat out dropped. > > > > > > I say "technically" because I'm a bit torn over SGX_ENCL_DEAD; encl->lock > > > must be held to SGX_ENCL_DEAD (the page fault and reclaim flows rely on > > > this), but as it stands today only ioctl() paths (guarded by > > > SGX_ENCL_IOCTL) > > > and sgx_release() (makes the ioctls() unreachable) set SGX_ENCL_DEAD. > > > > > > So it's safe to check SGX_ENCL_DEAD from ioctl() context without holding > > > encl->lock, at least in the current code base, but it feels weird/sketchy. > > > > > > In the end I don't think I have a strong opinion. Removing the > > > technically > > > unnecessary DEAD check in sgx_encl_add_page() is the simplest change, so > > > it > > > may make sense to do that and nothing more for initial upstreaming. Long > > > term, I fully expect we'll add paths that set SGX_ENCL_DEAD outside of > > > ioctl() context, e.g. to handle EPC OOM, but it wouldn't be a bad thing to > > > have a standalone commit in a future series to add DEAD checks (under > > > encl->lock) in the ADD and INIT flows. > > > > AFAIK nonne of th ioctl's should not need SGX_ENCL_DEAD check. > > I can't tell if the double negative was intended, but I took a peek at your > current master and see that you removed the SGX_ENCL_DEAD check in > sgx_ioctl(). That check needs to stay, e.g. if EEXTEND fails we absolutely > need to prevent any further operations on the enclave. > > The above was calling out that additional checks on SGX_ENCL_DEAD after > acquiring encl->lock are not necessary because SGX_ENCL_DEAD can only be > set when the ioctls() are no longer reachable or from within an ioctl(), > which provides exclusivity via SGX_ENCL_IOCTL.
Got it. /Jarkko
