Messages in this thread | | | Subject | Re: [RFC PATCH 8/9] LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX | From | Stephen Smalley <> | Date | Mon, 3 Jun 2019 14:38:31 -0400 |
| |
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(¤t->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(¤t->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,
| |