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

[Jarkko Sakkinen]

On Tue, Sep 22, 2020 at 09:43:02AM -0700, Sean Christopherson wrote:
> On Tue, Sep 22, 2020 at 08:35:15AM +0300, Jarkko Sakkinen wrote:
> > On Tue, Sep 22, 2020 at 08:30:06AM +0300, Jarkko Sakkinen wrote:
> > > On Mon, Sep 21, 2020 at 02:18:49PM -0700, Sean Christopherson wrote:
> > > > Userspace can add the page without EXEC permissions in the EPCM, and thus
> > > > avoid the noexec/VM_MAYEXEC check.  The enclave can then do EMODPE to gain
> > > > EXEC permissions in the EPMC.  Without the ->mprotect() hook, we wouldn't
> > > > be able to detect/prevent such shenanigans.
> > > 
> > > Right, the VM_MAYEXEC in the code is nested under VM_EXEC check.
> > > 
> > > I'm only wondering why not block noexec completely with any permissions,
> > > i.e. why not just have unconditional VM_MAYEXEC check?
> > 
> > I.e. why not this:
> > 
> > static int __sgx_encl_add_page(struct sgx_encl *encl,
> > 			       struct sgx_encl_page *encl_page,
> > 			       struct sgx_epc_page *epc_page,
> > 			       struct sgx_secinfo *secinfo, unsigned long src)
> > {
> > 	struct sgx_pageinfo pginfo;
> > 	struct vm_area_struct *vma;
> > 	struct page *src_page;
> > 	int ret;
> > 
> > 	vma = find_vma(current->mm, src);
> > 	if (!vma)
> > 		return -EFAULT;
> > 
> > 	if (!(vma->vm_flags & VM_MAYEXEC))
> > 		return -EACCES;
> > 
> > I'm not seeing the reason for "partial support" for noexec partitions.
> > 
> > If there is a good reason, fine, let's just then document it.
> 
> There are scenarios I can contrive, e.g. loading an enclave from a noexec
> filesystem without having to copy the entire enclave to anon memory, or
> loading a data payload from a noexec FS.
> 
> They're definitely contrived scenarios, but given that we also want the
> ->mprotect() hook/behavior for potential LSM interaction, supporting said
> contrived scenarios costs is "free".

For me this has caused months of confusion and misunderstanding of this
feature. I only recently realized that "oh, right, we invented this".

They are contrived scenarios enough that they should be considered when
the workloads hit.

Either we fully support noexec or not at all. Any "partial" thing is a
two edged sword: it can bring some robustness with the price of
complexity and possible unknown uknown scenarios where they might become
API issue.

I rather think later on how to extend API in some way to enable such
contrivid scenarios rather than worrying about how this could be abused.

The whole SGX is complex beast already so lets not add any extra when
there is no a hard requirement to do so.

I'll categorically deny noexec in the next patch set version.

/Jarkko