linux-kernel - Re: [PATCH v2] mm/mprotect: Fix soft-dirty check in can_change_pte

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] [day] [month] [year] [list]

Message-ID: <YtmFa+BdYkY3SSVP@xz-m1.local>
Date:   Thu, 21 Jul 2022 12:57:15 -0400
From:   Peter Xu <peterx@...hat.com>
To:     David Hildenbrand <david@...hat.com>
Cc:     linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        Andrea Arcangeli <aarcange@...hat.com>,
        Nadav Amit <nadav.amit@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH v2] mm/mprotect: Fix soft-dirty check in
 can_change_pte_writable()

On Thu, Jul 21, 2022 at 09:28:47AM +0200, David Hildenbrand wrote:
> Modifying your test to map page from a file MAP_SHARED gives me under
> 5.18.11-100.fc35.x86_64:
> 
> 
> 53,54d52
> <       FILE *file = fopen("tmpfile", "w+");
> <       int file_fd;
> 56d53
> <       assert(file);
> 59,61d55
> <
> <       file_fd = fileno(file);
> <       ftruncate(file_fd, psize);
> 63c57
> <                   MAP_SHARED, file_fd, 0);
> ---
> >                   MAP_ANONYMOUS|MAP_PRIVATE, -1, 0);
> 
> 
> t480s: ~  $ sudo ./tmp
> ERROR: Wrote page again, soft-dirty=0 (expect: 1
> 
> 
> 
> IMHO, while the check in vma_wants_writenotify() is correct and makes
> sure that the pages are kept R/O by the generic machinery. We get
> vma_wants_writenotify(), so we activate MM_CP_TRY_CHANGE_WRITABLE. The
> wrong logic in can_change_pte_writable(), however, maps the page
> writable again without caring about softdirty.
> 
> At least that would be my explanation for the failure. But maybe I
> messes up something else :)

Correct, I missed that part.  I verified that the same test also fails for
me easily on a xfs file test of an old kernel.

Let me touch up the commit message for that.   Though I think I'll still
keep the Fixes since the patch won't apply to before the commit, but I'll
mention that's only for tracking purpose.

[...]

> Can we turn that into a vm selftest in
> tools/testing/selftests/vm/soft-dirty.c, and also extend it by
> MAP_SHARED froma  file as above?

Sure.  I'll post a v3 with that.

[...]

> > @@ -48,8 +48,11 @@ static inline bool can_change_pte_writable(struct vm_area_struct *vma,
> >  	if (pte_protnone(pte) || !pte_dirty(pte))
> >  		return false;
> >  
> > -	/* Do we need write faults for softdirty tracking? */
> > -	if ((vma->vm_flags & VM_SOFTDIRTY) && !pte_soft_dirty(pte))
> > +	/*
> > +	 * Do we need write faults for softdirty tracking?  Note,
> > +	 * soft-dirty is enabled when !VM_SOFTDIRTY.
> > +	 */
> > +	if (!(vma->vm_flags & VM_SOFTDIRTY) && !pte_soft_dirty(pte))
> >  		return false;
> 
> I wonder if we now want, just as in vma_wants_writenotify(), an early
> check for IS_ENABLED(CONFIG_MEM_SOFT_DIRTY), to optimize this out
> completely.

Hmm, it may not even be an optimization issue, since when
!CONFIG_MEM_SOFT_DIRTY we have VM_SOFTDIRTY defined as 0x0.

It means !(vma->vm_flags & VM_SOFTDIRTY) will be constantly true even if
soft dirty not compiled in.

I'll add that check too.  Thanks,

-- 
Peter Xu