[RFC,v3,00/12] security: x86/sgx: SGX vs. LSM, round 3
mbox series

Message ID 20190617222438.2080-1-sean.j.christopherson@intel.com
Headers show
Series
  • security: x86/sgx: SGX vs. LSM, round 3
Related show

Message

Sean Christopherson June 17, 2019, 10:24 p.m. UTC
My original plan was for my next RFC to be an implementation of Andy's
proposed "dynamic tracking" model.  I actually finished the tracking
portion, but was completely flummoxed by the auditing[1].  Since Cedric's
RFC is essentially a variation of the dynamic tracking model, it too has
the same auditing complexities.  End result, I ended back at the "make
userspace state its intentions" approach.

Except for patch 12 (see below), the SGX changes have been fully tested,
including updating the kernel's selftest as well as my own fork of (an old
version of) Intel's SDK to use the new UAPI.  The LSM changes have been
smoke tested, but I haven't actually configured AppArmor or SELinux to
verify the permissions work as intended.

Patches 1-3 are bug fixes that should go into v21 regardless of what we
end up doing for LSM support.  They're included here as the actual LSM
RFC patches are essentially untestable without them.

Patches 4-11 are the meat of the RFC.

Patch 12 is purely to show how we might implement SGX2 support.  It's not
intended to be included in v21.


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:

  4cc5d411db1b ("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.


[1] https://lkml.kernel.org/r/20190614003759.GE18385@linux.intel.com

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

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

  - 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].

v3:
  - Clear VM_MAY* flags instead of using .may_mprotect() to enforce
    maximal enclave page protections.

  - Update the SGX selftest to work with the new API.

  - Rewrite SELinux code to use SGX specific permissions, with the goal
    of addressing Andy's feedback regarding what people will actually
    care about when it comes to SGX, e.g. add permissions for restricing
    unmeasured code and stop trying to infer permissions from the source
    of each enclave page.

  - Add a (very minimal) AppArmor patch.

  - Show line of sight to SGX2 support.

  - Rebased to Jarkko's latest code base.

Sean Christopherson (12):
  x86/sgx: Add mm to enclave at mmap()
  x86/sgx: Do not naturally align MAP_FIXED address
  selftests: x86/sgx: Mark the enclave loader as not needing an exec
    stack
  x86/sgx: Require userspace to define enclave pages' protection bits
  x86/sgx: Enforce noexec filesystem restriction for enclaves
  mm: Introduce vm_ops->may_mprotect()
  LSM: x86/sgx: Introduce ->enclave_map() hook for Intel SGX
  security/selinux: Require SGX_EXECMEM to map enclave page WX
  LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX
  security/selinux: Add enclave_load() implementation
  security/apparmor: Add enclave_load() implementation
  LSM: x86/sgx: Show line of sight to LSM support SGX2's EAUG

 arch/x86/include/uapi/asm/sgx.h          |  5 +-
 arch/x86/kernel/cpu/sgx/driver/ioctl.c   | 69 +++++++++++------
 arch/x86/kernel/cpu/sgx/driver/main.c    | 94 +++++++++++++++++++++++-
 arch/x86/kernel/cpu/sgx/encl.c           | 81 ++++++++++++--------
 arch/x86/kernel/cpu/sgx/encl.h           |  7 +-
 include/linux/lsm_hooks.h                | 20 +++++
 include/linux/mm.h                       |  2 +
 include/linux/security.h                 | 18 +++++
 mm/mprotect.c                            | 15 +++-
 security/apparmor/include/audit.h        |  2 +
 security/apparmor/lsm.c                  | 14 ++++
 security/security.c                      | 12 +++
 security/selinux/hooks.c                 | 72 ++++++++++++++++++
 security/selinux/include/classmap.h      |  6 +-
 tools/testing/selftests/x86/sgx/Makefile |  2 +-
 tools/testing/selftests/x86/sgx/main.c   | 32 +++++++-
 16 files changed, 384 insertions(+), 67 deletions(-)

Comments

