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

[Jarkko Sakkinen]

On Mon, Sep 21, 2020 at 09:57:58AM -0700, Sean Christopherson wrote:
> On Mon, Sep 21, 2020 at 03:49:46PM +0300, Jarkko Sakkinen wrote:
> > On Fri, Sep 18, 2020 at 04:53:37PM -0700, Sean Christopherson wrote:
> > > a noexec filesystem by loading code into an enclave, and to give the kernel the
> > > option of adding enclave specific LSM policies in the future.
> > > 
> > > The source file (if one exists) for the enclave is long gone when the enclave
> > > is actually mmap()'d and mprotect()'d.  To enforce noexec, the requested
> > > permissions for a given page are snapshotted when the page is added to the
> > > enclave, i.e. when the enclave is built.  Enclave pages that will be executable
> > > must originate from an a MAYEXEC VMA, e.g. the source page can't come from a
> > > noexec file system.
> > 
> > noexec check is done in __sgx_encl_add_page(), not in this callback.
> > sgx_vma_mprotect() calls sgx_encl_may_map(), which iterates the
> > addresses, checks that permissions are not surpassed and there are
> > no holes.
> 
> Yes, that's what I said.

sgx_encl_add_page() will remove such page. The callback does not
interact with this process as such pages never get to the enclave.

> I would copy-paste the part of the response that was snipped...

I do agree with the main conclusions but it contains also things that I
do not see relating that much, like noexec partitions. It goes too far
in detail what will LSM's end up doing. I absolutely do not want to
forecast too far how LSM hooks would work.

Since we do not have ioctl's for EMODPE and such, I see EMODPE as the
only reason for doing this right now. Otherwise, we are in trouble with
any possible LSM callbacks. For any sort of access control decision,
things decided must stick.

I would add something like this to the commit message largely based on
your text:

"SGX stores the permissions for each page when they are first added, and
will implement this callback to check that mmap() or mprotect() does not
surpass these permissions in the requested address range.

This is done to prevent using EMODPE upgrading permissions of a page
after mmap() or mprotect() has been done, which would prevent any sort
of LSM callbacks to be implemented later on because the access control
decision could deprecate."

/Jarkko