| Message ID | 20191016183745.8226-2-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sgx: Bug fixes for v23 | expand |
On Wed, Oct 16, 2019 at 11:37:34AM -0700, Sean Christopherson wrote: > Use the kernel's virtual address to reference the source page when > EADDing a page to the enclave. gup() "does not guarantee that the page > exists in the user mappings", i.e. EADD can still fault and deadlock > due to mmap_sem contention if it consumes the userspace address. > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > + kunmap_atomic((void *)pginfo.contents); Remark: if not casted first to unsigned long, it will give a warning on 32-bit build. /Jarkko
On Fri, Oct 18, 2019 at 12:57:13PM +0300, Jarkko Sakkinen wrote: > On Wed, Oct 16, 2019 at 11:37:34AM -0700, Sean Christopherson wrote: > > Use the kernel's virtual address to reference the source page when > > EADDing a page to the enclave. gup() "does not guarantee that the page > > exists in the user mappings", i.e. EADD can still fault and deadlock > > due to mmap_sem contention if it consumes the userspace address. > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > + kunmap_atomic((void *)pginfo.contents); > > Remark: if not casted first to unsigned long, it will give a warning on > 32-bit build. Do we care? SGX is 64-bit only, I don't see that changing anytime soon.
On Mon, Oct 21, 2019 at 08:22:07PM -0700, Sean Christopherson wrote: > On Fri, Oct 18, 2019 at 12:57:13PM +0300, Jarkko Sakkinen wrote: > > On Wed, Oct 16, 2019 at 11:37:34AM -0700, Sean Christopherson wrote: > > > Use the kernel's virtual address to reference the source page when > > > EADDing a page to the enclave. gup() "does not guarantee that the page > > > exists in the user mappings", i.e. EADD can still fault and deadlock > > > due to mmap_sem contention if it consumes the userspace address. > > > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > > + kunmap_atomic((void *)pginfo.contents); > > > > Remark: if not casted first to unsigned long, it will give a warning on > > 32-bit build. > > Do we care? SGX is 64-bit only, I don't see that changing anytime soon. Nope, that's why I started it with remark :-) It's merely note to myself. /Jarkko
diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c index bb8fcc4f91e3..2dd0eceee111 100644 --- a/arch/x86/kernel/cpu/sgx/ioctl.c +++ b/arch/x86/kernel/cpu/sgx/ioctl.c @@ -328,16 +328,14 @@ static int __sgx_encl_add_page(struct sgx_encl *encl, if (ret < 1) return ret; - __uaccess_begin(); - pginfo.secs = (unsigned long)sgx_epc_addr(encl->secs.epc_page); pginfo.addr = SGX_ENCL_PAGE_ADDR(encl_page); pginfo.metadata = (unsigned long)secinfo; - pginfo.contents = (unsigned long)src; + pginfo.contents = (unsigned long)kmap_atomic(src_page); ret = __eadd(&pginfo, sgx_epc_addr(epc_page)); - __uaccess_end(); + kunmap_atomic((void *)pginfo.contents); put_page(src_page); return ret ? -EFAULT : 0;
Use the kernel's virtual address to reference the source page when EADDing a page to the enclave. gup() "does not guarantee that the page exists in the user mappings", i.e. EADD can still fault and deadlock due to mmap_sem contention if it consumes the userspace address. Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/kernel/cpu/sgx/ioctl.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)