[v33,10/21] mm: Introduce vm_ops->may_mprotect()
diff mbox series

Message ID 20200617220844.57423-11-jarkko.sakkinen@linux.intel.com
State New, archived
Headers show
Series
  • Intel SGX foundations
Related show

Commit Message

Jarkko Sakkinen June 17, 2020, 10:08 p.m. UTC
From: Sean Christopherson <sean.j.christopherson@intel.com>

Add vm_ops()->may_mprotect() to check additional constraints.

SGX uses this callback to add two constraints:

1. Verify that the address range does not have holes: for each page
   address, there is an actual enclave page created.
2. Mapped permissions do not surpass the lowest enclave page permissions
   in the address range.

linux-mm@kvack.org
Andrew Morton <akpm@linux-foundation.org>
Acked-by: Jethro Beekman <jethro@fortanix.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 include/linux/mm.h |  2 ++
 mm/mprotect.c      | 14 +++++++++++---
 2 files changed, 13 insertions(+), 3 deletions(-)

Comments

Borislav Petkov June 25, 2020, 5:14 p.m. UTC | #1
On Thu, Jun 18, 2020 at 01:08:32AM +0300, Jarkko Sakkinen wrote:
> From: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> Add vm_ops()->may_mprotect() to check additional constraints.
> 
> SGX uses this callback to add two constraints:
> 
> 1. Verify that the address range does not have holes: for each page
>    address, there is an actual enclave page created.
> 2. Mapped permissions do not surpass the lowest enclave page permissions
>    in the address range.
> 
> linux-mm@kvack.org
> Andrew Morton <akpm@linux-foundation.org>

Something ate the Cc:s. Lemme add the mm list, akpm is already on Cc.

Leaving in the rest for mm folks.

