[intel-sgx-kernel-dev] intel_sgx: fix SECS page eviction
diff mbox

Message ID 20161215151802.10552-1-jarkko.sakkinen@linux.intel.com
State New
Headers show

Commit Message

Jarkko Sakkinen Dec. 15, 2016, 3:18 p.m. UTC
The SECS page was evicted to the first virtual address of ELRANGE. If
there was an EPC page there, evicting the SECS page would overwrite its
swapped copy.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/platform/x86/intel_sgx_ioctl.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Sean Christopherson Dec. 15, 2016, 4:50 p.m. UTC | #1
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> The SECS page was evicted to the first virtual address of ELRANGE. If there
> was an EPC page there, evicting the SECS page would overwrite its swapped
> copy.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  drivers/platform/x86/intel_sgx_ioctl.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/platform/x86/intel_sgx_ioctl.c
> b/drivers/platform/x86/intel_sgx_ioctl.c index 3a4a8fa..186d789 100644
> --- a/drivers/platform/x86/intel_sgx_ioctl.c
> +++ b/drivers/platform/x86/intel_sgx_ioctl.c
> @@ -556,6 +556,7 @@ static long sgx_ioc_enclave_create(struct file *filep,
> unsigned int cmd, goto out;
>  	}
>  
> +	encl->secs_page.addr = encl->base + encl->size;
>  	encl->secs_page.epc_page = secs_epc;
>  	createp->src = (unsigned long)encl->base;
>  

This change should have no effect, encl->secs_page.addr is already set to base+size via the call to sgx_init_page:

	ret = sgx_init_page(encl, &encl->secs_page,
			    encl->base + encl->size);
Jarkko Sakkinen Dec. 15, 2016, 5:30 p.m. UTC | #2
On Thu, Dec 15, 2016 at 04:50:46PM +0000, Christopherson, Sean J wrote:
> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> > The SECS page was evicted to the first virtual address of ELRANGE. If there
> > was an EPC page there, evicting the SECS page would overwrite its swapped
> > copy.
> > 
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > ---
> >  drivers/platform/x86/intel_sgx_ioctl.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/platform/x86/intel_sgx_ioctl.c
> > b/drivers/platform/x86/intel_sgx_ioctl.c index 3a4a8fa..186d789 100644
> > --- a/drivers/platform/x86/intel_sgx_ioctl.c
> > +++ b/drivers/platform/x86/intel_sgx_ioctl.c
> > @@ -556,6 +556,7 @@ static long sgx_ioc_enclave_create(struct file *filep,
> > unsigned int cmd, goto out;
> >  	}
> >  
> > +	encl->secs_page.addr = encl->base + encl->size;
> >  	encl->secs_page.epc_page = secs_epc;
> >  	createp->src = (unsigned long)encl->base;
> >  
> 
> This change should have no effect, encl->secs_page.addr is already set to base+size via the call to sgx_init_page:
> 
> 	ret = sgx_init_page(encl, &encl->secs_page,
> 			    encl->base + encl->size);

 Right good :) I thought it must have been set somewhere but missed it.
 I mean presumably SECS eviction has been working for ages but there's
 been some things that went wrong here and there when this stuff was
 opened so I quickly thought that this might be one of them.

 /Jarkko

Patch
diff mbox

diff --git a/drivers/platform/x86/intel_sgx_ioctl.c b/drivers/platform/x86/intel_sgx_ioctl.c
index 3a4a8fa..186d789 100644
--- a/drivers/platform/x86/intel_sgx_ioctl.c
+++ b/drivers/platform/x86/intel_sgx_ioctl.c
@@ -556,6 +556,7 @@  static long sgx_ioc_enclave_create(struct file *filep, unsigned int cmd,
 		goto out;
 	}
 
+	encl->secs_page.addr = encl->base + encl->size;
 	encl->secs_page.epc_page = secs_epc;
 	createp->src = (unsigned long)encl->base;