Re: [PATCH 2/4] x86/sgx: Put enclaves into anonymous files

[Sean Christopherson <sean.j.christopherson@intel.com>]

On Tue, Mar 31, 2020 at 10:39:38AM -0700, Andy Lutomirski wrote:
> On Tue, Mar 31, 2020 at 4:44 AM Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > When creating an enclave attach it to an anonymous file. This prepares the
> > code to have a separate interface at runtime, which can be published to the
> > user space after the enclave has been fully initialized.
> 
> This isn't an objection per se, but I can't shake the feeling that
> this seems ridiculous.  This changes the type of object returned by
> open() because, without this change, the old type was problematic.
> 
> So I have some questions:
> 
>  - Can sgx just ignore the fs noexec option on the chardev inode's fs instead?

Not easily.  do_mmap() rejects PROT_EXEC based on noexec without checking
VM_MAYEXEC (because it's the one that configures VM_MAYEXEC).  SGX could add
VM_MAYEXEC back in during its ->mmap hook, but it would mean userspace would
never be able to do mmap() w/ PROT_EXEC, i.e. would always have to mmap()
then mprotect().  I don't see any way to a dodge the check without doing
something nasty.

                        if (path_noexec(&file->f_path)) {
                                if (vm_flags & VM_EXEC)
                                        return -EPERM;
                                vm_flags &= ~VM_MAYEXEC;
                        }

>  - Would SELinux users *want* to put a useful label on the inode?

Probably?  EXECMEM is likely the biggest point of contention.  I assume
users would also want to do ioctl() whitelisting, etc...  I can't think of
a use case where someone would want to lock down the SGX ioctls, but I can
see someone wanting to ensure the enclave fd can only be used for SGX stuff.

> if so, can they still accomplish whatever they were trying to accomplish
> with this patch applied?

Not at this time.  There's the "secure anon inode" series floating around
that would theoretically remedy that, but that's pure happenstance and not
something we want to rely on.

It wasn't mentioned in the cover letter, but this will effectively require
PROCESS_EXECMEM (in SELinux) for all processes that _run_ enclaves.  It
would allow processes that only _build_ enclaves to avoid PROCESS_EXECMEM,
as they wouldn't need to actually map the enclave.  Jarkko's contention is
enclaves _should_ require EXECMEM and that it's ok to require EXECMEM on
the runtime so long as the builder can run without EXECMEM.

If EXECMEM is a sticking point, one way to dodge it would be to add a
helper to allow SELinux to detect enclave files.  It'd be ugly, but simple.
That doesn't solve the generic labeling issue though.  It also begs the
question of why hacking SELinux but not do_mmap() would be acceptable.

If you have any ideas for fixing the noexec issue without resorting to an
anon inode, we're all ears.

> 
> --Andy