Re: [RFC PATCH 6/9] x86/sgx: Require userspace to provide allowed prots to ADD_PAGES

[Sean Christopherson <sean.j.christopherson@intel.com>]

On Tue, Jun 04, 2019 at 07:23:06PM +0300, Jarkko Sakkinen wrote:
> On Fri, May 31, 2019 at 04:31:56PM -0700, Sean Christopherson wrote:
> > ...to support (the equivalent) of existing Linux Security Module
> > functionality.
> 
> Long and short descriptions should be separate. Also this does not
> make any sense. LSM is a framework with a set of hook to make access
> decisions and there various implementations of it.
> 
> How this replicates LSMs and why that even would be a goal?
> 
> My guess is that you are trying to do something else. I'm just saying
> that the idea to do equivalent of LSMs to another subsystems would be
> insane if it was done.

Heh, yeah, it's not duplicating LSM functionality.  What I was trying to
say is that this patch allows LSMs to implement policies that are
equivalent to their existing functionality, e.g. paves the way to add
security_enclave_load() as an equivalent to security_file_mprotect().

> > always be MAP_SHARED.  Lastly, all real world enclaves will need read,
> > write and execute permissions to EPC pages.  As a result, SGX does not
> > play nice with existing LSM behavior as it is impossible to apply
> > policies to enclaves with any reasonable granularity, e.g. an LSM can
> > deny access to EPC altogether, but can't deny potentially dangerous
> > behavior such as mapping pages RW->RW or RWX.
> 
> The mapping must be shared given that it is iomem but why enclave pages
> would need RWX for all pages? The information that is missing from this
> paragraph is the explanation why an LSM could not deny dangerous
> behavior in PTE level.

I'll add that.

> > To give LSMs enough information to implement their policies without
> > having to resort to ugly things, e.g. holding a reference to the vm_file
> > of each enclave page, require userspace to explicitly state the allowed
> > protections for each page (region), i.e. take ALLOW_{READ,WRITE,EXEC}
> > in the ADD_PAGES ioctl.
> 
> I would keep descriptions such as "ugly things" away from commit
> messages as it is easy way to be not clear and explicit what you are
> trying to say.
> 
> > The ALLOW_* flags will be passed to LSMs so that they can make informed
> > decisions when the enclave is being built, i.e. when the source vm_file
> > is available.  For example, SELinux's EXECMOD permission can be
> > required if an enclave is requesting both ALLOW_WRITE and ALLOW_EXEC.
> 
> There should be some explanation what ALLOW_* flag are. It is now like
> as it was in common knowledge. SECINFO already has protection flags to
> name an example and without any explanation all of this is just very
> confusing.

Noted.

> This should address SECINFO and ALLOW_* relationship and differences.
> 
> > Update the mmap()/mprotect() hooks to enforce the ALLOW_* protections,
> > a la the standard VM_MAY{READ,WRITE,EXEC} flags.
> > 
> > The ALLOW_EXEC flag also has a second (important) use in that it can
> > be used to prevent loading an enclave from a noexec file system, on
> > SGX2 hardware (regardless of kernel support for SGX2), userspace could
> > EADD from a noexec path using read-only permissions and later mprotect()
> > and ENCLU[EMODPE] the page to gain execute permissions.  By requiring
> > ALLOW_EXEC up front, SGX will be able to enforce noexec paths when
> > building the enclave.
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > ---
> >  arch/x86/include/uapi/asm/sgx.h        |  9 ++++++++-
> >  arch/x86/kernel/cpu/sgx/driver/ioctl.c | 23 +++++++++++++++++------
> >  arch/x86/kernel/cpu/sgx/encl.c         |  2 +-
> >  arch/x86/kernel/cpu/sgx/encl.h         |  1 +
> >  4 files changed, 27 insertions(+), 8 deletions(-)
> > 
> > diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> > index 4a12d6abbcb7..4489e92fa0dc 100644
> > --- a/arch/x86/include/uapi/asm/sgx.h
> > +++ b/arch/x86/include/uapi/asm/sgx.h
> > @@ -31,6 +31,11 @@ struct sgx_enclave_create  {
> >  	__u64	src;
> >  };
> >  
> > +/* Supported flags for struct sgx_enclave_add_pages. */
> > +#define SGX_ALLOW_READ	VM_READ
> > +#define SGX_ALLOW_WRITE	VM_WRITE
> > +#define SGX_ALLOW_EXEC	VM_EXEC
> 
> Why these flags are even defined if they are the same as VM_* flags?

Brain fart.  Flags can just take PROT_{READ,WRITE,EXEC}.

