| Message ID | 20200330185452.1382455-1-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sgx: Remove PROT_NONE branch from sgx_encl_may_map(). | expand |
On Mon, Mar 30, 2020 at 09:54:52PM +0300, Jarkko Sakkinen wrote: > sgx_encl_may_map() always succeeding when PROT_NONE is given is not that > useful behaviour as one can just well as do an anonymous mapping as > demonstrated by the change in this patch to the test program. As a > consequence, remove the special case. > > Pratically any possible way to make sure that you don't overwrite anything > useful in the memory, should be fine. MAP_FIXED does not care what's > underneath (if you want't it to care you ought to use > MAP_FIXED_NO_REPLACE). > > After this change, the selftest run called sgx_mmap() only three times > (TCS, text, data) instead of four. > > test_sgx-1811 [002] .... 586.907585: sgx_mmap <-mmap_region > test_sgx-1811 [002] .... 586.911752: sgx_mmap <-mmap_region > test_sgx-1811 [002] .... 586.911756: sgx_mmap <-mmap_region > > This also gives more angles to segregate enclave building and mapping as > the mmap()'s need to be applied only when the enclave is fully built: > > Cc: luto@kernel.org > Cc: Haitao Huang <haitao.huang@linux.intel.com> > Cc: Sean Christopherson <sean.j.christopherson@intel.com> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> Sean, would be cool if you can try this out soonish because I'm building on top of this. I tried you vdso change on a GLK NUC and they seem to work. /Jarkko
On Tue, Mar 31, 2020 at 12:13:36AM +0300, Jarkko Sakkinen wrote: > On Mon, Mar 30, 2020 at 09:54:52PM +0300, Jarkko Sakkinen wrote: > > sgx_encl_may_map() always succeeding when PROT_NONE is given is not that > > useful behaviour as one can just well as do an anonymous mapping as > > demonstrated by the change in this patch to the test program. As a > > consequence, remove the special case. > > > > Pratically any possible way to make sure that you don't overwrite anything > > useful in the memory, should be fine. MAP_FIXED does not care what's > > underneath (if you want't it to care you ought to use > > MAP_FIXED_NO_REPLACE). > > > > After this change, the selftest run called sgx_mmap() only three times > > (TCS, text, data) instead of four. > > > > test_sgx-1811 [002] .... 586.907585: sgx_mmap <-mmap_region > > test_sgx-1811 [002] .... 586.911752: sgx_mmap <-mmap_region > > test_sgx-1811 [002] .... 586.911756: sgx_mmap <-mmap_region > > > > This also gives more angles to segregate enclave building and mapping as > > the mmap()'s need to be applied only when the enclave is fully built: > > > > Cc: luto@kernel.org > > Cc: Haitao Huang <haitao.huang@linux.intel.com> > > Cc: Sean Christopherson <sean.j.christopherson@intel.com> > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > Sean, would be cool if you can try this out soonish because I'm > building on top of this. Two and a half weeks is soonish, right? Anyways, finally tested this, no issues. Just to be different than the selftest, I used MAP_PRIVATE instead of MAP_SHARED along with MAP_ANONYMOUS in my test code.
On Fri, Apr 17, 2020 at 09:37:49PM -0700, Sean Christopherson wrote: > On Tue, Mar 31, 2020 at 12:13:36AM +0300, Jarkko Sakkinen wrote: > > On Mon, Mar 30, 2020 at 09:54:52PM +0300, Jarkko Sakkinen wrote: > > > sgx_encl_may_map() always succeeding when PROT_NONE is given is not that > > > useful behaviour as one can just well as do an anonymous mapping as > > > demonstrated by the change in this patch to the test program. As a > > > consequence, remove the special case. > > > > > > Pratically any possible way to make sure that you don't overwrite anything > > > useful in the memory, should be fine. MAP_FIXED does not care what's > > > underneath (if you want't it to care you ought to use > > > MAP_FIXED_NO_REPLACE). > > > > > > After this change, the selftest run called sgx_mmap() only three times > > > (TCS, text, data) instead of four. > > > > > > test_sgx-1811 [002] .... 586.907585: sgx_mmap <-mmap_region > > > test_sgx-1811 [002] .... 586.911752: sgx_mmap <-mmap_region > > > test_sgx-1811 [002] .... 586.911756: sgx_mmap <-mmap_region > > > > > > This also gives more angles to segregate enclave building and mapping as > > > the mmap()'s need to be applied only when the enclave is fully built: > > > > > > Cc: luto@kernel.org > > > Cc: Haitao Huang <haitao.huang@linux.intel.com> > > > Cc: Sean Christopherson <sean.j.christopherson@intel.com> > > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > > > Sean, would be cool if you can try this out soonish because I'm > > building on top of this. > > Two and a half weeks is soonish, right? > > Anyways, finally tested this, no issues. Just to be different than the > selftest, I used MAP_PRIVATE instead of MAP_SHARED along with MAP_ANONYMOUS > in my test code. Ah, MAP_SHARED was not intentional. Not gonna change it tho because both should equally work (or basically anything to guarante the availability of the address space). /Jarkko
diff --git a/Documentation/x86/sgx.rst b/Documentation/x86/sgx.rst index 79de1f01aa5b..9609a3409ad1 100644 --- a/Documentation/x86/sgx.rst +++ b/Documentation/x86/sgx.rst @@ -168,14 +168,11 @@ accounted to the processes of which behalf the kernel happened to be acting on. Access control ============== -`mmap()` permissions are capped by the enclave permissions. The consequences of -this is are: - -1. Pages for a VMA must built before `mmap()` can be applied with the - permissions required for running the enclave. -2. An address range for an enclave can be reserved with a `PROT_NONE` mapping - (not required but usually makes sense to guarantee that the used address - range is available). +`mmap()` permissions are capped by the enclave permissions. A direct +consequence of this is that all the pages for an address range must be added +before `mmap()` can be applied. Effectively an enclave page with minimum +permission in the address range sets the permission cap for the mapping +operation. Usage Models ============ diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c index d6a19bdd1921..e0124a2f22d5 100644 --- a/arch/x86/kernel/cpu/sgx/encl.c +++ b/arch/x86/kernel/cpu/sgx/encl.c @@ -288,8 +288,7 @@ static unsigned int sgx_vma_fault(struct vm_fault *vmf) * * Iterate through the enclave pages contained within [@start, @end) to verify * the permissions requested by @vm_prot_bits do not exceed that of any enclave - * page to be mapped. Page addresses that do not have an associated enclave - * page are interpreted to zero permissions. + * page to be mapped. * * Return: * 0 on success, @@ -308,10 +307,6 @@ int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start, if (!!(current->personality & READ_IMPLIES_EXEC)) return -EACCES; - /* PROT_NONE always succeeds. */ - if (!vm_prot_bits) - return 0; - idx_start = PFN_DOWN(start); idx_end = PFN_DOWN(end - 1); diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c index 12e1496f8a8b..3af0596530a8 100644 --- a/arch/x86/kernel/cpu/sgx/ioctl.c +++ b/arch/x86/kernel/cpu/sgx/ioctl.c @@ -481,15 +481,10 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src, * * 1. A regular page: PROT_R, PROT_W and PROT_X match the SECINFO permissions. * 2. A TCS page: PROT_R | PROT_W. - * 3. No page: PROT_NONE. * * mmap() is not allowed to surpass the minimum of the maximum protection bits * within the given address range. * - * As stated above, a non-existent page is interpreted as a page with no - * permissions. In effect, this allows mmap() with PROT_NONE to be used to seek - * an address range for the enclave that can be then populated into SECS. - * * If ENCLS opcode fails, that effectively means that EPC has been invalidated. * When this happens the enclave is destroyed and -EIO is returned to the * caller. diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c index 35a2d7a47dd5..53c565347e9e 100644 --- a/tools/testing/selftests/sgx/load.c +++ b/tools/testing/selftests/sgx/load.c @@ -231,7 +231,8 @@ static bool encl_map_area(struct encl *encl) size_t encl_size = encl->encl_size; void *area; - area = mmap(NULL, encl_size * 2, PROT_NONE, MAP_SHARED, encl->fd, 0); + area = mmap(NULL, encl_size * 2, PROT_NONE, + MAP_SHARED | MAP_ANONYMOUS, -1, 0); if (area == MAP_FAILED) { perror("mmap"); return false;
sgx_encl_may_map() always succeeding when PROT_NONE is given is not that useful behaviour as one can just well as do an anonymous mapping as demonstrated by the change in this patch to the test program. As a consequence, remove the special case. Pratically any possible way to make sure that you don't overwrite anything useful in the memory, should be fine. MAP_FIXED does not care what's underneath (if you want't it to care you ought to use MAP_FIXED_NO_REPLACE). After this change, the selftest run called sgx_mmap() only three times (TCS, text, data) instead of four. test_sgx-1811 [002] .... 586.907585: sgx_mmap <-mmap_region test_sgx-1811 [002] .... 586.911752: sgx_mmap <-mmap_region test_sgx-1811 [002] .... 586.911756: sgx_mmap <-mmap_region This also gives more angles to segregate enclave building and mapping as the mmap()'s need to be applied only when the enclave is fully built: Cc: luto@kernel.org Cc: Haitao Huang <haitao.huang@linux.intel.com> Cc: Sean Christopherson <sean.j.christopherson@intel.com> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> --- Documentation/x86/sgx.rst | 13 +++++-------- arch/x86/kernel/cpu/sgx/encl.c | 7 +------ arch/x86/kernel/cpu/sgx/ioctl.c | 5 ----- tools/testing/selftests/sgx/load.c | 3 ++- 4 files changed, 8 insertions(+), 20 deletions(-)