RE: [RFC PATCH v4 09/12] LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX

["Xing, Cedric" <cedric.xing@intel.com>]

> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
> owner@vger.kernel.org] On Behalf Of Stephen Smalley
> Sent: Tuesday, June 25, 2019 2:49 PM
> 
> On 6/25/19 5:01 PM, Stephen Smalley wrote:
> > On 6/21/19 1:05 PM, Xing, Cedric wrote:
> >>> From: Christopherson, Sean J
> >>> Sent: Wednesday, June 19, 2019 3:24 PM
> >>>
> >>> diff --git a/include/linux/security.h b/include/linux/security.h
> >>> index 6a1f54ba6794..572ddfc53039 100644
> >>> --- a/include/linux/security.h
> >>> +++ b/include/linux/security.h
> >>> @@ -1832,11 +1832,18 @@ static inline void
> >>> security_bpf_prog_free(struct bpf_prog_aux *aux)
> >>>   #ifdef CONFIG_INTEL_SGX
> >>>   #ifdef CONFIG_SECURITY
> >>>   int security_enclave_map(unsigned long prot);
> >>> +int security_enclave_load(struct vm_area_struct *vma, unsigned long
> >>> prot,
> >>> +              bool measured);
> >>>   #else
> >>>   static inline int security_enclave_map(unsigned long prot)
> >>>   {
> >>>       return 0;
> >>>   }
> >>> +static inline int security_enclave_load(struct vm_area_struct *vma,
> >>> +                    unsigned long prot, bool measured) {
> >>> +    return 0;
> >>> +}
> >>>   #endif /* CONFIG_SECURITY */
> >>>   #endif /* CONFIG_INTEL_SGX */
> >>
> >> Parameters to security_enclave_load() are specific on what's being
> >> loading only, but unspecific on which enclave to be loaded into. That
> >> kills the possibility of an LSM module making enclave dependent
> >> decisions.
> >>
> >> Btw, if enclave (in the form of struct file) is also passed in as a
> >> parameter, it'd let LSM know that file is an enclave, hence would be
> >> able to make the same decision in security_mmap_file() as in
> >> security_enclave_map(). In other words, you wouldn't need
> >> security_enclave_map().
> >
> > Sorry, you want security_enclave_load() to stash a reference to the
> > enclave file in some security module-internal state, then match it
> > upon later security_mmap_file() calls to determine that it is dealing
> > with an enclave, and then adjust its logic accordingly?  When do we
> > release that reference?
> 
> I guess you mean set a flag in the enclave file security struct upon
> security_enclave_load() and check that flag in security_mmap_file().

Yes, by invoking security_enclave_load(), the SGX subsystem indicates to LSM the file struct in subject refers to an enclave. But security_mmap_file() doesn't pass in the range being mmap()'ed so LSM still cannot decide. Instead of changing the definition of security_mmap_file(), I'd invoke security_file_mprotect() from sgx_mmap(). After all, creating a new mapping is equivalent to changing the target range from PROT_NONE to @prot being requested. I just sent out a patch series with all those details in code.

> 
> This seems somewhat similar to one of Sean's alternatives in the patch
> description for 06/12, except by pushing the information from sgx to LSM
> upon security_enclave_load() rather than pulling it via a
> is_sgx_enclave() helper.  Not clear if it is still subject to the same
> limitations.

Yes, they are similar except who keeps track of that piece of information. As Dr. Greg pointed out, the new hooks do have to be SGX specific. But calling is_sgx_enclave() really ties LSM to SGX. In contrast, inferring it through security_enclave_load() makes LSM SGX-agnostic. Then the only SGX specific thing left in the hooks is the sgx_sigstruct. In theory, that's just a digital signature and as Dr. Greg pointed out, SGX is probably not the only technology that uses digital signature to identify executable contents. And in that sense, if we rename it to something generic with probably a tag indicating its format, then the whole thing would become SGX agnostic and could be useful for other TEEs on architectures other than x86.