diff mbox series

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

Message ID 20190531233159.30992-7-sean.j.christopherson@intel.com (mailing list archive)
State Superseded
Headers show
Series security: x86/sgx: SGX vs. LSM | expand

Commit Message

Sean Christopherson May 31, 2019, 11:31 p.m. UTC
...to support (the equivalent) of existing Linux Security Module
functionality.

Because SGX manually manages EPC memory, all enclave VMAs are backed by
the same vm_file, i.e. /dev/sgx/enclave, so that SGX can implement the
necessary hooks to move pages in/out of the EPC.  And because EPC pages
for any given enclave are fundamentally shared between processes, i.e.
CoW semantics are not possible with EPC pages, /dev/sgx/enclave must
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.

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.

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.

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(-)

Comments

Xing, Cedric June 3, 2019, 6:28 a.m. UTC | #1
> From: Christopherson, Sean J
> Sent: Friday, May 31, 2019 4:32 PM
> 
> ...to support (the equivalent) of existing Linux Security Module functionality.
> 
> Because SGX manually manages EPC memory, all enclave VMAs are backed by the same vm_file,
> i.e. /dev/sgx/enclave, so that SGX can implement the necessary hooks to move pages in/out
> of the EPC.  And because EPC pages for any given enclave are fundamentally shared between
> processes, i.e.
> CoW semantics are not possible with EPC pages, /dev/sgx/enclave must 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.
> 
> 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.
> 
> 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.
> 
> 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.

ALLOW_* flags shall be kept internal to LSM.

This patch is completely unnecessary.
Jarkko Sakkinen June 4, 2019, 4:23 p.m. UTC | #2
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.

> 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.

> 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.

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?

