linux-kernel - Re: [RFC PATCH 8/9] LSM: x86/sgx: Introduce ->enclave

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CALCETrXf3ujAn6uOwWMU8SRZOvBRb8ALvo_LQvwxc899mrakwQ@mail.gmail.com>
Date:   Tue, 4 Jun 2019 13:29:10 -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 8/9] LSM: x86/sgx: Introduce ->enclave_load() hook for
 Intel SGX

On Fri, May 31, 2019 at 4:32 PM Sean Christopherson
<sean.j.christopherson@...el.com> 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@...el.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(&current->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(&current->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.