On Tue, Sep 22, 2020 at 12:07:36AM +0300, Jarkko Sakkinen wrote: > 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 think we're in violent agreement, mostly. 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. > > 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. As above, this does directly related to noexec/VM_MAYEXEC. > 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. That's fine, I was responding to Andy's question, not intending to write a changelog. > 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. Yes, again, violent agreement :-). > 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."
