linux-kernel - Re: [RFC PATCH v2 1/2] mm/userfaultfd: fix memory corruption due to writeprotect

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: <20210105184134.GD149908@xz-x1>
Date:   Tue, 5 Jan 2021 13:41:34 -0500
From:   Peter Xu <peterx@...hat.com>
To:     Andrea Arcangeli <aarcange@...hat.com>
Cc:     Nadav Amit <nadav.amit@...il.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, Nadav Amit <namit@...are.com>,
        Yu Zhao <yuzhao@...gle.com>, Andy Lutomirski <luto@...nel.org>,
        Pavel Emelyanov <xemul@...nvz.org>,
        Mike Kravetz <mike.kravetz@...cle.com>,
        Mike Rapoport <rppt@...ux.vnet.ibm.com>,
        Minchan Kim <minchan@...nel.org>,
        Will Deacon <will@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [RFC PATCH v2 1/2] mm/userfaultfd: fix memory corruption due to
 writeprotect

On Tue, Jan 05, 2021 at 01:08:48PM -0500, Andrea Arcangeli wrote:
> On Tue, Jan 05, 2021 at 10:08:13AM -0500, Peter Xu wrote:
> > On Fri, Dec 25, 2020 at 01:25:28AM -0800, Nadav Amit wrote:
> > > diff --git a/mm/mprotect.c b/mm/mprotect.c
> > > index ab709023e9aa..c08c4055b051 100644
> > > --- a/mm/mprotect.c
> > > +++ b/mm/mprotect.c
> > > @@ -75,7 +75,8 @@ static unsigned long change_pte_range(struct vm_area_struct *vma, pmd_t *pmd,
> > >  		oldpte = *pte;
> > >  		if (pte_present(oldpte)) {
> > >  			pte_t ptent;
> > > -			bool preserve_write = prot_numa && pte_write(oldpte);
> > > +			bool preserve_write = (prot_numa || uffd_wp_resolve) &&
> > > +					      pte_write(oldpte);
> > 
> > Irrelevant of the other tlb issue, this is a standalone one and I commented in
> > v1 about simply ignore the change if necessary; unluckily that seems to be
> > ignored..  so I'll try again - would below be slightly better?
> > 
> >     if (uffd_wp_resolve && !pte_uffd_wp(oldpte))
> >         continue;
> 
> I posted the exact same code before seeing the above so I take it as a good
> sign :). I'd suggest to add the reverse check to the uffd_wp too.

Agreed. I didn't mention uffd_wp check (which I actually mentioned in the reply
to v1 patchset) here only because the uffd_wp check is pure optimization; while
the uffd_wp_resolve check is more critical because it is potentially a fix of
similar tlb flushing issue where we could have demoted the pte without being
noticed, so I think it's indeed more important as Nadav wanted to fix in the
same patch.

It would be even nicer if we have both covered (all of them can be in
unlikely() as Andrea suggested in the other email), then maybe nicer as a
standalone patch, then mention about the difference of the two in the commit
log (majorly, the resolving change will be more than optimization).

Thanks,

-- 
Peter Xu