Re: [RFC PATCH 0/9] security: x86/sgx: SGX vs. LSM

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

On Tue, Jun 04, 2019 at 11:33:44AM -0400, Stephen Smalley wrote:
> The RFC series seemed to dispense with the use of the sigstruct file and
> just used the source file throughout IIUC.  That allowed for reuse of
> FILE__* permissions without ambiguity rather than introducing separate
> ENCLAVE__* permissions or using /dev/sgx/enclave inode as the target of all
> checks.

Drat, I meant to explicitly call that out in the cover letter.  Yes, the
concept of using sigstruct as a proxy was dropped for this RFC.  The
primary motivation was to avoid having to take a hold a reference to the
sigstruct file for the lifetime of the enclave, and in general so that
userspace isn't forced to put sigstruct into a file.

> Regardless, IIUC, your approach requires that we always check FILE__EXECMOD,
> and FILE__EXECUTE up front during security_enclave_load() irrespective of
> prot so that we can save the result in the f_security for later use by the
> mprotect hook.

Correct, this approach requires up front checks.

> This may generate many spurious audit messages for cases
> where PROT_EXEC will never be requested, and users will be prone to just
> always allowing it since they cannot tell when it was actually needed.

Userspace will be able to understand when PROT_EXEC is actually needed
as mprotect() will (eventually) fail.  Of course that assumes userspace
is being intelligent and isn't blindly declaring permissions they don't
need, e.g. declaring RWX on all pages even though the enclave never
actually maps a RWX or RW->RX page.

One thought for handling this in a more user friendly fashion would be
to immediately return -EACCES instead of modifying @allowed_prot.  An
enclave that truly needs the permission would fail immediately.

An enclave loader that wants/needs to speculatively declare PROT_EXEC,
e.g. because the exact requirements of the enclave are unknown, could
handle -EACCESS gracefully by retrying the SGX ioctl() with different
@allowed_prot, e.g.:

  region.flags = SGX_ALLOW_READ | SGX_ALLOW_WRITE | SGX_ALLOW_EXEC;

  ret = ioctl(fd, SGX_IOC_ENCLAVE_ADD_REGION, &region);
  if (ret && errno == EACCES && !(prot & PROT_EXEC)) {
      region.flags &= ~SGX_ALLOW_EXEC;
      ret = ioctl(fd, SGX_IOC_ENCLAVE_ADD_REGION, &region);
  }

This type of enclave loader would still generate spurious audit messages,
but the spurious messages would be limited to enclave loaders that are
deliberately probing the allowed permissions.

> >The noexec case should be addressed in IOC_ADD_PAGES by testing
> >@source_vma->vm_flags & VM_MAYEXEC.
> >
> >>
> >>>* In hook security_file_free(), if @file is an  enclave, free storage
> >>>   allocated for WRITTEN flags.
>