[RFC,v3,02/12] x86/sgx: Do not naturally align MAP_FIXED address
diff mbox series

Message ID 20190617222438.2080-3-sean.j.christopherson@intel.com
State New
Headers show
Series
  • security: x86/sgx: SGX vs. LSM, round 3
Related show

Commit Message

Sean Christopherson June 17, 2019, 10:24 p.m. UTC
SGX enclaves have an associated Enclave Linear Range (ELRANGE) that is
tracked and enforced by the CPU using a base+mask approach, similar to
how hardware range registers such as the variable MTRRs.  As a result,
the ELRANGE must be naturally sized and aligned.

To reduce boilerplate code that would be needed in every userspace
enclave loader, the SGX driver naturally aligns the mmap() address and
also requires the range to be naturally sized.  Unfortunately, SGX fails
to grant a waiver to the MAP_FIXED case, e.g. incorrectly rejects mmap()
if userspace is attempting to map a small slice of an existing enclave.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kernel/cpu/sgx/driver/main.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Jarkko Sakkinen June 19, 2019, 1:24 p.m. UTC | #1
On Mon, 2019-06-17 at 15:24 -0700, Sean Christopherson wrote:
>  {
> -	if (len < 2 * PAGE_SIZE || len & (len - 1) || flags & MAP_PRIVATE)
> +	if (flags & MAP_PRIVATE)
> +		return -EINVAL;
> +
> +	if (flags & MAP_FIXED)
> +		return addr;
> +
> +	if (len < 2 * PAGE_SIZE || len & (len - 1))
>  		return -EINVAL;
>
>  	addr = current->mm->get_unmapped_area(file, addr, 2 * len, pgoff,

Just sanity checking that for MAP_FIXED case the mm checks that the area is
unmapped before calling this?

I don't think we need to check any alignment constraints here anymore.

The summarize end result would be:

static unsigned long sgx_get_unmapped_area(struct file *file,
					   unsigned long addr,
					   unsigned long len,
					   unsigned long pgoff,
					   unsigned long flags)
{
	if (flags & MAP_PRIVATE)
		return -EINVAL;

	if (flags & MAP_FIXED)
		return addr;

	return current->mm->get_unmapped_area(file, addr, 2 * len, pgoff,
					      flags);
}

/Jarkko
Sean Christopherson June 19, 2019, 2:08 p.m. UTC | #2
On Wed, Jun 19, 2019 at 04:24:05PM +0300, Jarkko Sakkinen wrote:
> On Mon, 2019-06-17 at 15:24 -0700, Sean Christopherson wrote:
> >  {
> > -	if (len < 2 * PAGE_SIZE || len & (len - 1) || flags & MAP_PRIVATE)
> > +	if (flags & MAP_PRIVATE)
> > +		return -EINVAL;
> > +
> > +	if (flags & MAP_FIXED)
> > +		return addr;
> > +
> > +	if (len < 2 * PAGE_SIZE || len & (len - 1))
> >  		return -EINVAL;
> >
> >  	addr = current->mm->get_unmapped_area(file, addr, 2 * len, pgoff,
> 
> Just sanity checking that for MAP_FIXED case the mm checks that the area is
> unmapped before calling this?

No, straight MAP_FIXED unmaps any existing mappings.  The NOREPLACE variant
fails with -EEXIST if there are existing mappings.

The MAP_FIXED behavior is actually useful, bordering on mandatory, for the
new flow.  It allows the loader to keep its initial mmap(PROT_NONE) of
ELRANGE while (re)mapping the individual enclave sections, e.g. to prevent
a different aspect of the process from mapping the require ELRANGE.

> 
> I don't think we need to check any alignment constraints here anymore.
> 
> The summarize end result would be:
> 
> static unsigned long sgx_get_unmapped_area(struct file *file,
> 					   unsigned long addr,
> 					   unsigned long len,
> 					   unsigned long pgoff,
> 					   unsigned long flags)
> {
> 	if (flags & MAP_PRIVATE)
> 		return -EINVAL;
> 
> 	if (flags & MAP_FIXED)
> 		return addr;
> 
> 	return current->mm->get_unmapped_area(file, addr, 2 * len, pgoff,
> 					      flags);
> }
> 
> /Jarkko
>
Jarkko Sakkinen June 20, 2019, 10:07 p.m. UTC | #3
On Wed, Jun 19, 2019 at 07:08:53AM -0700, Sean Christopherson wrote:
> On Wed, Jun 19, 2019 at 04:24:05PM +0300, Jarkko Sakkinen wrote:
> > On Mon, 2019-06-17 at 15:24 -0700, Sean Christopherson wrote:
> > >  {
> > > -	if (len < 2 * PAGE_SIZE || len & (len - 1) || flags & MAP_PRIVATE)
> > > +	if (flags & MAP_PRIVATE)
> > > +		return -EINVAL;
> > > +
> > > +	if (flags & MAP_FIXED)
> > > +		return addr;
> > > +
> > > +	if (len < 2 * PAGE_SIZE || len & (len - 1))
> > >  		return -EINVAL;
> > >
> > >  	addr = current->mm->get_unmapped_area(file, addr, 2 * len, pgoff,
> > 
> > Just sanity checking that for MAP_FIXED case the mm checks that the area is
> > unmapped before calling this?
> 
> No, straight MAP_FIXED unmaps any existing mappings.  The NOREPLACE variant
> fails with -EEXIST if there are existing mappings.

Ah so it was [1]!

> The MAP_FIXED behavior is actually useful, bordering on mandatory, for the
> new flow.  It allows the loader to keep its initial mmap(PROT_NONE) of
> ELRANGE while (re)mapping the individual enclave sections, e.g. to prevent
> a different aspect of the process from mapping the require ELRANGE.

Yeah, totally agree.

[1] http://man7.org/linux/man-pages/man2/mmap.2.html

/Jarkko

Patch
diff mbox series

diff --git a/arch/x86/kernel/cpu/sgx/driver/main.c b/arch/x86/kernel/cpu/sgx/driver/main.c
index 03eca4bccee1..d7de4d9aea87 100644
--- a/arch/x86/kernel/cpu/sgx/driver/main.c
+++ b/arch/x86/kernel/cpu/sgx/driver/main.c
@@ -105,7 +105,13 @@  static unsigned long sgx_get_unmapped_area(struct file *file,
 					   unsigned long pgoff,
 					   unsigned long flags)
 {
-	if (len < 2 * PAGE_SIZE || len & (len - 1) || flags & MAP_PRIVATE)
+	if (flags & MAP_PRIVATE)
+		return -EINVAL;
+
+	if (flags & MAP_FIXED)
+		return addr;
+
+	if (len < 2 * PAGE_SIZE || len & (len - 1))
 		return -EINVAL;
 
 	addr = current->mm->get_unmapped_area(file, addr, 2 * len, pgoff,