Message ID | 20240517110631.3441817-3-dmitrii.kuvaiskii@intel.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | x86/sgx: Fix two data races in EAUG/EREMOVE flows | expand |
On 5/17/24 04:06, Dmitrii Kuvaiskii wrote: ... First, why is SGX so special here? How is the SGX problem different than what the core mm code does? > --- a/arch/x86/kernel/cpu/sgx/encl.h > +++ b/arch/x86/kernel/cpu/sgx/encl.h > @@ -25,6 +25,9 @@ > /* 'desc' bit marking that the page is being reclaimed. */ > #define SGX_ENCL_PAGE_BEING_RECLAIMED BIT(3) > > +/* 'desc' bit marking that the page is being removed. */ > +#define SGX_ENCL_PAGE_BEING_REMOVED BIT(2) Second, convince me that this _needs_ a new bit. Why can't we just have a bit that effectively means "return EBUSY if you see this bit when handling a fault". > struct sgx_encl_page { > unsigned long desc; > unsigned long vm_max_prot_bits:8; > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c > index 5d390df21440..de59219ae794 100644 > --- a/arch/x86/kernel/cpu/sgx/ioctl.c > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c > @@ -1142,6 +1142,7 @@ static long sgx_encl_remove_pages(struct sgx_encl *encl, > * Do not keep encl->lock because of dependency on > * mmap_lock acquired in sgx_zap_enclave_ptes(). > */ > + entry->desc |= SGX_ENCL_PAGE_BEING_REMOVED; This also needs a comment, no matter what.
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c index 41f14b1a3025..7ccd8b2fce5f 100644 --- a/arch/x86/kernel/cpu/sgx/encl.c +++ b/arch/x86/kernel/cpu/sgx/encl.c @@ -257,7 +257,8 @@ static struct sgx_encl_page *__sgx_encl_load_page(struct sgx_encl *encl, /* Entry successfully located. */ if (entry->epc_page) { - if (entry->desc & SGX_ENCL_PAGE_BEING_RECLAIMED) + if (entry->desc & (SGX_ENCL_PAGE_BEING_RECLAIMED | + SGX_ENCL_PAGE_BEING_REMOVED)) return ERR_PTR(-EBUSY); return entry; diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h index f94ff14c9486..fff5f2293ae7 100644 --- a/arch/x86/kernel/cpu/sgx/encl.h +++ b/arch/x86/kernel/cpu/sgx/encl.h @@ -25,6 +25,9 @@ /* 'desc' bit marking that the page is being reclaimed. */ #define SGX_ENCL_PAGE_BEING_RECLAIMED BIT(3) +/* 'desc' bit marking that the page is being removed. */ +#define SGX_ENCL_PAGE_BEING_REMOVED BIT(2) + struct sgx_encl_page { unsigned long desc; unsigned long vm_max_prot_bits:8; diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c index 5d390df21440..de59219ae794 100644 --- a/arch/x86/kernel/cpu/sgx/ioctl.c +++ b/arch/x86/kernel/cpu/sgx/ioctl.c @@ -1142,6 +1142,7 @@ static long sgx_encl_remove_pages(struct sgx_encl *encl, * Do not keep encl->lock because of dependency on * mmap_lock acquired in sgx_zap_enclave_ptes(). */ + entry->desc |= SGX_ENCL_PAGE_BEING_REMOVED; mutex_unlock(&encl->lock); sgx_zap_enclave_ptes(encl, addr);