Stephen Smalley June 18, 2019, 1:38 p.m. UTC | #1
On 6/17/19 6:24 PM, Sean Christopherson wrote:
> My original plan was for my next RFC to be an implementation of Andy's
> proposed "dynamic tracking" model.  I actually finished the tracking
> portion, but was completely flummoxed by the auditing[1].  Since Cedric's
> RFC is essentially a variation of the dynamic tracking model, it too has
> the same auditing complexities.  End result, I ended back at the "make
> userspace state its intentions" approach.
> 
> Except for patch 12 (see below), the SGX changes have been fully tested,
> including updating the kernel's selftest as well as my own fork of (an old
> version of) Intel's SDK to use the new UAPI.  The LSM changes have been
> smoke tested, but I haven't actually configured AppArmor or SELinux to
> verify the permissions work as intended.

Was dropping linux-security-module and selinux lists intentional for 
this RFC? Not recommended.

Is the entire series aside from patch 12 available in a public tree 
somewhere?

Ultimately we'll want additions to the selinux-testsuite that exercise 
each of the new permissions, both a permission denied scenario and a 
permission allowed scenario.

> 
> Patches 1-3 are bug fixes that should go into v21 regardless of what we
> end up doing for LSM support.  They're included here as the actual LSM
> RFC patches are essentially untestable without them.
> 
> Patches 4-11 are the meat of the RFC.
> 
> Patch 12 is purely to show how we might implement SGX2 support.  It's not
> intended to be included in v21.
> 
> 
> 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:
> 
>    4cc5d411db1b ("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.
> 
> 
> [1] https://lkml.kernel.org/r/20190614003759.GE18385@linux.intel.com
> 
> v1: https://lkml.kernel.org/r/20190531233159.30992-1-sean.j.christopherson@intel.com
> 
> v2: https://lkml.kernel.org/r/20190606021145.12604-1-sean.j.christopherson@intel.com
> 
>    - 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].
> 
> v3:
>    - Clear VM_MAY* flags instead of using .may_mprotect() to enforce
>      maximal enclave page protections.
> 
>    - Update the SGX selftest to work with the new API.
> 
>    - Rewrite SELinux code to use SGX specific permissions, with the goal
>      of addressing Andy's feedback regarding what people will actually
>      care about when it comes to SGX, e.g. add permissions for restricing
>      unmeasured code and stop trying to infer permissions from the source
>      of each enclave page.
> 
>    - Add a (very minimal) AppArmor patch.
> 
>    - Show line of sight to SGX2 support.
> 
>    - Rebased to Jarkko's latest code base.
> 
> Sean Christopherson (12):
>    x86/sgx: Add mm to enclave at mmap()
>    x86/sgx: Do not naturally align MAP_FIXED address
>    selftests: x86/sgx: Mark the enclave loader as not needing an exec
>      stack
>    x86/sgx: Require userspace to define enclave pages' protection bits
>    x86/sgx: Enforce noexec filesystem restriction for enclaves
>    mm: Introduce vm_ops->may_mprotect()
>    LSM: x86/sgx: Introduce ->enclave_map() hook for Intel SGX
>    security/selinux: Require SGX_EXECMEM to map enclave page WX
>    LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX
>    security/selinux: Add enclave_load() implementation
>    security/apparmor: Add enclave_load() implementation
>    LSM: x86/sgx: Show line of sight to LSM support SGX2's EAUG
> 
>   arch/x86/include/uapi/asm/sgx.h          |  5 +-
>   arch/x86/kernel/cpu/sgx/driver/ioctl.c   | 69 +++++++++++------
>   arch/x86/kernel/cpu/sgx/driver/main.c    | 94 +++++++++++++++++++++++-
>   arch/x86/kernel/cpu/sgx/encl.c           | 81 ++++++++++++--------
>   arch/x86/kernel/cpu/sgx/encl.h           |  7 +-
>   include/linux/lsm_hooks.h                | 20 +++++
>   include/linux/mm.h                       |  2 +
>   include/linux/security.h                 | 18 +++++
>   mm/mprotect.c                            | 15 +++-
>   security/apparmor/include/audit.h        |  2 +
>   security/apparmor/lsm.c                  | 14 ++++
>   security/security.c                      | 12 +++
>   security/selinux/hooks.c                 | 72 ++++++++++++++++++
>   security/selinux/include/classmap.h      |  6 +-
>   tools/testing/selftests/x86/sgx/Makefile |  2 +-
>   tools/testing/selftests/x86/sgx/main.c   | 32 +++++++-
>   16 files changed, 384 insertions(+), 67 deletions(-)
>
Sean Christopherson June 18, 2019, 1:55 p.m. UTC | #2
On Tue, Jun 18, 2019 at 09:38:19AM -0400, Stephen Smalley wrote:
> On 6/17/19 6:24 PM, Sean Christopherson wrote:
> >My original plan was for my next RFC to be an implementation of Andy's
> >proposed "dynamic tracking" model.  I actually finished the tracking
> >portion, but was completely flummoxed by the auditing[1].  Since Cedric's
> >RFC is essentially a variation of the dynamic tracking model, it too has
> >the same auditing complexities.  End result, I ended back at the "make
> >userspace state its intentions" approach.
> >
> >Except for patch 12 (see below), the SGX changes have been fully tested,
> >including updating the kernel's selftest as well as my own fork of (an old
> >version of) Intel's SDK to use the new UAPI.  The LSM changes have been
> >smoke tested, but I haven't actually configured AppArmor or SELinux to
> >verify the permissions work as intended.
> 
> Was dropping linux-security-module and selinux lists intentional for this
> RFC? Not recommended.

