mbox series

[RFC,v2,0/5] security: x86/sgx: SGX vs. LSM

Message ID 20190606021145.12604-1-sean.j.christopherson@intel.com (mailing list archive)
Headers show
Series security: x86/sgx: SGX vs. LSM | expand

Message

Sean Christopherson June 6, 2019, 2:11 a.m. UTC
This series is the result of a rather absurd amount of discussion over
how to get SGX to play nice with LSM policies, without having to resort
to evil shenanigans or put undue burden on userspace.  Discussions are
still ongoing, e.g. folks are exploring alternatives to changing the
proposed SGX UAPI, but I wanted to get this updated version of the code
posted to show a fairly minimal implemenation(from a kernel perspective),
e.g. the diff stats aren't too scary, especially considering 50% of the
added lines are comments.

This series is a delta to Jarkko's ongoing SGX series and applies on
Jarkko's current master at https://github.com/jsakkine-intel/linux-sgx.git:

  dfc89a83b5bc ("docs: x86/sgx: Document the enclave API")

The basic gist of the approach is to track an enclave's page protections
separately from any vmas that map the page, and separate from the hardware
enforced protections.  The SGX UAPI is modified to require userspace to
explicitly define the protections for each enclave page, i.e. the ioctl
to add pages to an enclave is extended to take PROT_{READ,WRITE,EXEC}
flags.

An enclave page's protections are the maximal protections that userspace
can use to map the page, e.g. mprotect() and mmap() are rejected if the
protections for the vma would be more permissible than those of the
associated enclave page.

Tracking protections for an enclave page (in additional to vmas) allows
SGX to invoke LSM upcalls while the enclave is being built.  This is
critical to enabling LSMs to implement policies for enclave pages that
are functionally equivalent to existing policies for normal pages.

v1: https://lkml.kernel.org/r/20190531233159.30992-1-sean.j.christopherson@intel.com

v2:
  - Dropped the patch(es) to extend the SGX UAPI to allow adding multiple
    enclave pages in a single syscall [Jarkko].

  - Reject ioctl() immediately on LSM denial [Stephen].

  - Rework SELinux code to avoid checking EXEMEM multiple times [Stephen].

  - Adding missing equivalents to existing selinux_file_protect() checks
    [Stephen].

  - Hold mmap_sem across copy_to_user() to prevent a TOCTOU race when
    checking the source vma [Stephen].

  - Stubify security_enclave_load() if !CONFIG_SECURITY [Stephen].

  - Make flags a 32-bit field [Andy].

  - Don't validate the SECINFO protection flags against the enclave
    page's protection flags [Andy].

  - Rename mprotect() hook to may_mprotect() [Andy].

  - Test 'vma->vm_flags & VM_MAYEXEC' instead of manually checking for
    a noexec path [Jarkko].

  - Drop the SGX defined flags (use PROT_*) [Jarkko].

  - Improve comments and changelogs [Jarkko].

Sean Christopherson (5):
  mm: Introduce vm_ops->may_mprotect()
  x86/sgx: Require userspace to define enclave pages' protection bits
  x86/sgx: Enforce noexec filesystem restriction for enclaves
  LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX
  security/selinux: Add enclave_load() implementation

 arch/x86/include/uapi/asm/sgx.h        |  2 +
 arch/x86/kernel/cpu/sgx/driver/ioctl.c | 57 ++++++++++++++++++---
 arch/x86/kernel/cpu/sgx/driver/main.c  |  5 ++
 arch/x86/kernel/cpu/sgx/encl.c         | 53 ++++++++++++++++++++
 arch/x86/kernel/cpu/sgx/encl.h         |  4 ++
 include/linux/lsm_hooks.h              | 13 +++++
 include/linux/mm.h                     |  2 +
 include/linux/security.h               | 12 +++++
 mm/mprotect.c                          | 15 ++++--
 security/security.c                    |  7 +++
 security/selinux/hooks.c               | 69 ++++++++++++++++++++++++++
 11 files changed, 228 insertions(+), 11 deletions(-)

Comments

Xing, Cedric June 10, 2019, 7:03 a.m. UTC | #1
This series intends to make the new SGX subsystem and the existing LSM
architecture work together smoothly so that, say, SGX cannot be abused to work
around restrictions set forth by LSM. This series applies on top of Jarkko
Sakkinen's SGX series v20 (https://lkml.org/lkml/2019/4/17/344), where abundant
details of this SGX/LSM problem could be found.