> +
>  /**
>   * 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;
> +};
>  
>  /**
>   * struct sgx_enclave_init - parameter structure for the
> diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> index 6acfcbdeca9a..c30acd3fbbdd 100644
> --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> @@ -235,7 +235,8 @@ static int sgx_validate_secs(const struct sgx_secs *secs,
>  }
>  
>  static struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl,
> -						 unsigned long addr)
> +						 unsigned long addr,
> +						 unsigned long allowed_prot)
>  {
>  	struct sgx_encl_page *encl_page;
>  	int ret;
> @@ -247,6 +248,7 @@ 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;
> +	encl_page->allowed_prot = allowed_prot;
>  	ret = radix_tree_insert(&encl->page_tree, PFN_DOWN(encl_page->desc),
>  				encl_page);
>  	if (ret) {
> @@ -530,7 +532,7 @@ static int sgx_encl_queue_page(struct sgx_encl *encl,
>  
>  static int __sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
>  			       void *data, struct sgx_secinfo *secinfo,
> -			       unsigned int mrmask)
> +			       unsigned int mrmask, unsigned long allowed_prot)
>  {
>  	u64 page_type = secinfo->flags & SGX_SECINFO_PAGE_TYPE_MASK;
>  	struct sgx_encl_page *encl_page;
> @@ -556,7 +558,7 @@ static int __sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
>  		goto out;
>  	}
>  
> -	encl_page = sgx_encl_page_alloc(encl, addr);
> +	encl_page = sgx_encl_page_alloc(encl, addr, allowed_prot);
>  	if (IS_ERR(encl_page)) {
>  		ret = PTR_ERR(encl_page);
>  		goto out;
> @@ -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.

> +	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?

>  	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?

> +
> +	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.

>  
> 
> 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
Sean Christopherson June 4, 2019, 4:45 p.m. UTC | #3
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
Andy Lutomirski June 4, 2019, 8:23 p.m. UTC | #4
On Fri, May 31, 2019 at 4:32 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> ...to support (the equivalent) of existing Linux Security Module
> functionality.
>
> Because SGX manually manages EPC memory, all enclave VMAs are backed by
> the same vm_file, i.e. /dev/sgx/enclave, so that SGX can implement the
> necessary hooks to move pages in/out of the EPC.  And because EPC pages
> for any given enclave are fundamentally shared between processes, i.e.
> CoW semantics are not possible with EPC pages, /dev/sgx/enclave must
> 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.
>
> 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.
>
> 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.
>
> 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
> +
>  /**
>   * 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;

Minor nit: I would use at least u32 for flags.  It's not like the size
saving is important.

>  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);
> +       unsigned long allowed_prot = flags & (VM_READ | VM_WRITE | VM_EXEC);

...

> +       if (prot & ~allowed_prot)
> +               return -EACCES;

This seems like a well-intentioned sanity check, but it doesn't really
accomplish anything with SGX2, so I tend to think it would be better
to omit it.
Ayoun, Serge June 5, 2019, 11:10 a.m. UTC | #5
> From: Christopherson, Sean J
> Sent: Saturday, June 01, 2019 02:32
> 
>  /**
>   * 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;
> +};

You are adding a flags member. The secinfo structure has already a flags member in it.
Why do you need both - they are both coming from user mode. What kind of scenario would
require having different values. Seems confusing.

---------------------------------------------------------------------
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.
Jarkko Sakkinen June 5, 2019, 3:06 p.m. UTC | #6
On Tue, Jun 04, 2019 at 09:45:14AM -0700, Sean Christopherson wrote:
> 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().

I would suggest describing explicitly in the commit message what you
want to do, which you said here e.g. "I do this because I want to add
LSM hooks". This also relevant information for the LKM discussion.

Lets see how the next version looks like now that you have some
feedback.

In the whole scope of the patch set, in order to make it more
readable, I'll give following suggestions on how it is organized:

1. Leave out anything that is not strictly necessary (cosmetic
fix, batch operation if possible). Better to focus one thing at
a time.
2. Try to organize it so that each function is fully defined in
the scope of one patch even if it would mean larger patches.
3. Do not add one call site helpers unless there is a good
reason to do so. A good reason would be something like needing
to extensive work in error rollback, which would make the
caller a mess.

/Jarkko
Sean Christopherson June 5, 2019, 11:58 p.m. UTC | #7
On Wed, Jun 05, 2019 at 04:10:44AM -0700, Ayoun, Serge wrote:
> > From: Christopherson, Sean J
> > Sent: Saturday, June 01, 2019 02:32
> > 
> >  /**
> >   * 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;
> > +};
> 
> You are adding a flags member. The secinfo structure has already a flags member in it.
> Why do you need both - they are both coming from user mode. What kind of scenario would
> require having different values. Seems confusing.

The format of SECINFO.FLAGS is hardware defined, e.g. we can't add a flag
to tag the page as being a zero page for optimization purposes, at least
not without breaking future compatibility or doing tricky overloading.

If you're specifically talking about SECINFO.FLAGS.RWX, due to SGX2 there
are scenarios where userspace will initially want the page to be RW, and
will later want to convert the page to RX.  Making decisions based solely
on the initial EPCM permissions would either create a security hole or
force SGX to track "dirty" pages along with a implementing a pre-check
scheme for LSMs (or restricting LSMs to tieing permissions to the host
process and not the enclave).
diff mbox series

Patch

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
+
 /**
  * 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;
+};
 
 /**
  * struct sgx_enclave_init - parameter structure for the
diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index 6acfcbdeca9a..c30acd3fbbdd 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -235,7 +235,8 @@  static int sgx_validate_secs(const struct sgx_secs *secs,
 }
 
 static struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl,
-						 unsigned long addr)
+						 unsigned long addr,
+						 unsigned long allowed_prot)
 {
 	struct sgx_encl_page *encl_page;
 	int ret;
@@ -247,6 +248,7 @@  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;
+	encl_page->allowed_prot = allowed_prot;
 	ret = radix_tree_insert(&encl->page_tree, PFN_DOWN(encl_page->desc),
 				encl_page);
 	if (ret) {
@@ -530,7 +532,7 @@  static int sgx_encl_queue_page(struct sgx_encl *encl,
 
 static int __sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
 			       void *data, struct sgx_secinfo *secinfo,
-			       unsigned int mrmask)
+			       unsigned int mrmask, unsigned long allowed_prot)
 {
 	u64 page_type = secinfo->flags & SGX_SECINFO_PAGE_TYPE_MASK;
 	struct sgx_encl_page *encl_page;
@@ -556,7 +558,7 @@  static int __sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
 		goto out;
 	}
 
-	encl_page = sgx_encl_page_alloc(encl, addr);
+	encl_page = sgx_encl_page_alloc(encl, addr, allowed_prot);
 	if (IS_ERR(encl_page)) {
 		ret = PTR_ERR(encl_page);
 		goto out;
@@ -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);
+	unsigned long allowed_prot = flags & (VM_READ | VM_WRITE | VM_EXEC);
 	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);
+
+	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;
 	}
 
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;