On Fri, May 31, 2019 at 4:32 PM Sean Christopherson
<sean.j.christopherson@xxxxxxxxx> wrote:
>
> ...to support (the equivalent) of existing Linux Security Module
> functionality.
>
> Because SGX manually manages EPC memory, all enclave VMAs are backed by
> the same vm_file, i.e. /dev/sgx/enclave, so that SGX can implement the
> necessary hooks to move pages in/out of the EPC. And because EPC pages
> for any given enclave are fundamentally shared between processes, i.e.
> CoW semantics are not possible with EPC pages, /dev/sgx/enclave must
> always be MAP_SHARED. Lastly, all real world enclaves will need read,
> write and execute permissions to EPC pages. As a result, SGX does not
> play nice with existing LSM behavior as it is impossible to apply
> policies to enclaves with any reasonable granularity, e.g. an LSM can
> deny access to EPC altogether, but can't deny potentially dangerous
> behavior such as mapping pages RW->RW or RWX.
>
> To give LSMs enough information to implement their policies without
> having to resort to ugly things, e.g. holding a reference to the vm_file
> of each enclave page, require userspace to explicitly state the allowed
> protections for each page (region), i.e. take ALLOW_{READ,WRITE,EXEC}
> in the ADD_PAGES ioctl.
>
> The ALLOW_* flags will be passed to LSMs so that they can make informed
> decisions when the enclave is being built, i.e. when the source vm_file
> is available. For example, SELinux's EXECMOD permission can be
> required if an enclave is requesting both ALLOW_WRITE and ALLOW_EXEC.
>
> Update the mmap()/mprotect() hooks to enforce the ALLOW_* protections,
> a la the standard VM_MAY{READ,WRITE,EXEC} flags.
>
> The ALLOW_EXEC flag also has a second (important) use in that it can
> be used to prevent loading an enclave from a noexec file system, on
> SGX2 hardware (regardless of kernel support for SGX2), userspace could
> EADD from a noexec path using read-only permissions and later mprotect()
> and ENCLU[EMODPE] the page to gain execute permissions. By requiring
> ALLOW_EXEC up front, SGX will be able to enforce noexec paths when
> building the enclave.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> ---
> arch/x86/include/uapi/asm/sgx.h | 9 ++++++++-
> arch/x86/kernel/cpu/sgx/driver/ioctl.c | 23 +++++++++++++++++------
> arch/x86/kernel/cpu/sgx/encl.c | 2 +-
> arch/x86/kernel/cpu/sgx/encl.h | 1 +
> 4 files changed, 27 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> index 4a12d6abbcb7..4489e92fa0dc 100644
> --- a/arch/x86/include/uapi/asm/sgx.h
> +++ b/arch/x86/include/uapi/asm/sgx.h
> @@ -31,6 +31,11 @@ struct sgx_enclave_create {
> __u64 src;
> };
>
> +/* Supported flags for struct sgx_enclave_add_pages. */
> +#define SGX_ALLOW_READ VM_READ
> +#define SGX_ALLOW_WRITE VM_WRITE
> +#define SGX_ALLOW_EXEC VM_EXEC
> +
> /**
> * struct sgx_enclave_add_pages - parameter structure for the
> * %SGX_IOC_ENCLAVE_ADD_PAGES ioctl
> @@ -39,6 +44,7 @@ struct sgx_enclave_create {
> * @secinfo: address for the SECINFO data (common to all pages)
> * @nr_pages: number of pages (must be virtually contiguous)
> * @mrmask: bitmask for the measured 256 byte chunks (common to all pages)
> + * @flags: flags, e.g. SGX_ALLOW_{READ,WRITE,EXEC} (common to all pages)
> */
> struct sgx_enclave_add_pages {
> __u64 addr;
> @@ -46,7 +52,8 @@ struct sgx_enclave_add_pages {
> __u64 secinfo;
> __u32 nr_pages;
> __u16 mrmask;
> -} __attribute__((__packed__));
> + __u16 flags;
Minor nit: I would use at least u32 for flags. It's not like the size
saving is important.
> static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
> unsigned long src, struct sgx_secinfo *secinfo,
> - unsigned int mrmask)
> + unsigned int mrmask, unsigned int flags)
> {
> + unsigned long prot = secinfo->flags & (VM_READ | VM_WRITE | VM_EXEC);
> + unsigned long allowed_prot = flags & (VM_READ | VM_WRITE | VM_EXEC);
...
> + if (prot & ~allowed_prot)
> + return -EACCES;
This seems like a well-intentioned sanity check, but it doesn't really
accomplish anything with SGX2, so I tend to think it would be better
to omit it.