> > +
> >  /**
> >   * struct sgx_enclave_add_pages - parameter structure for the
> >   *                                %SGX_IOC_ENCLAVE_ADD_PAGES ioctl
> > @@ -39,6 +44,7 @@ struct sgx_enclave_create  {
> >   * @secinfo:	address for the SECINFO data (common to all pages)
> >   * @nr_pages:	number of pages (must be virtually contiguous)
> >   * @mrmask:	bitmask for the measured 256 byte chunks (common to all pages)
> > + * @flags:	flags, e.g. SGX_ALLOW_{READ,WRITE,EXEC} (common to all pages)
> >   */
> >  struct sgx_enclave_add_pages {
> >  	__u64	addr;
> > @@ -46,7 +52,8 @@ struct sgx_enclave_add_pages {
> >  	__u64	secinfo;
> >  	__u32	nr_pages;
> >  	__u16	mrmask;
> > -} __attribute__((__packed__));
> > +	__u16	flags;
> > +};
> >  

...

> > @@ -576,12 +578,20 @@ static int __sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
> >  
> >  static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
> >  			     unsigned long src, struct sgx_secinfo *secinfo,
> > -			     unsigned int mrmask)
> > +			     unsigned int mrmask, unsigned int flags)
> >  {
> > +	unsigned long prot = secinfo->flags & (VM_READ | VM_WRITE | VM_EXEC);
> 
> Even if the secinfo flags have the exactly the same values you should
> not do this as they are kind of from different type. This is confusing
> to read.

I can add a dummy helper to translate flags and encapsulate the below
assert.

> > +	unsigned long allowed_prot = flags & (VM_READ | VM_WRITE | VM_EXEC);
> 
> Why you take the trouble defining those macros and do not then use them
> even yourself?

The original thought was to define them for userspace, but that's broken
because VM_* aren't defined for userspace.

> >  	struct page *data_page;
> >  	void *data;
> >  	int ret;
> >  
> > +	BUILD_BUG_ON(SGX_SECINFO_R != VM_READ || SGX_SECINFO_W != VM_WRITE ||
> > +		     SGX_SECINFO_X != VM_EXEC);
> 
> Why this check?

To assert that the hardware defined SECINFO flags are interchangeable with
Linux's software defined flags, i.e. don't need to be translated.

> 
> > +
> > +	if (prot & ~allowed_prot)
> > +		return -EACCES;
> > +
> >  	data_page = alloc_page(GFP_HIGHUSER);
> >  	if (!data_page)
> >  		return -ENOMEM;
> > @@ -593,7 +603,8 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
> >  		goto out;
> >  	}
> >  
> > -	ret = __sgx_encl_add_page(encl, addr, data, secinfo, mrmask);
> > +	ret = __sgx_encl_add_page(encl, addr, data, secinfo, mrmask,
> > +				  allowed_prot);
> >  out:
> >  	kunmap(data_page);
> >  	__free_page(data_page);
> > @@ -645,7 +656,7 @@ static long sgx_ioc_enclave_add_pages(struct file *filep, unsigned int cmd,
> >  
> >  		ret = sgx_encl_add_page(encl, addp->addr + i*PAGE_SIZE,
> >  					addp->src + i*PAGE_SIZE,
> > -					&secinfo, addp->mrmask);
> > +					&secinfo, addp->mrmask, addp->flags);
> >  	}
> >  	return ret;
> >  }
> > diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> > index 955d4f430adc..e5847571a265 100644
> > --- a/arch/x86/kernel/cpu/sgx/encl.c
> > +++ b/arch/x86/kernel/cpu/sgx/encl.c
> > @@ -249,7 +249,7 @@ int sgx_map_allowed(struct sgx_encl *encl, unsigned long start,
> >  
> >  	for (addr = start; addr < end; addr += PAGE_SIZE) {
> >  		page = radix_tree_lookup(&encl->page_tree, addr >> PAGE_SHIFT);
> > -		if (!page)
> > +		if (!page || (prot & ~page->allowed_prot))
> >  			return -EACCES;
> >  	}
> 
> However this goes it would be good idea to have only ony patch in the
> patch set that fully defines this function. Impossible to review
> properly with this split.

Sorry, I don't understand what you're suggesting.

> 
> >  
> > 
> > diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h
> > index 6e310e3b3fff..7cca076a4987 100644
> > --- a/arch/x86/kernel/cpu/sgx/encl.h
> > +++ b/arch/x86/kernel/cpu/sgx/encl.h
> > @@ -41,6 +41,7 @@ enum sgx_encl_page_desc {
> >  
> >  struct sgx_encl_page {
> >  	unsigned long desc;
> > +	unsigned long allowed_prot;
> >  	struct sgx_epc_page *epc_page;
> >  	struct sgx_va_page *va_page;
> >  	struct sgx_encl *encl;
> > -- 
> > 2.21.0
> > 
> 
> This patch left me very confused. I don't get it.
> 
> /Jarkko