> Acked-by: Jethro Beekman <jethro@fortanix.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  include/linux/mm.h |  2 ++
>  mm/mprotect.c      | 14 +++++++++++---
>  2 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index dc7b87310c10..be40b9c29327 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -542,6 +542,8 @@ struct vm_operations_struct {
>  	void (*close)(struct vm_area_struct * area);
>  	int (*split)(struct vm_area_struct * area, unsigned long addr);
>  	int (*mremap)(struct vm_area_struct * area);
> +	int (*may_mprotect)(struct vm_area_struct *vma, unsigned long start,
> +			    unsigned long end, unsigned long prot);
>  	vm_fault_t (*fault)(struct vm_fault *vmf);
>  	vm_fault_t (*huge_fault)(struct vm_fault *vmf,
>  			enum page_entry_size pe_size);
> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index ce8b8a5eacbb..f7731dc13ff0 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
> @@ -603,13 +603,21 @@ static int do_mprotect_pkey(unsigned long start, size_t len,
>  			goto out;
>  		}
>  
> +		tmp = vma->vm_end;
> +		if (tmp > end)
> +			tmp = end;
> +
> +		if (vma->vm_ops && vma->vm_ops->may_mprotect) {
> +			error = vma->vm_ops->may_mprotect(vma, nstart, tmp,
> +							  prot);
> +			if (error)
> +				goto out;
> +		}
> +
>  		error = security_file_mprotect(vma, reqprot, prot);
>  		if (error)
>  			goto out;
>  
> -		tmp = vma->vm_end;
> -		if (tmp > end)
> -			tmp = end;
>  		error = mprotect_fixup(vma, &prev, nstart, tmp, newflags);
>  		if (error)
>  			goto out;
> -- 
> 2.25.1
>
Matthew Wilcox June 25, 2020, 5:30 p.m. UTC | #2
On Thu, Jun 25, 2020 at 07:14:16PM +0200, Borislav Petkov wrote:
> On Thu, Jun 18, 2020 at 01:08:32AM +0300, Jarkko Sakkinen wrote:
> > From: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> Something ate the Cc:s. Lemme add the mm list, akpm is already on Cc.
> 
> Leaving in the rest for mm folks.

Thanks!

> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index dc7b87310c10..be40b9c29327 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -542,6 +542,8 @@ struct vm_operations_struct {
> >  	void (*close)(struct vm_area_struct * area);
> >  	int (*split)(struct vm_area_struct * area, unsigned long addr);
> >  	int (*mremap)(struct vm_area_struct * area);
> > +	int (*may_mprotect)(struct vm_area_struct *vma, unsigned long start,
> > +			    unsigned long end, unsigned long prot);

This is unlike any other vm operation.  Every other one is a verb.

> > diff --git a/mm/mprotect.c b/mm/mprotect.c
> > index ce8b8a5eacbb..f7731dc13ff0 100644
> > --- a/mm/mprotect.c
> > +++ b/mm/mprotect.c
> > @@ -603,13 +603,21 @@ static int do_mprotect_pkey(unsigned long start, size_t len,
> >  			goto out;
> >  		}
> >  
> > +		tmp = vma->vm_end;
> > +		if (tmp > end)
> > +			tmp = end;
> > +
> > +		if (vma->vm_ops && vma->vm_ops->may_mprotect) {
> > +			error = vma->vm_ops->may_mprotect(vma, nstart, tmp,
> > +							  prot);
> > +			if (error)
> > +				goto out;
> > +		}
> > +
> >  		error = security_file_mprotect(vma, reqprot, prot);
> >  		if (error)
> >  			goto out;
> >  

I think the right way to do this is:

                error = security_file_mprotect(vma, reqprot, prot);
                if (error)
                        goto out;

                tmp = vma->vm_end;
                if (tmp > end)
                        tmp = end;
+		if (vma->vm_ops->mprotect)
+			error = vma->vm_ops->mprotect(vma, &prev, nstart, tmp,
+					newflags);
+		else
+			error = mprotect_fixup(vma, &prev, nstart, tmp,
+					newflags);
-               error = mprotect_fixup(vma, &prev, nstart, tmp, newflags);
                if (error)
                        goto out;

and then the vma owner can do whatever it needs to before calling
mprotect_fixup(), which is already not static.

(how did we get to v33 with this kind of problem still in the patch set?)
Sean Christopherson June 25, 2020, 6:06 p.m. UTC | #3
On Thu, Jun 25, 2020 at 06:30:50PM +0100, Matthew Wilcox wrote:
> On Thu, Jun 25, 2020 at 07:14:16PM +0200, Borislav Petkov wrote:
> > On Thu, Jun 18, 2020 at 01:08:32AM +0300, Jarkko Sakkinen wrote:
> > > diff --git a/mm/mprotect.c b/mm/mprotect.c
> > > index ce8b8a5eacbb..f7731dc13ff0 100644
> > > --- a/mm/mprotect.c
> > > +++ b/mm/mprotect.c
> > > @@ -603,13 +603,21 @@ static int do_mprotect_pkey(unsigned long start, size_t len,
> > >  			goto out;
> > >  		}
> > >  
> > > +		tmp = vma->vm_end;
> > > +		if (tmp > end)
> > > +			tmp = end;
> > > +
> > > +		if (vma->vm_ops && vma->vm_ops->may_mprotect) {
> > > +			error = vma->vm_ops->may_mprotect(vma, nstart, tmp,
> > > +							  prot);
> > > +			if (error)
> > > +				goto out;
> > > +		}
> > > +
> > >  		error = security_file_mprotect(vma, reqprot, prot);
> > >  		if (error)
> > >  			goto out;
> > >  
> 
> I think the right way to do this is:
> 
>                 error = security_file_mprotect(vma, reqprot, prot);
>                 if (error)
>                         goto out;
> 
>                 tmp = vma->vm_end;
>                 if (tmp > end)
>                         tmp = end;
> +		if (vma->vm_ops->mprotect)
> +			error = vma->vm_ops->mprotect(vma, &prev, nstart, tmp,
> +					newflags);
> +		else
> +			error = mprotect_fixup(vma, &prev, nstart, tmp,
> +					newflags);
> -               error = mprotect_fixup(vma, &prev, nstart, tmp, newflags);
>                 if (error)
>                         goto out;
> 
> and then the vma owner can do whatever it needs to before calling
> mprotect_fixup(), which is already not static.

I'm certainly not opposed to a straight ->mprotect() hook.  ->may_protect()
came about because I/we thought it would be less objectionable to allow the
vma owner to apply additional restrictions as opposed to a wholesale
replacement.

> (how did we get to v33 with this kind of problem still in the patch set?)

Because no one from the mm world has looked at it.  Which is completely
understandable because it's a giant patch set and the first 25 or so versions
were spent sorting out fundamental architectural/design issue (there have
been a _lot_ of speed bumps), e.g. the need for hooking mprotect() didn't
even come about until v21.
Jarkko Sakkinen June 25, 2020, 10:26 p.m. UTC | #4
On Thu, Jun 25, 2020 at 07:14:16PM +0200, Borislav Petkov wrote:
> On Thu, Jun 18, 2020 at 01:08:32AM +0300, Jarkko Sakkinen wrote:
> > From: Sean Christopherson <sean.j.christopherson@intel.com>
> > 
> > Add vm_ops()->may_mprotect() to check additional constraints.
> > 
> > SGX uses this callback to add two constraints:
> > 
> > 1. Verify that the address range does not have holes: for each page
> >    address, there is an actual enclave page created.
> > 2. Mapped permissions do not surpass the lowest enclave page permissions
> >    in the address range.
> > 
> > linux-mm@kvack.org
> > Andrew Morton <akpm@linux-foundation.org>
> 
> Something ate the Cc:s. Lemme add the mm list, akpm is already on Cc.
> 
> Leaving in the rest for mm folks.

Thank you. So it seems. I've fixed them now.

/Jarkko
Jarkko Sakkinen June 25, 2020, 10:40 p.m. UTC | #5
On Thu, Jun 25, 2020 at 11:06:46AM -0700, Sean Christopherson wrote:
> On Thu, Jun 25, 2020 at 06:30:50PM +0100, Matthew Wilcox wrote:
> > On Thu, Jun 25, 2020 at 07:14:16PM +0200, Borislav Petkov wrote:
> > > On Thu, Jun 18, 2020 at 01:08:32AM +0300, Jarkko Sakkinen wrote:
> > > > diff --git a/mm/mprotect.c b/mm/mprotect.c
> > > > index ce8b8a5eacbb..f7731dc13ff0 100644
> > > > --- a/mm/mprotect.c
> > > > +++ b/mm/mprotect.c
> > > > @@ -603,13 +603,21 @@ static int do_mprotect_pkey(unsigned long start, size_t len,
> > > >  			goto out;
> > > >  		}
> > > >  
> > > > +		tmp = vma->vm_end;
> > > > +		if (tmp > end)
> > > > +			tmp = end;
> > > > +
> > > > +		if (vma->vm_ops && vma->vm_ops->may_mprotect) {
> > > > +			error = vma->vm_ops->may_mprotect(vma, nstart, tmp,
> > > > +							  prot);
> > > > +			if (error)
> > > > +				goto out;
> > > > +		}
> > > > +
> > > >  		error = security_file_mprotect(vma, reqprot, prot);
> > > >  		if (error)
> > > >  			goto out;
> > > >  
> > 
> > I think the right way to do this is:
> > 
> >                 error = security_file_mprotect(vma, reqprot, prot);
> >                 if (error)
> >                         goto out;
> > 
> >                 tmp = vma->vm_end;
> >                 if (tmp > end)
> >                         tmp = end;
> > +		if (vma->vm_ops->mprotect)
> > +			error = vma->vm_ops->mprotect(vma, &prev, nstart, tmp,
> > +					newflags);
> > +		else
> > +			error = mprotect_fixup(vma, &prev, nstart, tmp,
> > +					newflags);
> > -               error = mprotect_fixup(vma, &prev, nstart, tmp, newflags);
> >                 if (error)
> >                         goto out;
> > 
> > and then the vma owner can do whatever it needs to before calling
> > mprotect_fixup(), which is already not static.
> 
> I'm certainly not opposed to a straight ->mprotect() hook.  ->may_protect()
> came about because I/we thought it would be less objectionable to allow the
> vma owner to apply additional restrictions as opposed to a wholesale
> replacement.

Can you send fixes to associated patches to linux-sgx (i.e. this and
driver)?

> > (how did we get to v33 with this kind of problem still in the patch set?)
> 
> Because no one from the mm world has looked at it.  Which is completely
> understandable because it's a giant patch set and the first 25 or so versions
> were spent sorting out fundamental architectural/design issue (there have
> been a _lot_ of speed bumps), e.g. the need for hooking mprotect() didn't
> even come about until v21.

Actually also because we did not have an explicit linux-mm CC.

/Jarkko

Patch
diff mbox series

diff --git a/include/linux/mm.h b/include/linux/mm.h
index dc7b87310c10..be40b9c29327 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -542,6 +542,8 @@  struct vm_operations_struct {
 	void (*close)(struct vm_area_struct * area);
 	int (*split)(struct vm_area_struct * area, unsigned long addr);
 	int (*mremap)(struct vm_area_struct * area);
+	int (*may_mprotect)(struct vm_area_struct *vma, unsigned long start,
+			    unsigned long end, unsigned long prot);
 	vm_fault_t (*fault)(struct vm_fault *vmf);
 	vm_fault_t (*huge_fault)(struct vm_fault *vmf,
 			enum page_entry_size pe_size);
diff --git a/mm/mprotect.c b/mm/mprotect.c
index ce8b8a5eacbb..f7731dc13ff0 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -603,13 +603,21 @@  static int do_mprotect_pkey(unsigned long start, size_t len,
 			goto out;
 		}
 
+		tmp = vma->vm_end;
+		if (tmp > end)
+			tmp = end;
+
+		if (vma->vm_ops && vma->vm_ops->may_mprotect) {
+			error = vma->vm_ops->may_mprotect(vma, nstart, tmp,
+							  prot);
+			if (error)
+				goto out;
+		}
+
 		error = security_file_mprotect(vma, reqprot, prot);
 		if (error)
 			goto out;
 
-		tmp = vma->vm_end;
-		if (tmp > end)
-			tmp = end;
 		error = mprotect_fixup(vma, &prev, nstart, tmp, newflags);
 		if (error)
 			goto out;