linux-kernel - Re: [PATCH v33 10/21] mm: Introduce vm_ops->may

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200625180646.GF3437@linux.intel.com>
Date:   Thu, 25 Jun 2020 11:06:46 -0700
From:   Sean Christopherson <sean.j.christopherson@...el.com>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     Borislav Petkov <bp@...en8.de>,
        Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
        x86@...nel.org, linux-sgx@...r.kernel.org,
        linux-kernel@...r.kernel.org, Jethro Beekman <jethro@...tanix.com>,
        akpm@...ux-foundation.org, andriy.shevchenko@...ux.intel.com,
        asapek@...gle.com, cedric.xing@...el.com, chenalexchen@...gle.com,
        conradparker@...gle.com, cyhanish@...gle.com,
        dave.hansen@...el.com, haitao.huang@...el.com,
        josh@...htriplett.org, kai.huang@...el.com, kai.svahn@...el.com,
        kmoy@...gle.com, ludloff@...gle.com, luto@...nel.org,
        nhorman@...hat.com, npmccallum@...hat.com, puiterwijk@...hat.com,
        rientjes@...gle.com, tglx@...utronix.de, yaozhangx@...gle.com,
        linux-mm@...ck.org
Subject: Re: [PATCH v33 10/21] mm: Introduce vm_ops->may_mprotect()

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.