RE: [PATCH v21 16/28] x86/sgx: Add the Linux SGX Enclave Driver

["Ayoun, Serge" <serge.ayoun@intel.com>]

> From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Sent: Saturday, July 13, 2019 20:08
> Subject: [PATCH v21 16/28] x86/sgx: Add the Linux SGX Enclave Driver

> +static long sgx_ioc_enclave_add_page(struct file *filep, void __user
> +*arg) {
> +	struct sgx_encl *encl = filep->private_data;
> +	struct sgx_enclave_add_page addp;
> +	struct sgx_secinfo secinfo;
> +	struct page *data_page;
> +	unsigned long prot;
> +	void *data;
> +	int ret;
> +
> +	if (copy_from_user(&addp, arg, sizeof(addp)))
> +		return -EFAULT;
> +
> +	if (copy_from_user(&secinfo, (void __user *)addp.secinfo,
> +			   sizeof(secinfo)))
> +		return -EFAULT;
> +
> +	data_page = alloc_page(GFP_HIGHUSER);
> +	if (!data_page)
> +		return -ENOMEM;
> +
> +	data = kmap(data_page);
> +
> +	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. */
> +	if ((secinfo.flags & SGX_SECINFO_PAGE_TYPE_MASK) ==
> SGX_SECINFO_TCS)
> +		prot |= PROT_READ | PROT_WRITE;

For TCS pages you add both RD and WR maximum protection bits.
For the enclave to be able to run, user mode will have to change the "vma->vm_flags" from PROT_NONE to PROT_READ | PROT_WRITE (otherwise eenter fails). 
This is exactly what your selftest  does.
But when mmap (or mprotect) is called with PROT_READ bit, it automatically adds the PROT_EXEC bit unless the host application has been compiled with '-z noexecstack' option; pasting below the mmap() code which does it:

	if ((prot & PROT_READ) && (current->personality & READ_IMPLIES_EXEC))
		if (!(file && path_noexec(&file->f_path)))
			prot |= PROT_EXEC;

The problem is that if PROT_EXEC bit is added then sgx_mmap callback will fail since PROT_EXEC will get blocked by your code and not allowed for TCS pages.
This restriction is not necessary at all, i.e. I wouldn't block PROT_EXEC on tcs area because anyway, the hardware will never let those areas to execute: the SGX protection flags are fixed by the cpu and can not be changed by any mean.
So in order to facilitate user's interface I would let prot |= PROT_READ | PROT_WRITE | PROT_EXEC; we do not give up to any security criteria and make user interaction easier.

> +
> +	ret = sgx_encl_page_import_user(data, addp.src, prot);
> +	if (ret)
> +		goto out;
> +
> +	ret = sgx_encl_add_page(encl, addp.addr, data, &secinfo,
> addp.mrmask,
> +				prot);
> +	if (ret)
> +		goto out;
> +
> +out:
> +	kunmap(data_page);
> +	__free_page(data_page);
> +	return ret;
> +}
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.