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@xxxxxxxxxxxxxx
Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
---
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,
