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]

Date:   Tue, 5 Jan 2021 14:43:29 -0500
From:   Peter Xu <peterx@...hat.com>
To:     Nadav Amit <namit@...are.com>
Cc:     linux-mm <linux-mm@...ck.org>, lkml <linux-kernel@...r.kernel.org>,
        Andrea Arcangeli <aarcange@...hat.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 07:07:51PM +0000, Nadav Amit wrote:
> > On Jan 5, 2021, at 7:08 AM, Peter Xu <peterx@...hat.com> 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;
> > 
> > Firstly, current patch is confusing at least to me, because "uffd_wp_resolve"
> > means "unprotect the pte", whose write bit should mostly be cleared already
> > when uffd_wp_resolve is applicable.  Then "preserve_write" for that pte looks
> > odd already.
> > 
> > Meanwhile, if that really happens (when pte write bit set, but during a
> > uffd_wp_resolve request) imho there is really nothing we can do, so we should
> > simply avoid touching that at all, and also avoid ptep_modify_prot_start,
> > pte_modify, ptep_modify_prot_commit, calls etc., which takes extra cost.
> 
> Sorry for missing your feedback before. What you suggest makes perfect
> sense.

No problem.  I actually appreciated a lot for all your great works on these.
The strange thing is the userfaultfd kselftest seems to be working always fine
locally to me (probably another reason that I mostly test uffd-wp with
umapsort), so I won't be able to reproduce some issue you (and Andrea) have
encountered.  It's great you unveiled all these hard tlb problems and nailed
them down so lives should be easier for all of us.

Thanks,

-- 
Peter Xu