SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)

[Stephen Smalley]

On 5/30/19 10:31 AM, Andy Lutomirski wrote:
> Hi all-
> 
> After an offline discussion with Sean yesterday, here are some updates
> to the user API parts of my proposal.
> 
> Unfortunately, Sean convinced me that MAXPERM doesn't work the way I
> described it because, for SGX2, the enclave loader won't know at load
> time whether a given EAUG-ed page will ever be executed.  So here's an
> update.
> 
> First, here are the requrements as I see them, where EXECUTE, EXECMOD,
> and EXECMEM could be substituted with other rules at the LSM's
> discretion:
> 
>   - You can create a WX or RWX mapping if and only if you have EXECMEM.
> 
>   - To create an X mapping of an enclave page that has ever been W, you
> need EXECMOD.

EXECMOD to what file? The enclave file from which the page's content 
originated, the sigstruct file, or /dev/sgx/enclave?

>   - To create an X mapping of an enclave page that came from EADD, you
> need EXECUTE on the source file.  Optionally, we could also permit
> this if you have EXECMOD.

What is the "source file" i.e. the target of the check?  Enclave file, 
sigstruct file, or /dev/sgx/enclave?

> 
> And I have two design proposals.  One is static and one is dynamic.
> To implement either one, we will probably need a new .may_mprotect vm
> operation, and that operation can call an LSM hook.  Or we can give
> LSMs a way to detect that a given vm_area_struct is an enclave.  As I
> see it, this is an implementation detail that is certainly solveable.
> 
> 
> Static proposal:
> 
> 
> EADD takes an execute_intent flag.  It calls a new hook:
> 
>    int security_enclave_load(struct vm_area_struct *source, bool execute_intent);
> 
> This hook will fail if execute_intent==true and the caller has neither
> EXECUTE, EXECMOD, nor EXECMEM.

EADD execute_intent flag is originally provided by whom (userspace or 
driver) on what basis? Which file is referenced by source->vm_file? Why 
trigger all three checks up front versus only checking if needed?  Won't 
this trigger a lot of unnecessary EXECMOD and EXECMEM denials that will 
need to be dontaudit'd? What if there is a mismatch between 
execute_intent and the initial permissions?

> 
> EAUG sets execute_intent = false.
> 
> EINIT takes a sigstruct pointer.  SGX can (when initially upstreamed
> or later on once there's demand) call a new hook:
> 
>    security_enclave_init(struct sigstruct *sigstruct, struct
> vm_area_struct *source);

Is struct sigstruct the same as struct sgx_sigstruct in the current 
patches (i.e. just the sigstruct data, no file)?  What file is 
referenced by source->vm_file (the sigstruct or the enclave or 
/dev/sgx/enclave)?  Is this hook only for enforcing a whitelist on what 
enclaves can be loaded?  What is the target of the check?

> mmap() and mprotect() will require EXECMEM to create WX or RWX
> mappings.  They will require EXECMOD to create RX or X mappings of an
> execute_intent==false page.  They require no permissions in the other
> cases.

Does this occur for both setting initial permissions and runtime 
permissions or just runtime? Both userspace- and driver-initiated 
mmap/mprotect operations or just userspace-initiated ones?  Does the 
driver use interfaces that call the mmap/mprotect hooks or lower level 
functions?

> 
> 
> Dynamic proposal:
> 
> 
> EADD does not take any special flags.  It does something like this internally:
> 
>    bool execute_intent = true;
>    int security_enclave_load(struct vm_area_struct *source, bool
> *execute_intent);
> 
> The implementation of security_enclave_load() may set *execute_intent to false.
> The driver records execute_intent after the LSM is done.

On what basis does LSM decide whether to set *execute_intent?  If the 
process lacks all three permissions? What if there is a mismatch with 
the initial permissions?

> 
> mmap() and mprotect() will require EXECMEM to create WX or RWX
> mappings.  They will require EXECMOD to create RX or X mappings of an
> execute_intent==false page.  They require no permissions in the other
> cases.
> 
> 
> 
> A benefit of the static proposal is that audit failures due to a lack
> of EXECUTE permission are easy to implement and to understand in the
> lods.  With the dynamic model, we can only really audit the lack of
> EXECMOD or EXECMEM.  A benefit of the dynamic model is that we hide
> what is arguably a decently large wart from the API.
>