mbox series

[RFC,v4,00/12] security: x86/sgx: SGX vs. LSM

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

Message

Sean Christopherson June 19, 2019, 10:23 p.m. UTC
For those of you whom I neglected to cc on v3, here's a quick recap:

  My original plan was for my next RFC to be an implementation of Andy's
  proposed "dynamic tracking" model, but I was completely flummoxed by the
  auditing[1].  Cedric's RFC has the same auditing complexities, so I
  I ended up back at the "make userspace state its intentions" approach.

There are no significant LSM changes in v4, e.g. a bug fix and some
renaming.  I'm spinning v4 early to get the cc list correct, and also
because I'm about to disappear on vacation for two weeks.

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 not directly related to LSM support.  They're included
here as the actual LSM RFC patches are essentially untestable without
them, and so that the patches apply to Jarkko's tree.  Ignore patches
1-3 unless you actually want to run code.

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 the initial upstreaming of SGX.

The full code is available at https://github.com/sean-jc/linux.git in
a few forms (tagged);

  sgx-lsm-v4        - Jarkko's full tree plus patches 1-11
  sgx-lsm-v4-eaug   - Everything above plus patch 12


<boilerplate>

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:

  91f3aa6d241d ("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.

</boilerplate>

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

v4:

  - Rename SGX__EXECMEM and SGX__EXECMOD to SGX__MAPWX and SGX_EXECDIRTY
    respectively [Stephen].

  - Fix an inverted check on IS_PRIVATE file check [Stephen].

  - Take a '__u8 prot' in SGX_IOC_ENCLAVE_ADD_PAGE [Jarkko].

  - Rebased to Jarkko's latest code base.

  - Replace patch 1 with a variant that does encl_mm tracking via
    mmu_notifier and SRCU.  Not relevant for most people, but I wanted
    to show the end state if we get rid of the per-vma tracking.

v3: https://patchwork.kernel.org/cover/11000601/

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

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

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

Sean Christopherson (12):
  x86/sgx: Use mmu_notifier.release() instead of per-vma refcounting
  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_MAPWX 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/Kconfig                         |   2 +
 arch/x86/include/uapi/asm/sgx.h          |   6 +-
 arch/x86/kernel/cpu/sgx/driver/ioctl.c   |  69 ++++--
 arch/x86/kernel/cpu/sgx/driver/main.c    | 106 ++++++++-
 arch/x86/kernel/cpu/sgx/encl.c           | 277 ++++++++++++-----------
 arch/x86/kernel/cpu/sgx/encl.h           |  22 +-
 arch/x86/kernel/cpu/sgx/reclaim.c        |  71 ++----
 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 ++-
 18 files changed, 532 insertions(+), 216 deletions(-)

Comments

Jarkko Sakkinen June 21, 2019, 1:32 a.m. UTC | #1
On Wed, Jun 19, 2019 at 03:23:49PM -0700, Sean Christopherson wrote:
> [SNAP]

The reason for delaying my proposal for encl->refcount (which I will
post as RFC and won't merge it before getting screened from you) was
that I really focused making the most well thought comments to this.

Generally I think that we need to get your SRCU changes in before
anything in the form that the patch does not anything else but only
that. Without doing that comparing changes dealing with the life-cycle
of an enclave is way too much mind bending.

/Jarkko
Jarkko Sakkinen July 5, 2019, 4:05 p.m. UTC | #2
On Wed, Jun 19, 2019 at 03:23:49PM -0700, Sean Christopherson wrote:

I still don't get why we need this whole mess and do not simply admit
that there are two distinct roles:

1. Creator
2. User

In the SELinux context Creator needs FILE__WRITE and FILE__EXECUTE but
User does not. It just gets the fd from the Creator. I'm sure that all
the SGX2 related functionality can be solved somehow in this role
playing game.

An example would be the usual case where enclave is actually a loader
that loads the actual piece of software that one wants to run. Things
simply need to be designed in a way the Creator runs the loader part.
These are non-trivial problems but oddball security model is not going
to make them disappear - on the contrary it will make designing user
space only more complicated.

I think this is classical example of when something overly complicated
is invented in the kernel only to realize that it should be solved in
the user space.

It would not be like the only use case where some kind of privileged
daemon is used for managing some a kernel provided resource.

I think a really good conclusion from this discussion that has taken two
months is to realize that nothing needs to be done in this area (except
*maybe* noexec check).

/Jarkko
Sean Christopherson July 8, 2019, 5:29 p.m. UTC | #3
On Fri, Jul 05, 2019 at 07:05:49PM +0300, Jarkko Sakkinen wrote:
> On Wed, Jun 19, 2019 at 03:23:49PM -0700, Sean Christopherson wrote:
> 
> I still don't get why we need this whole mess and do not simply admit
> that there are two distinct roles:
> 
> 1. Creator
> 2. User

Because SELinux has existing concepts of EXECMEM and EXECMOD.

> In the SELinux context Creator needs FILE__WRITE and FILE__EXECUTE but
> User does not. It just gets the fd from the Creator. I'm sure that all
> the SGX2 related functionality can be solved somehow in this role
> playing game.
> 
> An example would be the usual case where enclave is actually a loader
> that loads the actual piece of software that one wants to run. Things
> simply need to be designed in a way the Creator runs the loader part.
> These are non-trivial problems but oddball security model is not going
> to make them disappear - on the contrary it will make designing user
> space only more complicated.
> 
> I think this is classical example of when something overly complicated
> is invented in the kernel only to realize that it should be solved in
> the user space.
> 
> It would not be like the only use case where some kind of privileged
> daemon is used for managing some a kernel provided resource.
> 
> I think a really good conclusion from this discussion that has taken two
> months is to realize that nothing needs to be done in this area (except
> *maybe* noexec check).

Hmm, IMO we need to support at least equivalents to EXECMEM and EXECMOD.

That being said, we can do so without functional changes to the SGX uapi,
e.g. add reserved fields so that the initial uapi can be extended *if* we
decide to go with the "userspace provides maximal protections" path, and
use the EPCM permissions as the maximal protections for the initial
upstreaming.

That'd give us a minimal implemenation for initial upstreaming and would
eliminate Cedric's blocking complaint.  The "whole mess" of whitelisting,
blacklisting and SGX2 support would be deferred until post-upstreaming.
Xing, Cedric July 8, 2019, 5:33 p.m. UTC | #4
On 7/8/2019 10:29 AM, Sean Christopherson wrote:
> On Fri, Jul 05, 2019 at 07:05:49PM +0300, Jarkko Sakkinen wrote:
>> On Wed, Jun 19, 2019 at 03:23:49PM -0700, Sean Christopherson wrote:
>>
>> I still don't get why we need this whole mess and do not simply admit
>> that there are two distinct roles:
>>
>> 1. Creator
>> 2. User
> 
> Because SELinux has existing concepts of EXECMEM and EXECMOD.
> 
>> In the SELinux context Creator needs FILE__WRITE and FILE__EXECUTE but
>> User does not. It just gets the fd from the Creator. I'm sure that all
>> the SGX2 related functionality can be solved somehow in this role
>> playing game.
>>
>> An example would be the usual case where enclave is actually a loader
>> that loads the actual piece of software that one wants to run. Things
>> simply need to be designed in a way the Creator runs the loader part.
>> These are non-trivial problems but oddball security model is not going
>> to make them disappear - on the contrary it will make designing user
>> space only more complicated.
>>
>> I think this is classical example of when something overly complicated
>> is invented in the kernel only to realize that it should be solved in
>> the user space.

Are you talking about changing enclave loaders in user mode? That'd 
break all existing code. I don't think we shall ever consider this approach.

>>
>> It would not be like the only use case where some kind of privileged
>> daemon is used for managing some a kernel provided resource.
>>
>> I think a really good conclusion from this discussion that has taken two
>> months is to realize that nothing needs to be done in this area (except
>> *maybe* noexec check).
> 
> Hmm, IMO we need to support at least equivalents to EXECMEM and EXECMOD.
> 
> That being said, we can do so without functional changes to the SGX uapi,
> e.g. add reserved fields so that the initial uapi can be extended *if* we
> decide to go with the "userspace provides maximal protections" path, and
> use the EPCM permissions as the maximal protections for the initial
> upstreaming.
> 
> That'd give us a minimal implemenation for initial upstreaming and would
> eliminate Cedric's blocking complaint.  The "whole mess" of whitelisting,
> blacklisting and SGX2 support would be deferred until post-upstreaming.
>
Jarkko Sakkinen July 9, 2019, 4:22 p.m. UTC | #5
On Mon, Jul 08, 2019 at 10:29:30AM -0700, Sean Christopherson wrote:
> On Fri, Jul 05, 2019 at 07:05:49PM +0300, Jarkko Sakkinen wrote:
> > On Wed, Jun 19, 2019 at 03:23:49PM -0700, Sean Christopherson wrote:
> > 
> > I still don't get why we need this whole mess and do not simply admit
> > that there are two distinct roles:
> > 
> > 1. Creator
> > 2. User
> 
> Because SELinux has existing concepts of EXECMEM and EXECMOD.

What is the official documentation for those? I've only found some
explanations from discussions and some RHEL sysadmin guides.

> That being said, we can do so without functional changes to the SGX uapi,
> e.g. add reserved fields so that the initial uapi can be extended *if* we
> decide to go with the "userspace provides maximal protections" path, and
> use the EPCM permissions as the maximal protections for the initial
> upstreaming.
> 
> That'd give us a minimal implemenation for initial upstreaming and would
> eliminate Cedric's blocking complaint.  The "whole mess" of whitelisting,
> blacklisting and SGX2 support would be deferred until post-upstreaming.

I'd like that approach more too.

/Jarkko
Sean Christopherson July 9, 2019, 5:09 p.m. UTC | #6
On Tue, Jul 09, 2019 at 07:22:03PM +0300, Jarkko Sakkinen wrote:
> On Mon, Jul 08, 2019 at 10:29:30AM -0700, Sean Christopherson wrote:
> > On Fri, Jul 05, 2019 at 07:05:49PM +0300, Jarkko Sakkinen wrote:
> > > On Wed, Jun 19, 2019 at 03:23:49PM -0700, Sean Christopherson wrote:
> > > 
> > > I still don't get why we need this whole mess and do not simply admit
> > > that there are two distinct roles:
> > > 
> > > 1. Creator
> > > 2. User
> > 
> > Because SELinux has existing concepts of EXECMEM and EXECMOD.
> 
> What is the official documentation for those? I've only found some
> explanations from discussions and some RHEL sysadmin guides.

No clue.  My knowledge was gleaned from the code and from Stephen's
feedback.


The high level breakdown:

  - FILE__EXECUTE: required to gain X on a mapping to a regular file


  - FILE__EXECUTE + FILE__WRITE: required to gain WX or W->X on a shared
                                 mapping to a regular file

  - FILE__EXECMOD: required to gain W->X on a private mapping of a regular
                   file

  - PROCESS__EXECMEM: required to gain WX on a private mapping to a regular
                      file, OR to gain X on an anonymous mapping.


Translating those to SGX, with a lot of input from Stephen, I ended up
with the following:

  - FILE__ENCLAVE_EXECUTE: equivalent to FILE__EXECUTE, required to gain X
                           on an enclave page loaded from a regular file

  - PROCESS2__ENCLAVE_EXECDIRTY: hybrid of EXECMOD and EXECUTE+WRITE,
                                 required to gain W->X on an enclave page

  - PROCESS2__ENCLAVE_EXECANON: subset of EXECMEM, required to gain X on
                                an enclave page that is loaded from an
                                anonymous mapping
                       
  - PROCESS2__ENCLAVE_MAPWX: subset of EXECMEM, required to gain WX on an
                             enclave page



> > That being said, we can do so without functional changes to the SGX uapi,
> > e.g. add reserved fields so that the initial uapi can be extended *if* we
> > decide to go with the "userspace provides maximal protections" path, and
> > use the EPCM permissions as the maximal protections for the initial
> > upstreaming.
> > 
> > That'd give us a minimal implemenation for initial upstreaming and would
> > eliminate Cedric's blocking complaint.  The "whole mess" of whitelisting,
> > blacklisting and SGX2 support would be deferred until post-upstreaming.
> 
> I'd like that approach more too.
> 
> /Jarkko
Xing, Cedric July 9, 2019, 8:41 p.m. UTC | #7
On 7/9/2019 10:09 AM, Sean Christopherson wrote:
> On Tue, Jul 09, 2019 at 07:22:03PM +0300, Jarkko Sakkinen wrote:
>> On Mon, Jul 08, 2019 at 10:29:30AM -0700, Sean Christopherson wrote:
>>> On Fri, Jul 05, 2019 at 07:05:49PM +0300, Jarkko Sakkinen wrote:
>>>> On Wed, Jun 19, 2019 at 03:23:49PM -0700, Sean Christopherson wrote:
>>>>
>>>> I still don't get why we need this whole mess and do not simply admit
>>>> that there are two distinct roles:
>>>>
>>>> 1. Creator
>>>> 2. User
>>>
>>> Because SELinux has existing concepts of EXECMEM and EXECMOD.
>>
>> What is the official documentation for those? I've only found some
>> explanations from discussions and some RHEL sysadmin guides.
> 
> No clue.  My knowledge was gleaned from the code and from Stephen's
> feedback.
> 
> 
> The high level breakdown:
> 
>    - FILE__EXECUTE: required to gain X on a mapping to a regular file
> 
> 
>    - FILE__EXECUTE + FILE__WRITE: required to gain WX or W->X on a shared
>                                   mapping to a regular file
> 
>    - FILE__EXECMOD: required to gain W->X on a private mapping of a regular
>                     file
> 
>    - PROCESS__EXECMEM: required to gain WX on a private mapping to a regular
>                        file, OR to gain X on an anonymous mapping.
> 
> 
> Translating those to SGX, with a lot of input from Stephen, I ended up
> with the following:
> 
>    - FILE__ENCLAVE_EXECUTE: equivalent to FILE__EXECUTE, required to gain X
>                             on an enclave page loaded from a regular file
> 
>    - PROCESS2__ENCLAVE_EXECDIRTY: hybrid of EXECMOD and EXECUTE+WRITE,
>                                   required to gain W->X on an enclave page

EXECMOD basically indicates a file containing self-modifying code. Your 
ENCLAVE_EXECDIRTY is however a process permission, which is illogical.

>    - PROCESS2__ENCLAVE_EXECANON: subset of EXECMEM, required to gain X on
>                                  an enclave page that is loaded from an
>                                  anonymous mapping
>                         
>    - PROCESS2__ENCLAVE_MAPWX: subset of EXECMEM, required to gain WX on an
>                               enclave page
> 
> 
> 
>>> That being said, we can do so without functional changes to the SGX uapi,
>>> e.g. add reserved fields so that the initial uapi can be extended *if* we
>>> decide to go with the "userspace provides maximal protections" path, and
>>> use the EPCM permissions as the maximal protections for the initial
>>> upstreaming.
>>>
>>> That'd give us a minimal implemenation for initial upstreaming and would
>>> eliminate Cedric's blocking complaint.  The "whole mess" of whitelisting,
>>> blacklisting and SGX2 support would be deferred until post-upstreaming.
>>
>> I'd like that approach more too.
>>
>> /Jarkko
Sean Christopherson July 9, 2019, 10:25 p.m. UTC | #8
On Tue, Jul 09, 2019 at 01:41:28PM -0700, Xing, Cedric wrote:
> On 7/9/2019 10:09 AM, Sean Christopherson wrote:
> >Translating those to SGX, with a lot of input from Stephen, I ended up
> >with the following:
> >
> >   - FILE__ENCLAVE_EXECUTE: equivalent to FILE__EXECUTE, required to gain X
> >                            on an enclave page loaded from a regular file
> >
> >   - PROCESS2__ENCLAVE_EXECDIRTY: hybrid of EXECMOD and EXECUTE+WRITE,
> >                                  required to gain W->X on an enclave page
> 
> EXECMOD basically indicates a file containing self-modifying code. Your
> ENCLAVE_EXECDIRTY is however a process permission, which is illogical.

How is it illogical?  If a PROCESS wants to EXECute a DIRTY ENCLAVE page,
then it needs PROCESS2__ENCLAVE_EXECDIRTY.

FILE__EXECMOD on /dev/sgx/enclave is a process permission masquerading as
a file permission, let's call it what it is.
Xing, Cedric July 9, 2019, 11:11 p.m. UTC | #9
On 7/9/2019 3:25 PM, Sean Christopherson wrote:
> On Tue, Jul 09, 2019 at 01:41:28PM -0700, Xing, Cedric wrote:
>> On 7/9/2019 10:09 AM, Sean Christopherson wrote:
>>> Translating those to SGX, with a lot of input from Stephen, I ended up
>>> with the following:
>>>
>>>    - FILE__ENCLAVE_EXECUTE: equivalent to FILE__EXECUTE, required to gain X
>>>                             on an enclave page loaded from a regular file
>>>
>>>    - PROCESS2__ENCLAVE_EXECDIRTY: hybrid of EXECMOD and EXECUTE+WRITE,
>>>                                   required to gain W->X on an enclave page
>>
>> EXECMOD basically indicates a file containing self-modifying code. Your
>> ENCLAVE_EXECDIRTY is however a process permission, which is illogical.
> 
> How is it illogical?  If a PROCESS wants to EXECute a DIRTY ENCLAVE page,
> then it needs PROCESS2__ENCLAVE_EXECDIRTY
Just think of the purpose of FILE__EXECMOD. It indicates to LSM the file 
has self-modifying code, hence W->X transition should be considered 
"normal" and allowed, regardless which process that file is loaded into.

The same thing for enclaves here. Whether an enclave contains 
self-modifying code is specific to that enclave, regardless which 
process it is loaded into.

But what are you doing is quite the opposite, and that's I mean by 
"illogical".
Dr. Greg July 10, 2019, 1:28 a.m. UTC | #10
On Mon, Jul 08, 2019 at 10:29:30AM -0700, Sean Christopherson wrote:

Good evening to everyone.

> That being said, we can do so without functional changes to the SGX
> uapi, e.g. add reserved fields so that the initial uapi can be
> extended *if* we decide to go with the "userspace provides maximal
> protections" path, and use the EPCM permissions as the maximal
> protections for the initial upstreaming.
>
> That'd give us a minimal implemenation for initial upstreaming and
> would eliminate Cedric's blocking complaint.  The "whole mess" of
> whitelisting, blacklisting and SGX2 support would be deferred until
> post-upstreaming.

Are we convinced the 'mess' will be any easier to clean up after the
driver is upstreamed?

The primary problem is that we haven't addressed the issue of what
this technology is designed to do and its implications with respect to
the kernel.  As a result we are attempting to implement controls which
we are comfortable with and understand rather then those that are
relevant.

Have a good evening.

Dr. Greg

As always,
Dr. Greg Wettstein, Ph.D, Worker
IDfusion, LLC               Implementing SGX secured and modeled
4206 N. 19th Ave.           intelligent network endpoints.
Fargo, ND  58102
PH: 701-281-1686            EMAIL: greg@idfusion.net
------------------------------------------------------------------------------
"Courage is not the absence of fear, but rather the judgement that
 something else is more important than fear."
                                -- Ambrose Redmoon
Xing, Cedric July 10, 2019, 2:04 a.m. UTC | #11
On 7/9/2019 6:28 PM, Dr. Greg wrote:
> On Mon, Jul 08, 2019 at 10:29:30AM -0700, Sean Christopherson wrote:
> 
> Good evening to everyone.
> 
>> That being said, we can do so without functional changes to the SGX
>> uapi, e.g. add reserved fields so that the initial uapi can be
>> extended *if* we decide to go with the "userspace provides maximal
>> protections" path, and use the EPCM permissions as the maximal
>> protections for the initial upstreaming.
>>
>> That'd give us a minimal implemenation for initial upstreaming and
>> would eliminate Cedric's blocking complaint.  The "whole mess" of
>> whitelisting, blacklisting and SGX2 support would be deferred until
>> post-upstreaming.
> 
> Are we convinced the 'mess' will be any easier to clean up after the
> driver is upstreamed?
> 
> The primary problem is that we haven't addressed the issue of what
> this technology is designed to do and its implications with respect to
> the kernel.  As a result we are attempting to implement controls which
> we are comfortable with and understand rather then those that are
> relevant.

I don't think it's about easier or harder to clean up the mess, but a 
divide-and-conquer strategy. After all, SGX and LSM are kind of 
orthogonal as long as SGX doesn't compromise the protection provided by LSM.

Let's step back and look at what started this lengthy discussion. The 
primary problem of v20 was that the SGX module allows executable enclave 
pages to be created from non-executable regular pages, which could be 
exploited by adversaries to grant executable permissions to pages that 
would otherwise be denied without SGX. And that could be fixed simply by 
capping EPCM permissions to whatever allowed on the source page, without 
touching LSM. Of course the drawback is loss of functionality - i.e. 
self-modifying enclaves cannot be loaded unless the calling process has 
EXECMEM. But that should suffice, as most SGX1 applications don't 
contain self-modifying code anyway.

Then we could switch our focus back to LSM and work out what's relevant, 
especially for SGX2 and beyond.

> Have a good evening.
> 
> Dr. Greg
> 
> As always,
> Dr. Greg Wettstein, Ph.D, Worker
> IDfusion, LLC               Implementing SGX secured and modeled
> 4206 N. 19th Ave.           intelligent network endpoints.
> Fargo, ND  58102
> PH: 701-281-1686            EMAIL: greg@idfusion.net
> ------------------------------------------------------------------------------
> "Courage is not the absence of fear, but rather the judgement that
>   something else is more important than fear."
>                                  -- Ambrose Redmoon
>
Jethro Beekman July 10, 2019, 3:21 a.m. UTC | #12
On 2019-07-08 10:29, Sean Christopherson wrote:
> On Fri, Jul 05, 2019 at 07:05:49PM +0300, Jarkko Sakkinen wrote:
>> On Wed, Jun 19, 2019 at 03:23:49PM -0700, Sean Christopherson wrote:
>>
>> I still don't get why we need this whole mess and do not simply admit
>> that there are two distinct roles:
>>
>> 1. Creator
>> 2. User
> 
> Because SELinux has existing concepts of EXECMEM and EXECMOD.
> 
>> In the SELinux context Creator needs FILE__WRITE and FILE__EXECUTE but
>> User does not. It just gets the fd from the Creator. I'm sure that all
>> the SGX2 related functionality can be solved somehow in this role
>> playing game.
>>
>> An example would be the usual case where enclave is actually a loader
>> that loads the actual piece of software that one wants to run. Things
>> simply need to be designed in a way the Creator runs the loader part.
>> These are non-trivial problems but oddball security model is not going
>> to make them disappear - on the contrary it will make designing user
>> space only more complicated.
>>
>> I think this is classical example of when something overly complicated
>> is invented in the kernel only to realize that it should be solved in
>> the user space.
>>
>> It would not be like the only use case where some kind of privileged
>> daemon is used for managing some a kernel provided resource.
>>
>> I think a really good conclusion from this discussion that has taken two
>> months is to realize that nothing needs to be done in this area (except
>> *maybe* noexec check).
> 
> Hmm, IMO we need to support at least equivalents to EXECMEM and EXECMOD.
> 
> That being said, we can do so without functional changes to the SGX uapi,
> e.g. add reserved fields so that the initial uapi can be extended *if* we
> decide to go with the "userspace provides maximal protections" path, and
> use the EPCM permissions as the maximal protections for the initial
> upstreaming.

Why do you need to add reserved fields now? Isn't this what 
incorporating the struct size in the ioctl number is for?

--
Jethro Beekman | Fortanix
Sean Christopherson July 10, 2019, 4:57 p.m. UTC | #13
On Tue, Jul 09, 2019 at 04:11:08PM -0700, Xing, Cedric wrote:
> On 7/9/2019 3:25 PM, Sean Christopherson wrote:
> >On Tue, Jul 09, 2019 at 01:41:28PM -0700, Xing, Cedric wrote:
> >>On 7/9/2019 10:09 AM, Sean Christopherson wrote:
> >>>Translating those to SGX, with a lot of input from Stephen, I ended up
> >>>with the following:
> >>>
> >>>   - FILE__ENCLAVE_EXECUTE: equivalent to FILE__EXECUTE, required to gain X
> >>>                            on an enclave page loaded from a regular file
> >>>
> >>>   - PROCESS2__ENCLAVE_EXECDIRTY: hybrid of EXECMOD and EXECUTE+WRITE,
> >>>                                  required to gain W->X on an enclave page
> >>
> >>EXECMOD basically indicates a file containing self-modifying code. Your
> >>ENCLAVE_EXECDIRTY is however a process permission, which is illogical.
> >
> >How is it illogical?  If a PROCESS wants to EXECute a DIRTY ENCLAVE page,
> >then it needs PROCESS2__ENCLAVE_EXECDIRTY
> Just think of the purpose of FILE__EXECMOD. It indicates to LSM the file has
> self-modifying code, hence W->X transition should be considered "normal" and
> allowed, regardless which process that file is loaded into.
> 
> The same thing for enclaves here. Whether an enclave contains self-modifying
> code is specific to that enclave, regardless which process it is loaded
> into.
> 
> But what are you doing is quite the opposite, and that's I mean by
> "illogical".

Ah.  My intent was to minimize the number of new labels, and because W->X
scenarios are not guaranteed to be backed by a file, I went with a per
process permission.  Ditto for EXECANON.  I'm not opposed to also having a
per file permission that can be used when possible.

Something like this?  And maybe merge EXECANON and EXECDIRTY into a single
permission?

Depending on whether sigstruct is required to be in a pinned file, EAUG
pages would need either EXECDIRTY or EXECMOD.

static int selinux_enclave_load(struct vm_area_struct *vma, unsigned long prot,
				bool measured)
{
	const struct cred *cred = current_cred();
	u32 sid = cred_sid(cred);
	int ret;

	/* Currently supported only in noexec kernels. */
	WARN_ON_ONCE(!default_noexec);

	/* Only executable enclave pages are restricted in any way. */
	if (!(prot & PROT_EXEC))
		return 0;

	if (!measured) {
		ret = enclave_has_perm(sid, PROCESS2__ENCLAVE_EXECUNMR);
		if (ret)
			goto out;
	}

	if (!vma->vm_file || IS_PRIVATE(file_inode(vma->vm_file))) {
		ret = enclave_has_perm(sid, PROCESS2__ENCLAVE_EXECANON);
		if (ret)
			goto out;

		/* Ability to do W->X within the enclave. */
		if (prot & PROT_WRITE)
			ret = enclave_has_perm(sid,
					       PROCESS2__ENCLAVE_EXECDIRTY);
	} else {
		ret = file_has_perm(cred, vma->vm_file, FILE__ENCLAVE_EXECUTE);
		if (ret)
			goto out;

		/*
		 * Load code from a modified private mapping, or from any file
		 * mapping with the ability to do W->X within the enclave.
		 */
		if (vma->anon_vma || (prot & PROT_WRITE))
			ret = file_has_perm(cred, vma->vm_file,
					    FILE__ENCLAVE_EXECMOD);
	}

out:
	return ret;
}
Jarkko Sakkinen July 10, 2019, 8:19 p.m. UTC | #14
On Tue, Jul 09, 2019 at 10:09:17AM -0700, Sean Christopherson wrote:
> On Tue, Jul 09, 2019 at 07:22:03PM +0300, Jarkko Sakkinen wrote:
> > On Mon, Jul 08, 2019 at 10:29:30AM -0700, Sean Christopherson wrote:
> > > On Fri, Jul 05, 2019 at 07:05:49PM +0300, Jarkko Sakkinen wrote:
> > > > On Wed, Jun 19, 2019 at 03:23:49PM -0700, Sean Christopherson wrote:
> > > > 
> > > > I still don't get why we need this whole mess and do not simply admit
> > > > that there are two distinct roles:
> > > > 
> > > > 1. Creator
> > > > 2. User
> > > 
> > > Because SELinux has existing concepts of EXECMEM and EXECMOD.
> > 
> > What is the official documentation for those? I've only found some
> > explanations from discussions and some RHEL sysadmin guides.
> 
> No clue.  My knowledge was gleaned from the code and from Stephen's
> feedback.

OK, thanks for elaboration. Got nailed some details I was missing :-)

Anyway, to accompany your code changes I'm eager to document this not
least because it is a good peer test that this all make sense (you
cannot "unit test" a security model so that is the next best thing).

Still, we need a documentation reference to reflect the narrative
for these changes, seriously. It cannot be that SELinux is widely
deployed and it completely lacks documentation for its basic
objects, can it?

/Jarkko
Sean Christopherson July 10, 2019, 8:31 p.m. UTC | #15
On Wed, Jul 10, 2019 at 11:19:30PM +0300, Jarkko Sakkinen wrote:
> On Tue, Jul 09, 2019 at 10:09:17AM -0700, Sean Christopherson wrote:
> > On Tue, Jul 09, 2019 at 07:22:03PM +0300, Jarkko Sakkinen wrote:
> > > On Mon, Jul 08, 2019 at 10:29:30AM -0700, Sean Christopherson wrote:
> > > > On Fri, Jul 05, 2019 at 07:05:49PM +0300, Jarkko Sakkinen wrote:
> > > > > On Wed, Jun 19, 2019 at 03:23:49PM -0700, Sean Christopherson wrote:
> > > > > 
> > > > > I still don't get why we need this whole mess and do not simply admit
> > > > > that there are two distinct roles:
> > > > > 
> > > > > 1. Creator
> > > > > 2. User
> > > > 
> > > > Because SELinux has existing concepts of EXECMEM and EXECMOD.
> > > 
> > > What is the official documentation for those? I've only found some
> > > explanations from discussions and some RHEL sysadmin guides.
> > 
> > No clue.  My knowledge was gleaned from the code and from Stephen's
> > feedback.
> 
> OK, thanks for elaboration. Got nailed some details I was missing :-)
> 
> Anyway, to accompany your code changes I'm eager to document this not
> least because it is a good peer test that this all make sense (you
> cannot "unit test" a security model so that is the next best thing).
> 
> Still, we need a documentation reference to reflect the narrative
> for these changes, seriously. It cannot be that SELinux is widely
> deployed and it completely lacks documentation for its basic
> objects, can it?

