Re: [RFC PATCH v4 04/12] x86/sgx: Require userspace to define enclave pages' protection bits

[Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>]

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