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: <20210104122227.GL3021@hirez.programming.kicks-ass.net>
Date:   Mon, 4 Jan 2021 13:22:27 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Nadav Amit <nadav.amit@...il.com>
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Nadav Amit <namit@...are.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Yu Zhao <yuzhao@...gle.com>, Andy Lutomirski <luto@...nel.org>,
        Peter Xu <peterx@...hat.com>,
        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>, Mel Gorman <mgorman@...e.de>
Subject: Re: [RFC PATCH v2 1/2] mm/userfaultfd: fix memory corruption due to
 writeprotect

On Fri, Dec 25, 2020 at 01:25:28AM -0800, Nadav Amit wrote:

> The scenario that happens in selftests/vm/userfaultfd is as follows:
> 
> cpu0				cpu1			cpu2
> ----				----			----
> 							[ Writable PTE
> 							  cached in TLB ]
> userfaultfd_writeprotect()
> [ write-*unprotect* ]
> mwriteprotect_range()
> mmap_read_lock()
> change_protection()
> 
> change_protection_range()
> ...
> change_pte_range()
> [ *clear* “write”-bit ]
> [ defer TLB flushes ]
> 				[ page-fault ]
> 				...
> 				wp_page_copy()
> 				 cow_user_page()
> 				  [ copy page ]
> 							[ write to old
> 							  page ]
> 				...
> 				 set_pte_at_notify()

Yuck!

Isn't this all rather similar to the problem that resulted in the
tlb_flush_pending mess?

I still think that's all fundamentally buggered, the much saner solution
(IMO) would've been to make things wait for the pending flush, instead
of doing a local flush and fudging things like we do now.

Then the above could be fixed by having wp_page_copy() wait for the
pending invalidate (although a more fine-grained pending state would be
awesome).

The below probably doesn't compile and will probably cause massive
header fail at the very least, but does show the general.


diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 07d9acb5b19c..0210547ac424 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -649,7 +649,8 @@ static inline void dec_tlb_flush_pending(struct mm_struct *mm)
 	 *
 	 * Therefore we must rely on tlb_flush_*() to guarantee order.
 	 */
-	atomic_dec(&mm->tlb_flush_pending);
+	if (atomic_dec_and_test(&mm->tlb_flush_pending))
+		wake_up_var(&mm->tlb_flush_pending);
 }
 
 static inline bool mm_tlb_flush_pending(struct mm_struct *mm)
@@ -677,6 +678,12 @@ static inline bool mm_tlb_flush_nested(struct mm_struct *mm)
 	return atomic_read(&mm->tlb_flush_pending) > 1;
 }
 
+static inline void wait_tlb_flush_pending(struct mm_struct *mm)
+{
+	wait_var_event(&mm->tlb_flush_pending,
+		       atomic_read(&mm->tlb_flush_pending) == 0);
+}
+
 struct vm_fault;
 
 /**
diff --git a/mm/memory.c b/mm/memory.c
index feff48e1465a..3c36bca2972a 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3087,6 +3087,8 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf)
 {
 	struct vm_area_struct *vma = vmf->vma;
 
+	wait_tlb_flush_pending(vma->vm_mm);
+
 	if (userfaultfd_pte_wp(vma, *vmf->pte)) {
 		pte_unmap_unlock(vmf->pte, vmf->ptl);
 		return handle_userfault(vmf, VM_UFFD_WP);