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 <[email protected]>
---
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
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,
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 47f58cfb6a19..0562775424a0 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1446,6 +1446,14 @@
* @bpf_prog_free_security:
* Clean up the security information stored inside bpf prog.
*
+ * Security hooks for Intel SGX enclaves.
+ *
+ * @enclave_load:
+ * On success, returns 0 and optionally adjusts @allowed_prot
+ * @vma: the source memory region of the enclave page being loaded.
+ * @prot: the initial protection of the enclave page.
+ * @allowed_prot: the maximum protections of the enclave page.
+ * Return 0 if permission is granted.
*/
union security_list_options {
int (*binder_set_context_mgr)(struct task_struct *mgr);
@@ -1807,6 +1815,11 @@ union security_list_options {
int (*bpf_prog_alloc_security)(struct bpf_prog_aux *aux);
void (*bpf_prog_free_security)(struct bpf_prog_aux *aux);
#endif /* CONFIG_BPF_SYSCALL */
+
+#ifdef CONFIG_INTEL_SGX
+ int (*enclave_load)(struct vm_area_struct *vma, unsigned long prot,
+ unsigned long *allowed_prot);
+#endif /* CONFIG_INTEL_SGX */
};
struct security_hook_heads {
@@ -2046,6 +2059,9 @@ struct security_hook_heads {
struct hlist_head bpf_prog_alloc_security;
struct hlist_head bpf_prog_free_security;
#endif /* CONFIG_BPF_SYSCALL */
+#ifdef CONFIG_INTEL_SGX
+ struct hlist_head enclave_load;
+#endif /* CONFIG_INTEL_SGX */
} __randomize_layout;
/*
diff --git a/include/linux/security.h b/include/linux/security.h
index 659071c2e57c..2f7925eeef7e 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -392,6 +392,8 @@ void security_inode_invalidate_secctx(struct inode *inode);
int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
+int security_enclave_load(struct vm_area_struct *vma, unsigned long prot,
+ unsigned long *allowed_prot);
#else /* CONFIG_SECURITY */
static inline int call_lsm_notifier(enum lsm_event event, void *data)
diff --git a/security/security.c b/security/security.c
index 613a5c00e602..07ed6763571e 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2359,3 +2359,11 @@ void security_bpf_prog_free(struct bpf_prog_aux *aux)
call_void_hook(bpf_prog_free_security, aux);
}
#endif /* CONFIG_BPF_SYSCALL */
+
+#ifdef CONFIG_INTEL_SGX
+int security_enclave_load(struct vm_area_struct *vma, unsigned long prot,
+ unsigned long *allowed_prot)
+{
+ return call_int_hook(enclave_load, 0, vma, prot, allowed_prot);
+}
+#endif /* CONFIG_INTEL_SGX */
--
2.21.0
> From: Christopherson, Sean J
> Sent: Friday, May 31, 2019 4:32 PM
>
> diff --git a/include/linux/security.h b/include/linux/security.h index
> 659071c2e57c..2f7925eeef7e 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -392,6 +392,8 @@ void security_inode_invalidate_secctx(struct inode *inode); int
> security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen); int
> security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen); int
> security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
> +int security_enclave_load(struct vm_area_struct *vma, unsigned long prot,
> + unsigned long *allowed_prot);
Per my comments to the cover letter, security_enclave_load() should have a signature similar to the following:
int security_enclave_load(struct file *enclave_fd, unsigned long linear_address, unsigned long nr_pages, int prot, struct vm_area_struct *source_vma);
@enclave_fd identifies the enclave to which new pages are being added.
@linear_address/@nr_pages specifies the linear range of pages being added.
@prot specifies the initial protection of those newly added pages. It is taken from the vma covering the target range.
@source_vma covers the source pages in the case of EADD. An LSM is expected to make sure security_file_mprotect(source_vma, prot, prot) would succeed before checking anything else, unless @source_vma is NULL, indicating pages are being EAUG'ed. In all cases, LSM is expected to "remember" @prot for all those pages to be checked in future security_file_mprotect() invocations.
> #else /* CONFIG_SECURITY */
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 <[email protected]>
> ---
> 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.
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?
> 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,
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 47f58cfb6a19..0562775424a0 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1446,6 +1446,14 @@
> * @bpf_prog_free_security:
> * Clean up the security information stored inside bpf prog.
> *
> + * Security hooks for Intel SGX enclaves.
> + *
> + * @enclave_load:
> + * On success, returns 0 and optionally adjusts @allowed_prot
> + * @vma: the source memory region of the enclave page being loaded.
> + * @prot: the initial protection of the enclave page.
> + * @allowed_prot: the maximum protections of the enclave page.
> + * Return 0 if permission is granted.
> */
> union security_list_options {
> int (*binder_set_context_mgr)(struct task_struct *mgr);
> @@ -1807,6 +1815,11 @@ union security_list_options {
> int (*bpf_prog_alloc_security)(struct bpf_prog_aux *aux);
> void (*bpf_prog_free_security)(struct bpf_prog_aux *aux);
> #endif /* CONFIG_BPF_SYSCALL */
> +
> +#ifdef CONFIG_INTEL_SGX
> + int (*enclave_load)(struct vm_area_struct *vma, unsigned long prot,
> + unsigned long *allowed_prot);
> +#endif /* CONFIG_INTEL_SGX */
> };
>
> struct security_hook_heads {
> @@ -2046,6 +2059,9 @@ struct security_hook_heads {
> struct hlist_head bpf_prog_alloc_security;
> struct hlist_head bpf_prog_free_security;
> #endif /* CONFIG_BPF_SYSCALL */
> +#ifdef CONFIG_INTEL_SGX
> + struct hlist_head enclave_load;
> +#endif /* CONFIG_INTEL_SGX */
> } __randomize_layout;
>
> /*
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 659071c2e57c..2f7925eeef7e 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -392,6 +392,8 @@ void security_inode_invalidate_secctx(struct inode *inode);
> int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
> int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
> int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
> +int security_enclave_load(struct vm_area_struct *vma, unsigned long prot,
> + unsigned long *allowed_prot);
> #else /* CONFIG_SECURITY */
>
> static inline int call_lsm_notifier(enum lsm_event event, void *data)
> diff --git a/security/security.c b/security/security.c
> index 613a5c00e602..07ed6763571e 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2359,3 +2359,11 @@ void security_bpf_prog_free(struct bpf_prog_aux *aux)
> call_void_hook(bpf_prog_free_security, aux);
> }
> #endif /* CONFIG_BPF_SYSCALL */
> +
> +#ifdef CONFIG_INTEL_SGX
> +int security_enclave_load(struct vm_area_struct *vma, unsigned long prot,
> + unsigned long *allowed_prot)
> +{
> + return call_int_hook(enclave_load, 0, vma, prot, allowed_prot);
> +}
> +#endif /* CONFIG_INTEL_SGX */
>
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 <[email protected]>
> >---
> > 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?
> > 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,
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 <[email protected]>
>>> ---
>>> 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,
...
>>> 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.
Holding mmap_sem for *read* is OK since you can handle page faults
underneath it. Holding it for write is not.
But, holding it for read also locks out the writers which might be
messing with vm_file or other parts of the VMA.
Holding it for read for a long time is OK. It's obviously not ideal,
but it is something we do widely today.
On Tue, Jun 04, 2019 at 01:29:10PM -0700, Andy Lutomirski wrote:
> On Fri, May 31, 2019 at 4:32 PM Sean Christopherson
> <[email protected]> wrote:
> > static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
> > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > index 47f58cfb6a19..0562775424a0 100644
> > --- a/include/linux/lsm_hooks.h
> > +++ b/include/linux/lsm_hooks.h
> > @@ -1446,6 +1446,14 @@
> > * @bpf_prog_free_security:
> > * Clean up the security information stored inside bpf prog.
> > *
> > + * Security hooks for Intel SGX enclaves.
> > + *
> > + * @enclave_load:
> > + * On success, returns 0 and optionally adjusts @allowed_prot
> > + * @vma: the source memory region of the enclave page being loaded.
> > + * @prot: the initial protection of the enclave page.
>
> What do you mean "initial"? The page is always mapped PROT_NONE when
> this is called, right? I feel like I must be missing something here.
Initial protection in the EPCM. Yet another reason to ignore SECINFO.
On Fri, May 31, 2019 at 4:32 PM Sean Christopherson
<[email protected]> 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 <[email protected]>
> ---
> 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
> 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,
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 47f58cfb6a19..0562775424a0 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1446,6 +1446,14 @@
> * @bpf_prog_free_security:
> * Clean up the security information stored inside bpf prog.
> *
> + * Security hooks for Intel SGX enclaves.
> + *
> + * @enclave_load:
> + * On success, returns 0 and optionally adjusts @allowed_prot
> + * @vma: the source memory region of the enclave page being loaded.
> + * @prot: the initial protection of the enclave page.
What do you mean "initial"? The page is always mapped PROT_NONE when
this is called, right? I feel like I must be missing something here.
> From: Christopherson, Sean J
> Sent: Tuesday, June 04, 2019 1:37 PM
>
> On Tue, Jun 04, 2019 at 01:29:10PM -0700, Andy Lutomirski wrote:
> > On Fri, May 31, 2019 at 4:32 PM Sean Christopherson
> > <[email protected]> wrote:
> > > static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long
> > > addr, diff --git a/include/linux/lsm_hooks.h
> > > b/include/linux/lsm_hooks.h index 47f58cfb6a19..0562775424a0 100644
> > > --- a/include/linux/lsm_hooks.h
> > > +++ b/include/linux/lsm_hooks.h
> > > @@ -1446,6 +1446,14 @@
> > > * @bpf_prog_free_security:
> > > * Clean up the security information stored inside bpf prog.
> > > *
> > > + * Security hooks for Intel SGX enclaves.
> > > + *
> > > + * @enclave_load:
> > > + * On success, returns 0 and optionally adjusts @allowed_prot
> > > + * @vma: the source memory region of the enclave page being
> loaded.
> > > + * @prot: the initial protection of the enclave page.
> >
> > What do you mean "initial"? The page is always mapped PROT_NONE when
> > this is called, right? I feel like I must be missing something here.
>
> Initial protection in the EPCM. Yet another reason to ignore SECINFO.
I know you guys are talking in the background that all pages are mmap()'ed PROT_NONE. But that's an unnecessary limitation. And @prot here should be @target_vma->vm_flags&(VM_READ|VM_WRITE|VM_EXEC).
On Tue, Jun 04, 2019 at 02:43:09PM -0700, Xing, Cedric wrote:
> > From: Christopherson, Sean J
> > Sent: Tuesday, June 04, 2019 1:37 PM
> >
> > On Tue, Jun 04, 2019 at 01:29:10PM -0700, Andy Lutomirski wrote:
> > > On Fri, May 31, 2019 at 4:32 PM Sean Christopherson
> > > <[email protected]> wrote:
> > > > static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long
> > > > addr, diff --git a/include/linux/lsm_hooks.h
> > > > b/include/linux/lsm_hooks.h index 47f58cfb6a19..0562775424a0 100644
> > > > --- a/include/linux/lsm_hooks.h
> > > > +++ b/include/linux/lsm_hooks.h
> > > > @@ -1446,6 +1446,14 @@
> > > > * @bpf_prog_free_security:
> > > > * Clean up the security information stored inside bpf prog.
> > > > *
> > > > + * Security hooks for Intel SGX enclaves.
> > > > + *
> > > > + * @enclave_load:
> > > > + * On success, returns 0 and optionally adjusts @allowed_prot
> > > > + * @vma: the source memory region of the enclave page being
> > loaded.
> > > > + * @prot: the initial protection of the enclave page.
> > >
> > > What do you mean "initial"? The page is always mapped PROT_NONE when
> > > this is called, right? I feel like I must be missing something here.
> >
> > Initial protection in the EPCM. Yet another reason to ignore SECINFO.
>
> I know you guys are talking in the background that all pages are mmap()'ed
> PROT_NONE. But that's an unnecessary limitation.
Not all pages have to be mmap()'d PROT_NONE, only pages that do not have
an associated enclave page.
> And @prot here should be @target_vma->vm_flags&(VM_READ|VM_WRITE|VM_EXEC).
I don't follow, there is no target_vma at this point.