lkml.org 
[lkml]   [2019]   [Jun]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC PATCH 8/9] LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX
From
Date
On 6/3/19 10:42 AM, Sean Christopherson wrote:
> On Mon, Jun 03, 2019 at 10:19:18AM -0400, Stephen Smalley wrote:
>> On 5/31/19 7:31 PM, Sean Christopherson wrote:
>>> enclave_load() is roughly analogous to the existing file_mprotect().
>>>
>>> Due to the nature of SGX and its Enclave Page Cache (EPC), all enclave
>>> VMAs are backed by a single file, i.e. /dev/sgx/enclave, that must be
>>> MAP_SHARED. Furthermore, all enclaves need read, write and execute
>>> VMAs. As a result, file_mprotect() does not provide any meaningful
>>> security for enclaves since an LSM can only deny/grant access to the
>>> EPC as a whole.
>>>
>>> security_enclave_load() is called when SGX is first loading an enclave
>>> page, i.e. copying a page from normal memory into the EPC. The notable
>>> difference from file_mprotect() is the allowed_prot parameter, which
>>> is essentially an SGX-specific version of a VMA's MAY_{READ,WRITE,EXEC}
>>> flags. The purpose of allowed_prot is to enable checks such as
>>> SELinux's FILE__EXECMOD permission without having to track and update
>>> VMAs across multiple mm structs, i.e. SGX can ensure userspace doesn't
>>> overstep its bounds simply by restricting an enclave VMA's protections
>>> by vetting what is maximally allowed during build time.
>>>
>>> An alternative to the allowed_prot approach would be to use an enclave's
>>> SIGSTRUCT (a smallish structure that can uniquely identify an enclave)
>>> as a proxy for the enclave. For example, SGX could take and hold a
>>> reference to the file containing the SIGSTRUCT (if it's in a file) and
>>> call security_enclave_load() during mprotect(). While the SIGSTRUCT
>>> approach would provide better precision, the actual value added was
>>> deemed to be negligible. On the other hand, pinning a file for the
>>> lifetime of the enclave is ugly, and essentially caching LSM policies
>>> in each page's allowed_prot avoids having to make an extra LSM upcall
>>> during mprotect().
>>>
>>> Note, extensive discussion yielded no sane alternative to some form of
>>> SGX specific LSM hook[1].
>>>
>>> [1] https://lkml.kernel.org/r/CALCETrXf8mSK45h7sTK5Wf+pXLVn=Bjsc_RLpgO-h-qdzBRo5Q@mail.gmail.com
>>>
>>> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
>>> ---
>>> arch/x86/kernel/cpu/sgx/driver/ioctl.c | 14 +++++++++-----
>>> include/linux/lsm_hooks.h | 16 ++++++++++++++++
>>> include/linux/security.h | 2 ++
>>> security/security.c | 8 ++++++++
>>> 4 files changed, 35 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
>>> index 5f71be7cbb01..260417ecbcff 100644
>>> --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
>>> +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
>>> @@ -8,6 +8,7 @@
>>> #include <linux/highmem.h>
>>> #include <linux/ratelimit.h>
>>> #include <linux/sched/signal.h>
>>> +#include <linux/security.h>
>>> #include <linux/shmem_fs.h>
>>> #include <linux/slab.h>
>>> #include <linux/suspend.h>
>>> @@ -580,21 +581,24 @@ static int sgx_encl_page_protect(unsigned long src, unsigned long prot,
>>> unsigned long *allowed_prot)
>>> {
>>> struct vm_area_struct *vma;
>>> + int ret = 0;
>>> - if (!(*allowed_prot & VM_EXEC))
>>> + if (!(*allowed_prot & VM_EXEC) && !IS_ENABLED(CONFIG_SECURITY))
>>> goto do_check;
>>> down_read(&current->mm->mmap_sem);
>>> vma = find_vma(current->mm, src);
>>> if (!vma || (vma->vm_file && path_noexec(&vma->vm_file->f_path)))
>>> *allowed_prot &= ~VM_EXEC;
>>> +#ifdef CONFIG_SECURITY
>>> + ret = security_enclave_load(vma, prot, allowed_prot);
>>> +#endif
>>
>> Normally you'd define a static inline stub for the hook in the #else clause
>> for CONFIG_SECURITY in include/linux/security.h and avoid any ifdef here.
>
> Ah, right.
>
>> What ensures that the mapping referenced by src can't be changed to an
>> entirely different one (with a different vm_file) between the time of check
>> (here) and the time of use?
>
> Nothing. Holding mmap_sem across copy_from_user() would suffice, correct?

I don't believe you can do that; copy_from_user() could stall
indefinitely. Not sure how to do what you want here or if it requires
changing the interface.

>
>>> up_read(&current->mm->mmap_sem);
>>> do_check:
>>> - if (prot & ~*allowed_prot)
>>> - return -EACCES;
>>> -
>>> - return 0;
>>> + if (!ret && (prot & ~*allowed_prot))
>>> + ret = -EACCES;
>>> + return ret;
>>> }
>>> static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,

\
 
 \ /
  Last update: 2019-06-03 20:39    [W:0.083 / U:0.636 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site