| Message ID | 66da195d44cbbed57b6840c5d20bb789c06fb99f.1638381245.git.reinette.chatre@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sgx and selftests/sgx: Support SGX2 | expand |
On Wed, Dec 01, 2021 at 11:23:12AM -0800, Reinette Chatre wrote: > Before an enclave is initialized the enclave's memory range is unknown. > The enclave's memory range is learned at the time it is created via the > SGX_IOC_ENCLAVE_CREATE ioctl where the provided memory range is obtained > from an earlier mmap() of the sgx_enclave device. After an enclave is > initialized its memory can be mapped into user space (mmap()) from where > it can be entered at its defined entry points. > > With the enclave's memory range known after it is initialized there is > no reason why it should be possible to map memory outside this range. > > Lock down access to the initialized enclave's memory range by denying > any attempt to map memory outside its memory range. > > Locking down the memory range also makes adding pages to an initialized > enclave more efficient. Pages are added to an initialized enclave by > accessing memory that belongs to the enclave's memory range but not yet > backed by an enclave page. If it is possible for user space to map > memory that does not form part of the enclave then an access to this > memory would eventually fail. Failures range from a prompt general > protection fault if the access was an ENCLU[EACCEPT] from within the > enclave, or a page fault via the vDSO if it was another access from > within the enclave, or a SIGBUS (also resulting from a page fault) if > the access was from outside the enclave. > > Disallowing invalid memory to be mapped in the first place avoids > preventable failures. > > Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> > --- > arch/x86/kernel/cpu/sgx/encl.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c > index 342b97dd4c33..37203da382f8 100644 > --- a/arch/x86/kernel/cpu/sgx/encl.c > +++ b/arch/x86/kernel/cpu/sgx/encl.c > @@ -403,6 +403,10 @@ int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start, > > XA_STATE(xas, &encl->page_array, PFN_DOWN(start)); > Please write a comment here. > + if (test_bit(SGX_ENCL_INITIALIZED, &encl->flags) && > + (start < encl->base || end > encl->base + encl->size)) > + return -EACCES; > + > /* > * Disallow READ_IMPLIES_EXEC tasks as their VMA permissions might > * conflict with the enclave page permissions. > -- > 2.25.1 > Otherwise, makes sense. /Jarkko
Hi Jarkko, On 12/4/2021 3:14 PM, Jarkko Sakkinen wrote: >> diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c >> index 342b97dd4c33..37203da382f8 100644 >> --- a/arch/x86/kernel/cpu/sgx/encl.c >> +++ b/arch/x86/kernel/cpu/sgx/encl.c >> @@ -403,6 +403,10 @@ int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start, >> >> XA_STATE(xas, &encl->page_array, PFN_DOWN(start)); >> > > Please write a comment here. Would the comment below suffice? /* Disallow mapping outside enclave's address range. */ > >> + if (test_bit(SGX_ENCL_INITIALIZED, &encl->flags) && >> + (start < encl->base || end > encl->base + encl->size)) >> + return -EACCES; >> + >> /* >> * Disallow READ_IMPLIES_EXEC tasks as their VMA permissions might >> * conflict with the enclave page permissions. >> -- >> 2.25.1 >> > > Otherwise, makes sense. > Thank you Reinette
On Mon, 2021-12-06 at 13:45 -0800, Reinette Chatre wrote: > Hi Jarkko, > > On 12/4/2021 3:14 PM, Jarkko Sakkinen wrote: > > > diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c > > > index 342b97dd4c33..37203da382f8 100644 > > > --- a/arch/x86/kernel/cpu/sgx/encl.c > > > +++ b/arch/x86/kernel/cpu/sgx/encl.c > > > @@ -403,6 +403,10 @@ int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start, > > > > > > XA_STATE(xas, &encl->page_array, PFN_DOWN(start)); > > > > > > > Please write a comment here. > > Would the comment below suffice? > > /* Disallow mapping outside enclave's address range. */ Yeah, looks good to me. /Jarkko
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c index 342b97dd4c33..37203da382f8 100644 --- a/arch/x86/kernel/cpu/sgx/encl.c +++ b/arch/x86/kernel/cpu/sgx/encl.c @@ -403,6 +403,10 @@ int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start, XA_STATE(xas, &encl->page_array, PFN_DOWN(start)); + if (test_bit(SGX_ENCL_INITIALIZED, &encl->flags) && + (start < encl->base || end > encl->base + encl->size)) + return -EACCES; + /* * Disallow READ_IMPLIES_EXEC tasks as their VMA permissions might * conflict with the enclave page permissions.
Before an enclave is initialized the enclave's memory range is unknown. The enclave's memory range is learned at the time it is created via the SGX_IOC_ENCLAVE_CREATE ioctl where the provided memory range is obtained from an earlier mmap() of the sgx_enclave device. After an enclave is initialized its memory can be mapped into user space (mmap()) from where it can be entered at its defined entry points. With the enclave's memory range known after it is initialized there is no reason why it should be possible to map memory outside this range. Lock down access to the initialized enclave's memory range by denying any attempt to map memory outside its memory range. Locking down the memory range also makes adding pages to an initialized enclave more efficient. Pages are added to an initialized enclave by accessing memory that belongs to the enclave's memory range but not yet backed by an enclave page. If it is possible for user space to map memory that does not form part of the enclave then an access to this memory would eventually fail. Failures range from a prompt general protection fault if the access was an ENCLU[EACCEPT] from within the enclave, or a page fault via the vDSO if it was another access from within the enclave, or a SIGBUS (also resulting from a page fault) if the access was from outside the enclave. Disallowing invalid memory to be mapped in the first place avoids preventable failures. Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> --- arch/x86/kernel/cpu/sgx/encl.c | 4 ++++ 1 file changed, 4 insertions(+)