On Fri, Jun 14, 2019 at 8:38 AM Sean Christopherson
[off-list ref] wrote:

On Thu, Jun 13, 2019 at 05:46:00PM -0700, Sean Christopherson wrote:

quoted

On Thu, Jun 13, 2019 at 01:02:17PM -0400, Stephen Smalley wrote:

quoted

On 6/11/19 6:02 PM, Sean Christopherson wrote:

quoted

On Tue, Jun 11, 2019 at 09:40:25AM -0400, Stephen Smalley wrote:

quoted

I haven't looked at this code closely, but it feels like a lot of
SGX-specific logic embedded into SELinux that will have to be repeated or
reused for every security module.  Does SGX not track this state itself?

SGX does track equivalent state.

There are three proposals on the table (I think):

  1. Require userspace to explicitly specificy (maximal) enclave page
     permissions at build time.  The enclave page permissions are provided
     to, and checked by, LSMs at enclave build time.

     Pros: Low-complexity kernel implementation, straightforward auditing
     Cons: Sullies the SGX UAPI to some extent, may increase complexity of
           SGX2 enclave loaders.

  2. Pre-check LSM permissions and dynamically track mappings to enclave
     pages, e.g. add an SGX mprotect() hook to restrict W->X and WX
     based on the pre-checked permissions.

     Pros: Does not impact SGX UAPI, medium kernel complexity
     Cons: Auditing is complex/weird, requires taking enclave-specific
           lock during mprotect() to query/update tracking.

  3. Implement LSM hooks in SGX to allow LSMs to track enclave regions
     from cradle to grave, but otherwise defer everything to LSMs.

     Pros: Does not impact SGX UAPI, maximum flexibility, precise auditing
     Cons: Most complex and "heaviest" kernel implementation of the three,
           pushes more SGX details into LSMs.

My RFC series[1] implements #1.  My understanding is that Andy (Lutomirski)
prefers #2.  Cedric's RFC series implements #3.

Perhaps the easiest way to make forward progress is to rule out the
options we absolutely *don't* want by focusing on the potentially blocking
issue with each option:

  #1 - SGX UAPI funkiness

  #2 - Auditing complexity, potential enclave lock contention

  #3 - Pushing SGX details into LSMs and complexity of kernel implementation


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

Given the complexity tradeoff, what is the clear motivating example for why
#1 isn't the obvious choice? That the enclave loader has no way of knowing a
priori whether the enclave will require W->X or WX?  But aren't we better
off requiring enclaves to be explicitly marked as needing such so that we
can make a more informed decision about whether to load them in the first
place?

Andy and/or Cedric, can you please weigh in with a concrete (and practical)
use case that will break if we go with #1?  The auditing issues for #2/#3
are complex to say the least...

The most significant issue I see is the following.  Consider two
cases. First, an SGX2 enclave that dynamically allocates memory but
doesn't execute code from dynamic memory.  Second, an SGX2 enclave
that *does* execute code from dynamic memory.  In #1, the untrusted
stack needs to decide whether to ALLOW_EXEC when the memory is
allocated, which means that it either needs to assume the worst or it
needs to know at allocation time whether the enclave ever intends to
change the permission to X.

I suppose there's a middle ground.  The driver could use model #1 for
driver-filled pages and model #2 for dynamic pages.  I haven't tried
to fully work it out, but I think there would be the ALLOW_READ /
ALLOW_WRITE / ALLOW_EXEC flag for EADD-ed pages but, for EAUG-ed
pages, there would be a different policy.  This might be as simple as
internally having four flags instead of three:

ALLOW_READ, ALLOW_WRITE, ALLOW_EXEC: as before

ALLOW_EXEC_COND: set implicitly by the driver for EAUG.

As in #1, if you try to mmap or protect a page with neither ALLOW_EXEC
variant, it fails (-EACCES, perhaps).  But, if you try to mmap or
mprotect an ALLOW_EXEC_COND page with PROT_EXEC, you ask LSM for
permission.  There is no fancy DIRTY tracking here, since it's
reasonable to just act as though *every* ALLOW_EXEC_COND page is
dirty.  There is no real auditing issue here, since LSM can just log
what permission is missing.

Does this seem sensible?  It might give us the best of #1 and #2.

Follow-up question, is #1 any more palatable if SELinux adds SGX specific
permissions and ties them to the process (instead of the vma or sigstruct)?

I'm not sure this makes a difference.  It simplifies SIGSTRUCT
handling, which is handy.