This series is an alternative to Sean Christopherson's recent RFC series
(https://lkml.org/lkml/2019/6/5/1070) that was trying to solve the same
problem. The key problem is for LSM to determine the "maximal (most permissive)
protection" allowed for individual enclave pages. Sean's approach is to take
that from user mode code as a parameter of the EADD ioctl, validate it with LSM
ahead of time, and then enforce it inside the SGX subsystem. The major
disadvantage IMHO is that a priori knowledge of "maximal protection" is needed,
but it isn't always available in certain use cases. In fact, it is an unusual
approach to take "maximal protection" from user code, as what SELinux is doing
today is to determine "maximal protection" of a vma using attributes associated
with vma->vm_file instead. When it comes to enclaves, vma->vm_file always
points /dev/sgx/enclave, so what's missing is a new way for LSM modules to
remember origins of enclave pages so that they don't solely depend on
vma->vm_file to determine "maximal protection".

This series takes advantage of the fact that enclave pages cannot be remapped
(to different linear address), therefore the pair of { vma->vm_file,
linear_address } can be used to uniquely identify an enclave page. Then by
notifying LSM on creation of every enclave page (via a new LSM hook -
security_enclave_load), LSM modules would be able to track origin and
protection changes of every page, hence be able to judge correctly upon
mmap/mprotect requests.

Cedric Xing (3):
  LSM/x86/sgx: Add SGX specific LSM hooks
  LSM/x86/sgx: Implement SGX specific hooks in SELinux
  LSM/x86/sgx: Call new LSM hooks from SGX subsystem

 arch/x86/kernel/cpu/sgx/driver/ioctl.c |  72 +++++-
 arch/x86/kernel/cpu/sgx/driver/main.c  |  12 +-
 include/linux/lsm_hooks.h              |  33 +++
 include/linux/security.h               |  26 +++
 security/security.c                    |  21 ++
 security/selinux/Makefile              |   2 +
 security/selinux/hooks.c               |  77 ++++++-
 security/selinux/include/intel_sgx.h   |  18 ++
 security/selinux/include/objsec.h      |   3 +
 security/selinux/intel_sgx.c           | 292 +++++++++++++++++++++++++
 10 files changed, 545 insertions(+), 11 deletions(-)
 create mode 100644 security/selinux/include/intel_sgx.h
 create mode 100644 security/selinux/intel_sgx.c
Jarkko Sakkinen June 10, 2019, 5:36 p.m. UTC | #2
On Mon, Jun 10, 2019 at 12:03:03AM -0700, Cedric Xing wrote:
> This series intends to make the new SGX subsystem and the existing LSM
> architecture work together smoothly so that, say, SGX cannot be abused to work
> around restrictions set forth by LSM. This series applies on top of Jarkko
> Sakkinen's SGX series v20 (https://lkml.org/lkml/2019/4/17/344), where abundant
> details of this SGX/LSM problem could be found.
> 
> This series is an alternative to Sean Christopherson's recent RFC series
> (https://lkml.org/lkml/2019/6/5/1070) that was trying to solve the same
> problem. The key problem is for LSM to determine the "maximal (most permissive)
> protection" allowed for individual enclave pages. Sean's approach is to take
> that from user mode code as a parameter of the EADD ioctl, validate it with LSM
> ahead of time, and then enforce it inside the SGX subsystem. The major
> disadvantage IMHO is that a priori knowledge of "maximal protection" is needed,
> but it isn't always available in certain use cases. In fact, it is an unusual
> approach to take "maximal protection" from user code, as what SELinux is doing
> today is to determine "maximal protection" of a vma using attributes associated
> with vma->vm_file instead. When it comes to enclaves, vma->vm_file always
> points /dev/sgx/enclave, so what's missing is a new way for LSM modules to
> remember origins of enclave pages so that they don't solely depend on
> vma->vm_file to determine "maximal protection".
> 
> This series takes advantage of the fact that enclave pages cannot be remapped
> (to different linear address), therefore the pair of { vma->vm_file,
> linear_address } can be used to uniquely identify an enclave page. Then by
> notifying LSM on creation of every enclave page (via a new LSM hook -
> security_enclave_load), LSM modules would be able to track origin and
> protection changes of every page, hence be able to judge correctly upon
> mmap/mprotect requests.
> 
> Cedric Xing (3):
>   LSM/x86/sgx: Add SGX specific LSM hooks
>   LSM/x86/sgx: Implement SGX specific hooks in SELinux
>   LSM/x86/sgx: Call new LSM hooks from SGX subsystem

A patch set containing direct LSM changes should consider all LSMs.
This will allow all the LSM maintainers to consider the changes. Now we
have a limited audience and we are favoring one LSM.

There is no good reason why direct LSM changes cannot be done
post-upstreaming like we do for virtualization.

Looking at Sean's patches, overally 1/5-3/5 make perfect sense.

/Jarkko