linux-sgx.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: linux-sgx@vger.kernel.org
Subject: [PATCH 0/4] x86/sgx: Fix lock ordering bug w/ EADD
Date: Mon, 26 Aug 2019 17:11:24 -0700	[thread overview]
Message-ID: <20190827001128.25066-1-sean.j.christopherson@intel.com> (raw)

As discovered by Jarkko, taking mm->mmap_sem around EADD can lead to
deadlock as attempting to acquire mmap_sem while holding encl->lock
violates SGX's lock ordering.

Resolving the issue simply by reversing the lock ordering gets ugly due
to the behavior of sgx_encl_grow(), which has a path that drops encl->lock
in order to do EPC page reclaim, i.e. moving mm->mmap_sem up would require
it to be dropped and reacquired as well.

Luckily, the entire flow can be simplified by preventing userspace from
invoking concurrent ioctls on a single enclave.  Requiring ioctls to be
serialized means encl->lock doesn't need to be held to grow/shrink the
enclave, since only ioctls can grow/shrink the enclave.  This also
eliminates theoretical cases that sgx_encl_grow() has to handle, e.g. the
enclave being initialized while it's waiting on reclaim, since the
protection provided by serializing ioctls isn't dropped to do reclaim.

Alternatively, the issue could be resolved by sitching to get_user_pages()
to snapshot the physical page when performance the noexec check (and maybe
in the future, LSM checks).  Since there is no significant (dis)advantage
to using gup(), and simplifying sgx_encl_grow() is a worthwhile change on
its own, I'd prefer get this issue resolved and have a separate discussion
on gup() vs passing the userspace address to ENCLS as is.

Patches 1 and 2 are bug fixes that are affected somewhat by serializing
the ioctls.  I included them here to have the full context of what the
final code/semantics.

Sean Christopherson (4):
  x86/sgx: Ensure enclave state is visible before marking it created
  x86/sgx: Preserved allowed attributes during SGX_IOC_ENCLAVE_CREATE
  x86/sgx: Reject concurrent ioctls on single enclave
  x86/sgx: Take encl->lock inside of mm->mmap_sem for EADD

 arch/x86/kernel/cpu/sgx/driver.c |   1 +
 arch/x86/kernel/cpu/sgx/encl.h   |   1 +
 arch/x86/kernel/cpu/sgx/ioctl.c  | 162 +++++++++++++++++--------------
 3 files changed, 89 insertions(+), 75 deletions(-)

-- 
2.22.0


             reply	other threads:[~2019-08-27  0:13 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-27  0:11 Sean Christopherson [this message]
2019-08-27  0:11 ` [PATCH 1/4] x86/sgx: Ensure enclave state is visible before marking it created Sean Christopherson
2019-08-27 11:20   ` Jarkko Sakkinen
2019-08-27 16:42     ` Sean Christopherson
2019-08-27  0:11 ` [PATCH 2/4] x86/sgx: Preserved allowed attributes during SGX_IOC_ENCLAVE_CREATE Sean Christopherson
2019-08-27 12:12   ` Jarkko Sakkinen
2019-08-27 12:25     ` Jarkko Sakkinen
2019-08-27  0:11 ` [PATCH 3/4] x86/sgx: Reject concurrent ioctls on single enclave Sean Christopherson
2019-08-27 12:11   ` Jarkko Sakkinen
2019-08-27  0:11 ` [PATCH 4/4] x86/sgx: Take encl->lock inside of mm->mmap_sem for EADD Sean Christopherson
2019-08-27 12:21   ` Jarkko Sakkinen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190827001128.25066-1-sean.j.christopherson@intel.com \
    --to=sean.j.christopherson@intel.com \
    --cc=jarkko.sakkinen@linux.intel.com \
    --cc=linux-sgx@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).