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

[Jarkko Sakkinen]

On Tue, Sep 22, 2020 at 08:11:14AM -0700, Dave Hansen wrote:
> > Enclave permissions can be dynamically modified by using ENCLS[EMODPE]
> 
> I'm not sure this sentence matters.  I'm not sure why I care what the
> instruction is named that does this.  But, it _sounds_ here like an
> enclave can adjust its own permissions directly with ENCLS[EMODPE].

If there was no EMODPE, I would drop this patch from the patch set. It
is the only reason I keep it. There are no other hard reasons to have
this.

> Now I'm confused.  I actually don't think I have a strong understanding
> of how an enclave actually gets loaded, how mmap() and mprotect() are
> normally used and what this hook is intended to thwart.

Enclave gets loaded with the ioctl API so ATM we rely only on
DAC permissions.

In future you might want to have LSM hooks to improve granularity
in two points:

1. When pages are added using SGX_IOC_ENCLAVE ADD_PAGES.
2. When enclave is initialized using SGX_IOC_ENCLAVE_INIT

In both you might want to make a policy decision based on origin
and page permissions.

If we do not limit mmap(), enclave could later on upgrade its
permissions with EMODPE and do mmap().

No matter what kind of LSM hooks or whatever possible improvements
are done later on to access control, they won't work unless we have
this because the permissions could be different than the original.

With this change you can still do EMODPE inside an enclave, but you
don't gain anything with it because your max permissions are capped
during the build time.

I'm not sure what exactly from this is missing from the commit message
but if you get this explanation maybe you can help me out with that.

Thank you for the feedback.

/Jarkko