Re: [PATCH v38 10/24] mm: Add vm_ops->mprotect()

[Sean Christopherson]

On Thu, Sep 24, 2020 at 01:54:09PM -0700, Dave Hansen wrote:
> On 9/24/20 1:25 PM, Sean Christopherson wrote:
> ...
> >> Why don't we just declare enclave memory as "out of scope for noexec" in
> >> the same way that anonymous memory is, and just discard this patch?
> >> That doesn't seem too much of a stretch.
> > 
> > Because we lose line of sight to LSM support.  Without enforcing "declare perms
> > at load time" in the initial series, we would create an ABI where userspace
> > could load an enclave page with only READ permissions and then map the enclave
> > with whatever permissions it wants, without any convenient way for SGX to call
> > into the LSM.
> 
> This argument holds no water for me.  LSMs are all about taking what
> would otherwise be perfectly acceptable behavior and breaking them in
> the name of security.  They fundamentally break applications that used
> to work just fine and also did totally sane things.

It's not about LSMs breaking things, they can obviously do that without any
help.  The concern is that, unless we lay the groundwork now, adding support
for LSMs in the future will break existing applications or create an unholy
mess of an ABI.

If we want to support LSM policy for enclave page permissions, checking LSM
policies at load time and hooking mprotect() to enforce the policy at run
time is by far the cleanest solution of the many ideas we discussed.

The problem is that enforcing permissions via mprotect() needs to be done
unconditionally, otherwise we end up with weird behavior where the existence
of an LSM will change what is/isn't allowed, even if the LSM(s) has no SGX
policy whatsover.

And on the flip side, enforcing permissions unconditionally will break
backwards compatibility.

I'm a-ok if we want to kill off the ->mprotect() hook, so long as we
acknowledge that in doing so we are likely throwing in the towel on supporting
LSM policies for enclave page permissions.