Messages in this thread | | | Subject | Re: [PATCH v38 10/24] mm: Add vm_ops->mprotect() | Date | Thu, 24 Sep 2020 14:11:37 -0500 | From | "Haitao Huang" <> |
| |
On Wed, 23 Sep 2020 08:50:56 -0500, Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> 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
There are use cases supported currently in which enclave binary is received via IPC/RPC and held in buffers before EADD. Denying noexec altogether would break those, right?
| |