The vast majority of documentation I've found is on *using* SELinux,
e.g. writing policies and whatnot.  I haven't found anything on its
internal details, although I admitedly haven't looked all that hard.
Jarkko Sakkinen July 10, 2019, 10 p.m. UTC | #16
On Wed, Jul 10, 2019 at 11:19:30PM +0300, Jarkko Sakkinen wrote:
> Still, we need a documentation reference to reflect the narrative
> for these changes, seriously. It cannot be that SELinux is widely
> deployed and it completely lacks documentation for its basic
> objects, can it?

I found one good reference:

https://selinuxpTroject.org/page/ObjectClassesPerms

It describes EXECMOD as:

"Make executable a file mapping that has been modified by copy-on-write.
(Text relocation)"

This makes me wonder how EXECMOD even connects to this discussion?
Enclave is never a COW mapping. Seems like there is a huge diff on
how SELinux's official documentation describes it and how it is
described here...

/Jarkko
Jarkko Sakkinen July 10, 2019, 10:16 p.m. UTC | #17
Just some questions on these.

On Tue, Jul 09, 2019 at 10:09:17AM -0700, Sean Christopherson wrote:
>   - FILE__ENCLAVE_EXECUTE: equivalent to FILE__EXECUTE, required to gain X
>                            on an enclave page loaded from a regular file

