On Wed, Jun 19, 2019 at 03:23:53PM -0700, Sean Christopherson wrote:
> arch/x86/include/uapi/asm/sgx.h | 6 ++--
> arch/x86/kernel/cpu/sgx/driver/ioctl.c | 15 +++++---
> arch/x86/kernel/cpu/sgx/driver/main.c | 49 ++++++++++++++++++++++++++
> arch/x86/kernel/cpu/sgx/encl.h | 1 +
> tools/testing/selftests/x86/sgx/main.c | 32 +++++++++++++++--
Please split the kselftest change to a separate patch.
> 5 files changed, 94 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> index 6dba9f282232..67a3babbb24d 100644
> --- a/arch/x86/include/uapi/asm/sgx.h
> +++ b/arch/x86/include/uapi/asm/sgx.h
> @@ -35,15 +35,17 @@ struct sgx_enclave_create {
> * @src: address for the page data
> * @secinfo: address for the SECINFO data
> * @mrmask: bitmask for the measured 256 byte chunks
> + * @prot: maximal PROT_{READ,WRITE,EXEC} protections for the page
> */
> struct sgx_enclave_add_page {
> __u64 addr;
> __u64 src;
> __u64 secinfo;
> - __u64 mrmask;
> + __u16 mrmask;
> + __u8 prot;
> + __u8 pad;
__u8 pad[7];
> +/*
> + * Returns the AND of VM_{READ,WRITE,EXEC} permissions across all pages
> + * covered by the specific VMA. A non-existent (or yet to be added) enclave
> + * page is considered to have no RWX permissions, i.e. is inaccessible.
> + */
That was a bit hard to grasp (at least for me). I would rephrase it like:
/**
* sgx_calc_vma_prot_intersection() - Calculate intersection of the permissions
* for a VMA
* @encl: an enclave
* @vma: a VMA inside the enclave
*
* Iterate through the page addresses inside the VMA and calculate a bitmask
* of permissions that all pages have in common. Page addresses that do
* not have an associated enclave page are interpreted to zero
* permissions.
*/
> +static unsigned long sgx_allowed_rwx(struct sgx_encl *encl,
> + struct vm_area_struct *vma)
Suggestion for the name: sgx_calc_vma_prot_intersection()
> +{
> + unsigned long allowed_rwx = VM_READ | VM_WRITE | VM_EXEC;
> + unsigned long idx, idx_start, idx_end;
> + struct sgx_encl_page *page;
> +
> + idx_start = PFN_DOWN(vma->vm_start);
> + idx_end = PFN_DOWN(vma->vm_end - 1);
Suggestion: just open code these to the for-statement.
> +
> + for (idx = idx_start; idx <= idx_end; ++idx) {
> + /*
> + * No need to take encl->lock, vm_prot_bits is set prior to
> + * insertion and never changes, and racing with adding pages is
> + * a userspace bug.
> + */
> + rcu_read_lock();
> + page = radix_tree_lookup(&encl->page_tree, idx);
> + rcu_read_unlock();
> +
> + /* Do not allow R|W|X to a non-existent page. */
> + if (!page)
> + allowed_rwx = 0;
> + else
> + allowed_rwx &= page->vm_prot_bits;
This would be a more clean way to express the same:
if (!page)
return 0;
allowed_rwx &= page->vm_prot_bits;
/Jarkko