Yes, my thought was to keep the noise to the sgx list until we at least
agree on a direction for the SGX UAPI.  I am fully expecting that whatever
LSM and SELinux patches we end up with will go through a lot more scrutiny
when Jarkko sends them with his SGX series.

Anyways, would you like me to resend the series to Cc the aforementioned
lists?

> Is the entire series aside from patch 12 available in a public tree
> somewhere?

I pushed tag 'sgx-lsm-v3' to https://github.com/sean-jc/linux.git.

> Ultimately we'll want additions to the selinux-testsuite that exercise each
> of the new permissions, both a permission denied scenario and a permission
> allowed scenario.
Stephen Smalley June 18, 2019, 3:13 p.m. UTC | #3
On 6/18/19 9:55 AM, Sean Christopherson wrote:
> On Tue, Jun 18, 2019 at 09:38:19AM -0400, Stephen Smalley wrote:
>> On 6/17/19 6:24 PM, Sean Christopherson wrote:
>>> My original plan was for my next RFC to be an implementation of Andy's
>>> proposed "dynamic tracking" model.  I actually finished the tracking
>>> portion, but was completely flummoxed by the auditing[1].  Since Cedric's
>>> RFC is essentially a variation of the dynamic tracking model, it too has
>>> the same auditing complexities.  End result, I ended back at the "make
>>> userspace state its intentions" approach.
>>>
>>> Except for patch 12 (see below), the SGX changes have been fully tested,
>>> including updating the kernel's selftest as well as my own fork of (an old
>>> version of) Intel's SDK to use the new UAPI.  The LSM changes have been
>>> smoke tested, but I haven't actually configured AppArmor or SELinux to
>>> verify the permissions work as intended.
>>
>> Was dropping linux-security-module and selinux lists intentional for this
>> RFC? Not recommended.
> 
> Yes, my thought was to keep the noise to the sgx list until we at least
> agree on a direction for the SGX UAPI.  I am fully expecting that whatever
> LSM and SELinux patches we end up with will go through a lot more scrutiny
> when Jarkko sends them with his SGX series.
> 
> Anyways, would you like me to resend the series to Cc the aforementioned
> lists?

I guess it depends on how soon you plan to spin another version. If 
soon, then you can wait on the next round.  But I wouldn't wait until 
everything else is fully baked because the LSM discussion might chase 
out issues that require changes elsewhere.

> 
>> Is the entire series aside from patch 12 available in a public tree
>> somewhere?
> 
> I pushed tag 'sgx-lsm-v3' to https://github.com/sean-jc/linux.git.
> 
>> Ultimately we'll want additions to the selinux-testsuite that exercise each
>> of the new permissions, both a permission denied scenario and a permission
>> allowed scenario.
Jarkko Sakkinen June 25, 2019, 4:29 p.m. UTC | #4
On Mon, Jun 17, 2019 at 03:24:26PM -0700, Sean Christopherson wrote:

Since you are gone up until end of next week, I'm going to do some
proactive work and for those patches that I only had some feedback
concerning how the code is written etc. I'll just fix those myself and
squash. You can check when you're back and send me follow ups if you
disagree on something I made.

I'll also try to extract *only* the SRCU part from [1] and squash that
to my tree. The main reason is that I want to do my RFC for enclave
life-cycle handling on the same codebase so that we can compare and
make the best possible decision on that.

[1] https://patchwork.kernel.org/patch/11005495/

/Jarkko