One thing that I have hard time to perceive is that whether the process
or the target object has them. So would this be in the files extended
attribute or does process need to possess this or both?


>   - PROCESS2__ENCLAVE_EXECDIRTY: hybrid of EXECMOD and EXECUTE+WRITE,
>                                  required to gain W->X on an enclave page

Still puzzling with EXECMOD given that how it is documented in
https://selinuxproject.org/page/ObjectClassesPerms. If anything in that
document is out of date, would be nice if it was updated.

>   - PROCESS2__ENCLAVE_EXECANON: subset of EXECMEM, required to gain X on
>                                 an enclave page that is loaded from an
>                                 anonymous mapping
>
>   - PROCESS2__ENCLAVE_MAPWX: subset of EXECMEM, required to gain WX on an
>                              enclave page

I guess these three belong to the process and are not attached to file.

How in SELinux anyway process in the first place acquires any SELinux
permissions? I guess getty or whatever login process can set its perms
before setuid() et al somehow (I don't know how) because they run as
root?

/Jarkko
Xing, Cedric July 10, 2019, 11:16 p.m. UTC | #18
On 7/10/2019 3:16 PM, Jarkko Sakkinen wrote:
> Just some questions on these.
> 
> On Tue, Jul 09, 2019 at 10:09:17AM -0700, Sean Christopherson wrote:
>>    - FILE__ENCLAVE_EXECUTE: equivalent to FILE__EXECUTE, required to gain X
>>                             on an enclave page loaded from a regular file
> 
> One thing that I have hard time to perceive is that whether the process
> or the target object has them. So would this be in the files extended
> attribute or does process need to possess this or both?

The target object.

>>    - PROCESS2__ENCLAVE_EXECDIRTY: hybrid of EXECMOD and EXECUTE+WRITE,
>>                                   required to gain W->X on an enclave page
> 
> Still puzzling with EXECMOD given that how it is documented in
> https://selinuxproject.org/page/ObjectClassesPerms. If anything in that
> document is out of date, would be nice if it was updated.

If you search for "EXECMOD" in security/selinux/hooks.c in the latest 
(Linux-5.2) master, you'll find only one occurrence - at line 3702.

The logic over there, if translated into English, basically says 
FILE__EXECMOD is required (on the backing file) if mprotect() is called 
to request X on a private file mapping that has been modified by the 
calling process. That's what Sean meant by "W->X".

EXCLAVE_EXECDIRTY is similar to EXECMOD but because of his "maximal 
protection" model, LSM couldn't distinguish between "W->X" and "X->W", 
hence those two are collapsed into a single case - WX in "maximal 
protection".

>>    - PROCESS2__ENCLAVE_EXECANON: subset of EXECMEM, required to gain X on
>>                                  an enclave page that is loaded from an
>>                                  anonymous mapping
>>
>>    - PROCESS2__ENCLAVE_MAPWX: subset of EXECMEM, required to gain WX on an
>>                               enclave page
> 
> I guess these three belong to the process and are not attached to file.

Correct.

ENCLAVE_EXECANON basically means the calling process doesn't care what 
permissions given to enclave pages as the SIGSTRUCT alone is considered 
sufficient validation. This has a security impact process wide so shall 
be a process permission.

ENCLAVE_{EXECDIRTY|MAPWX} express enclave specific 
requirements/behaviors and IMO shall be enclave permissions, probably 
manifested as file permissions on the file containing SIGSTRUCT. Sean 
was taking a shortcut to make them process scope in order to avoid 
keeping the SIGSTRUCT file around, which was what I criticized as 
"illogical".

> How in SELinux anyway process in the first place acquires any SELinux
> permissions? I guess getty or whatever login process can set its perms
> before setuid() et al somehow (I don't know how) because they run as
> root?
> 
> /Jarkko
>
Jarkko Sakkinen July 11, 2019, 9:06 a.m. UTC | #19
On Wed, Jul 10, 2019 at 01:31:04PM -0700, Sean Christopherson wrote:
> On Wed, Jul 10, 2019 at 11:19:30PM +0300, Jarkko Sakkinen wrote:
> > On Tue, Jul 09, 2019 at 10:09:17AM -0700, Sean Christopherson wrote:
> > > On Tue, Jul 09, 2019 at 07:22:03PM +0300, Jarkko Sakkinen wrote:
> > > > On Mon, Jul 08, 2019 at 10:29:30AM -0700, Sean Christopherson wrote:
> > > > > On Fri, Jul 05, 2019 at 07:05:49PM +0300, Jarkko Sakkinen wrote:
> > > > > > On Wed, Jun 19, 2019 at 03:23:49PM -0700, Sean Christopherson wrote:
> > > > > > 
> > > > > > I still don't get why we need this whole mess and do not simply admit
> > > > > > that there are two distinct roles:
> > > > > > 
> > > > > > 1. Creator
> > > > > > 2. User
> > > > > 
> > > > > Because SELinux has existing concepts of EXECMEM and EXECMOD.
> > > > 
> > > > What is the official documentation for those? I've only found some
> > > > explanations from discussions and some RHEL sysadmin guides.
> > > 
> > > No clue.  My knowledge was gleaned from the code and from Stephen's
> > > feedback.
> > 
> > OK, thanks for elaboration. Got nailed some details I was missing :-)
> > 
> > Anyway, to accompany your code changes I'm eager to document this not
> > least because it is a good peer test that this all make sense (you
> > cannot "unit test" a security model so that is the next best thing).
> > 
> > Still, we need a documentation reference to reflect the narrative
> > for these changes, seriously. It cannot be that SELinux is widely
> > deployed and it completely lacks documentation for its basic
> > objects, can it?
> 
> The vast majority of documentation I've found is on *using* SELinux,
> e.g. writing policies and whatnot.  I haven't found anything on its
> internal details, although I admitedly haven't looked all that hard.

I think these are the best references.

Object classes:

https://selinuxproject.org/page/ObjectClassesPerms

Background/concepts:

https://selinuxproject.org/page/Category:Notebook#Notebook_Sections

/Jarkko
Jarkko Sakkinen July 11, 2019, 9:26 a.m. UTC | #20
On Wed, Jul 10, 2019 at 04:16:42PM -0700, Xing, Cedric wrote:
> > Still puzzling with EXECMOD given that how it is documented in
> > https://selinuxproject.org/page/ObjectClassesPerms. If anything in that
> > document is out of date, would be nice if it was updated.
> 
> If you search for "EXECMOD" in security/selinux/hooks.c in the latest
> (Linux-5.2) master, you'll find only one occurrence - at line 3702.
> 
> The logic over there, if translated into English, basically says
> FILE__EXECMOD is required (on the backing file) if mprotect() is called to
> request X on a private file mapping that has been modified by the calling
> process. That's what Sean meant by "W->X".

Looking at that part of code, there is this comment:

/*
 * We are making executable a file mapping that has
 * had some COW done. Since pages might have been
 * written, check ability to execute the possibly
 * modified content.  This typically should only
 * occur for text relocations.
 */

There is no COW done with enclaves, never. Thus, EXECMOD does not
connect in any possible way to SGX. OR, that comment is false.

Which one is it?

Also the official documentation for SELinux speaks only about COW
mappings.

Also the condition supports all this as a *private* file mapping ends up
to the anon_vma list when it gets written. We have a *shared* file
mapping

Nothing that you say makes sense to me, sorry...

/Jarkko
Stephen Smalley July 11, 2019, 2:32 p.m. UTC | #21
On 7/11/19 5:26 AM, Jarkko Sakkinen wrote:
> On Wed, Jul 10, 2019 at 04:16:42PM -0700, Xing, Cedric wrote:
>>> Still puzzling with EXECMOD given that how it is documented in
>>> https://selinuxproject.org/page/ObjectClassesPerms. If anything in that
>>> document is out of date, would be nice if it was updated.
>>
>> If you search for "EXECMOD" in security/selinux/hooks.c in the latest
>> (Linux-5.2) master, you'll find only one occurrence - at line 3702.
>>
>> The logic over there, if translated into English, basically says
>> FILE__EXECMOD is required (on the backing file) if mprotect() is called to
>> request X on a private file mapping that has been modified by the calling
>> process. That's what Sean meant by "W->X".
> 
> Looking at that part of code, there is this comment:
> 
> /*
>   * We are making executable a file mapping that has
>   * had some COW done. Since pages might have been
>   * written, check ability to execute the possibly
>   * modified content.  This typically should only
>   * occur for text relocations.
>   */
> 
> There is no COW done with enclaves, never. Thus, EXECMOD does not
> connect in any possible way to SGX. OR, that comment is false.
> 
> Which one is it?
> 
> Also the official documentation for SELinux speaks only about COW
> mappings.
> 
> Also the condition supports all this as a *private* file mapping ends up
> to the anon_vma list when it gets written. We have a *shared* file
> mapping
> 
> Nothing that you say makes sense to me, sorry...

The existing permissions don't map cleanly to SGX but I think Sean and 
Cedric were trying to make a best-effort approximation to the underlying 
concepts in a manner that permits control over the introduction of 
executable content.

Sure, the existing EXECMOD check is only applied today when there is an 
attempt to make executable a previously modified (detected based on COW 
having occurred) private file mapping.  But the general notion of 
controlling the ability to execute modified content is still meaningful.

In the case of regular files, having both FILE__WRITE and FILE__EXECUTE 
to the file is sufficient because that implies the ability to execute 
modified content.  And those FILE__* checks predated the introduction of 
EXECMOD and EXECMEM.

The mapping of /dev/sgx/enclave doesn't really fit existing categories 
because it doesn't provide the same semantics as a shared mapping of a 
regular file.  Userspace will always need both FILE__WRITE and 
FILE__EXECUTE to /dev/sgx/enclave.
Jarkko Sakkinen July 11, 2019, 5:51 p.m. UTC | #22
On Thu, Jul 11, 2019 at 10:32:41AM -0400, Stephen Smalley wrote:
> The existing permissions don't map cleanly to SGX but I think Sean and
> Cedric were trying to make a best-effort approximation to the underlying
> concepts in a manner that permits control over the introduction of
> executable content.
> 
> Sure, the existing EXECMOD check is only applied today when there is an
> attempt to make executable a previously modified (detected based on COW
> having occurred) private file mapping.  But the general notion of
> controlling the ability to execute modified content is still meaningful.

OK to summarize EXECMOD does not connect with SGX in any possible way
but SGX needs something that mimics EXECMOD behaviour?

And the same probably goes for EXECMEM, which is also private mapping
related concept.

> In the case of regular files, having both FILE__WRITE and FILE__EXECUTE to
> the file is sufficient because that implies the ability to execute modified
> content.  And those FILE__* checks predated the introduction of EXECMOD and
> EXECMEM.

Right.

> The mapping of /dev/sgx/enclave doesn't really fit existing categories
> because it doesn't provide the same semantics as a shared mapping of a
> regular file.  Userspace will always need both FILE__WRITE and FILE__EXECUTE
> to /dev/sgx/enclave.

Right.

Yeah, that has been the confusing part for me that they've been shuffled
in the discussion together with SGX concepts and hooks like there was
any connection. Thank you for clarifying this!

/Jarkko
Xing, Cedric July 12, 2019, 12:08 a.m. UTC | #23
On 7/11/2019 10:51 AM, Jarkko Sakkinen wrote:
> On Thu, Jul 11, 2019 at 10:32:41AM -0400, Stephen Smalley wrote:
>> The existing permissions don't map cleanly to SGX but I think Sean and
>> Cedric were trying to make a best-effort approximation to the underlying
>> concepts in a manner that permits control over the introduction of
>> executable content.
>>
>> Sure, the existing EXECMOD check is only applied today when there is an
>> attempt to make executable a previously modified (detected based on COW
>> having occurred) private file mapping.  But the general notion of
>> controlling the ability to execute modified content is still meaningful.
> 
> OK to summarize EXECMOD does not connect with SGX in any possible way
> but SGX needs something that mimics EXECMOD behaviour?

Stephen may correct me if I'm wrong. EXECMOD is granted to files, to 
indicate the bearer contains self-modifying code (or text relocation). 
So if it applies the enclaves, there are two aspects of it:

(1) An enclave may be loaded from multiple image files, among which some 
may contain self-modifying code and hence would require EXECMOD on each 
of them. At runtime, W->X will be allowed/denied for pages loaded from 
image files having/not having EXECMOD.

(2) But there are pages not loaded from any files - e.g. pages EAUG'ed 
at runtime. We are trying to use the file containing SIGSTRUCT as the 
"proxy file" - i.e. EXECMOD on the proxy file indicates the enclave may 
load code into EAUG'ed pages at runtime.

(3) Well, this is not an aspect of EXECMOD. Yet there's another category 
of pages - pages EADD'ed from anonymous memory. They are different than 
EAUG'ed pages in that their contents are supplied by untrusted code. How 
to control their accesses is still being debated. My argument was that 
the source pages must be executable before they could be loaded 
executable in EPC. Andy argued that SIGSTRUCT alone could be considered 
sufficient validation on the contents in certain usages, so both Sean 
and I had proposed PROCESS2__ENCLAVE_EXECANON as a new permission. But 
for the 1st upstream I think we all agree to do just the minimum until 
real requirements come up. After all, adding is generally easier than 
taking away existing things.