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 for Android: free password hash cracker in your pocket

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

Date:   Tue, 5 Jan 2021 09:22:49 +0000
From:   Nadav Amit <namit@...are.com>
To:     Peter Zijlstra <peterz@...radead.org>
CC:     Andrea Arcangeli <aarcange@...hat.com>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        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 Jan 5, 2021, at 12:58 AM, Peter Zijlstra <peterz@...radead.org> wrote:
> 
> On Mon, Jan 04, 2021 at 02:24:38PM -0500, Andrea Arcangeli wrote:
>> On Mon, Jan 04, 2021 at 01:22:27PM +0100, Peter Zijlstra wrote:
>>> 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!
>> 
>> Note, the above was posted before we figured out the details so it
>> wasn't showing the real deferred tlb flush that caused problems (the
>> one showed on the left causes zero issues).
>> 
>> The problematic one not pictured is the one of the wrprotect that has
>> to be running in another CPU which is also isn't picture above. More
>> accurate traces are posted later in the thread.
> 
> Lets assume CPU0 does a read-lock, W -> RO with deferred flush.

This is the second scenario that is mentioned in the patch. (The first one
is relatively easy to address by not clearing the write-bit).

>>> 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
>> 
>> How do intend you wait in PT lock while the writer also has to take PT
>> lock repeatedly before it can do wake_up_var?
>> 
>> If you release the PT lock before calling wait_tlb_flush_pending it
>> all falls apart again.
> 
> I suppose you can check for pending, if found, release lock, wait for 0,
> and re-take the fault?

My personal take on this issue (which for full disclosure I think Andrea
disagrees with) is that it the most important enhancement is to reduce the
number of cases which we mistakenly think that we must wait for pending TLB
flush. It will not be free though.

As to the enhancement that you propose: although it seems as a valid
enhancement to me, I think that it is more robust to make forward progress
when possible (as done today). This is especially important if the proposed
enhancement cannot be checked by lockdep.