| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CALCETrU1mV0NQoG=ba0BVf5WpeqfdJkhcGnyo9Jk+hFo9eCXUw@mail.gmail.com>
Date: Tue, 4 Jun 2019 13:23:40 -0700
From: Andy Lutomirski <luto@...nel.org>
To: Sean Christopherson <sean.j.christopherson@...el.com>
Cc: Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
Andy Lutomirski <luto@...nel.org>,
Cedric Xing <cedric.xing@...el.com>,
Stephen Smalley <sds@...ho.nsa.gov>,
James Morris <jmorris@...ei.org>,
"Serge E . Hallyn" <serge@...lyn.com>,
LSM List <linux-security-module@...r.kernel.org>,
Paul Moore <paul@...l-moore.com>,
Eric Paris <eparis@...isplace.org>, selinux@...r.kernel.org,
Jethro Beekman <jethro@...tanix.com>,
Dave Hansen <dave.hansen@...el.com>,
Thomas Gleixner <tglx@...utronix.de>,
Linus Torvalds <torvalds@...ux-foundation.org>,
LKML <linux-kernel@...r.kernel.org>, X86 ML <x86@...nel.org>,
linux-sgx@...r.kernel.org,
Andrew Morton <akpm@...ux-foundation.org>, nhorman@...hat.com,
npmccallum@...hat.com, Serge Ayoun <serge.ayoun@...el.com>,
Shay Katz-zamir <shay.katz-zamir@...el.com>,
Haitao Huang <haitao.huang@...el.com>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Kai Svahn <kai.svahn@...el.com>,
Borislav Petkov <bp@...en8.de>,
Josh Triplett <josh@...htriplett.org>,
Kai Huang <kai.huang@...el.com>,
David Rientjes <rientjes@...gle.com>,
William Roberts <william.c.roberts@...el.com>,
Philip Tricca <philip.b.tricca@...el.com>
Subject: Re: [RFC PATCH 6/9] x86/sgx: Require userspace to provide allowed
prots to ADD_PAGES
On Fri, May 31, 2019 at 4:32 PM Sean Christopherson
<sean.j.christopherson@...el.com> 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@...el.com>
> ---
> 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.
Powered by blists - more mailing lists