| Message ID | 20190819152544.7296-2-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sgx: Improve permission handing | expand |
On Mon, Aug 19, 2019 at 06:25:40PM +0300, Jarkko Sakkinen wrote: > The way permissions are managed inside the driver is not trivial and > intuitive. The non-obvious parts were not properly remarked in the > source code. This patch refines them a bit. > > Cc: Sean Christopherson <sean.j.christpherson@intel.com> > Cc: Shay Katz-zamir <shay.katz-zamir@intel.com> > Cc: Serge Ayoun <serge.ayoun@intel.com> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > --- > arch/x86/kernel/cpu/sgx/driver/ioctl.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c > index 9b784a061a47..64d3286f3324 100644 > --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c > +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c > @@ -259,7 +259,10 @@ static struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl, > return ERR_PTR(-ENOMEM); > encl_page->desc = addr; > encl_page->encl = encl; > + > + /* Calculate maximum of the VM flags for the page. */ > encl_page->vm_prot_bits = calc_vm_prot_bits(prot, 0); > + > ret = radix_tree_insert(&encl->page_tree, PFN_DOWN(encl_page->desc), > encl_page); > if (ret) { > @@ -640,11 +643,15 @@ static long sgx_ioc_enclave_add_page(struct file *filep, void __user *arg) > > data = kmap(data_page); > > + /* Set prot bits matching to the SECINFO permissions. */ > prot = _calc_vm_trans(secinfo.flags, SGX_SECINFO_R, PROT_READ) | > _calc_vm_trans(secinfo.flags, SGX_SECINFO_W, PROT_WRITE) | > _calc_vm_trans(secinfo.flags, SGX_SECINFO_X, PROT_EXEC); > > - /* TCS pages need to be RW in the PTEs, but can be 0 in the EPCM. */ > + /* TCS pages must always RW set for CPU access while the SECINFO Should be /* * TCS pages ... > + * permissions are *always* zero - the CPU ignores the user provided > + * values and silently overwrites zero permissions. Maybe 'overwrites with zero permissions'? > + */ > if ((secinfo.flags & SGX_SECINFO_PAGE_TYPE_MASK) == SGX_SECINFO_TCS) > prot |= PROT_READ | PROT_WRITE; > > -- > 2.20.1 >
On Wed, 2019-08-21 at 20:43 -0700, Sean Christopherson wrote: > > - /* TCS pages need to be RW in the PTEs, but can be 0 in the EPCM. */ > > + /* TCS pages must always RW set for CPU access while the SECINFO > > Should be > > /* > * TCS pages ... > > > + * permissions are *always* zero - the CPU ignores the user provided > > + * values and silently overwrites zero permissions. > > Maybe 'overwrites with zero permissions'? I squashed this with the proposed changes made. Thank you. /Jarkko
diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c index 9b784a061a47..64d3286f3324 100644 --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c @@ -259,7 +259,10 @@ static struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl, return ERR_PTR(-ENOMEM); encl_page->desc = addr; encl_page->encl = encl; + + /* Calculate maximum of the VM flags for the page. */ encl_page->vm_prot_bits = calc_vm_prot_bits(prot, 0); + ret = radix_tree_insert(&encl->page_tree, PFN_DOWN(encl_page->desc), encl_page); if (ret) { @@ -640,11 +643,15 @@ static long sgx_ioc_enclave_add_page(struct file *filep, void __user *arg) data = kmap(data_page); + /* Set prot bits matching to the SECINFO permissions. */ prot = _calc_vm_trans(secinfo.flags, SGX_SECINFO_R, PROT_READ) | _calc_vm_trans(secinfo.flags, SGX_SECINFO_W, PROT_WRITE) | _calc_vm_trans(secinfo.flags, SGX_SECINFO_X, PROT_EXEC); - /* TCS pages need to be RW in the PTEs, but can be 0 in the EPCM. */ + /* TCS pages must always RW set for CPU access while the SECINFO + * permissions are *always* zero - the CPU ignores the user provided + * values and silently overwrites zero permissions. + */ if ((secinfo.flags & SGX_SECINFO_PAGE_TYPE_MASK) == SGX_SECINFO_TCS) prot |= PROT_READ | PROT_WRITE;
The way permissions are managed inside the driver is not trivial and intuitive. The non-obvious parts were not properly remarked in the source code. This patch refines them a bit. Cc: Sean Christopherson <sean.j.christpherson@intel.com> Cc: Shay Katz-zamir <shay.katz-zamir@intel.com> Cc: Serge Ayoun <serge.ayoun@intel.com> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> --- arch/x86/kernel/cpu/sgx/driver/ioctl.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)