| 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
| ||
|
Message-ID: <ZBzOqwF2wrHgBVZb@x1n>
Date: Thu, 23 Mar 2023 18:11:55 -0400
From: Peter Xu <peterx@...hat.com>
To: Muhammad Usama Anjum <usama.anjum@...labora.com>
Cc: linux-mm@...ck.org, linux-kernel@...r.kernel.org,
Andrea Arcangeli <aarcange@...hat.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Axel Rasmussen <axelrasmussen@...gle.com>,
Mike Rapoport <rppt@...ux.vnet.ibm.com>,
Nadav Amit <nadav.amit@...il.com>,
linux-stable <stable@...r.kernel.org>,
David Hildenbrand <david@...hat.com>
Subject: Re: [PATCH] mm/hugetlb: Fix uffd wr-protection for CoW optimization
path
On Thu, Mar 23, 2023 at 08:33:07PM +0500, Muhammad Usama Anjum wrote:
> Hi Peter,
>
> Sorry for late reply.
>
> On 3/22/23 12:50 AM, Peter Xu wrote:
> > On Tue, Mar 21, 2023 at 08:36:35PM +0100, David Hildenbrand wrote:
> >> On 21.03.23 20:18, Peter Xu wrote:
> >>> This patch fixes an issue that a hugetlb uffd-wr-protected mapping can be
> >>> writable even with uffd-wp bit set. It only happens with all these
> >>> conditions met: (1) hugetlb memory (2) private mapping (3) original mapping
> >>> was missing, then (4) being wr-protected (IOW, pte marker installed). Then
> >>> write to the page to trigger.
> >>>
> >>> Userfaultfd-wp trap for hugetlb was implemented in hugetlb_fault() before
> >>> even reaching hugetlb_wp() to avoid taking more locks that userfault won't
> >>> need. However there's one CoW optimization path for missing hugetlb page
> >>> that can trigger hugetlb_wp() inside hugetlb_no_page(), that can bypass the
> >>> userfaultfd-wp traps.
> >>>
> >>> A few ways to resolve this:
> >>>
> >>> (1) Skip the CoW optimization for hugetlb private mapping, considering
> >>> that private mappings for hugetlb should be very rare, so it may not
> >>> really be helpful to major workloads. The worst case is we only skip the
> >>> optimization if userfaultfd_wp(vma)==true, because uffd-wp needs another
> >>> fault anyway.
> >>>
> >>> (2) Move the userfaultfd-wp handling for hugetlb from hugetlb_fault()
> >>> into hugetlb_wp(). The major cons is there're a bunch of locks taken
> >>> when calling hugetlb_wp(), and that will make the changeset unnecessarily
> >>> complicated due to the lock operations.
> >>>
> >>> (3) Carry over uffd-wp bit in hugetlb_wp(), so it'll need to fault again
> >>> for uffd-wp privately mapped pages.
> >>>
> >>> This patch chose option (3) which contains the minimum changeset (simplest
> >>> for backport) and also make sure hugetlb_wp() itself will start to be
> >>> always safe with uffd-wp ptes even if called elsewhere in the future.
> >>>
> >>> This patch will be needed for v5.19+ hence copy stable.
> >>>
> >>> Reported-by: Muhammad Usama Anjum <usama.anjum@...labora.com>
> >>> Cc: linux-stable <stable@...r.kernel.org>
> >>> Fixes: 166f3ecc0daf ("mm/hugetlb: hook page faults for uffd write protection")
> >>> Signed-off-by: Peter Xu <peterx@...hat.com>
> >>> ---
> >>> mm/hugetlb.c | 8 +++++---
> >>> 1 file changed, 5 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> >>> index 8bfd07f4c143..22337b191eae 100644
> >>> --- a/mm/hugetlb.c
> >>> +++ b/mm/hugetlb.c
> >>> @@ -5478,7 +5478,7 @@ static vm_fault_t hugetlb_wp(struct mm_struct *mm, struct vm_area_struct *vma,
> >>> struct folio *pagecache_folio, spinlock_t *ptl)
> >>> {
> >>> const bool unshare = flags & FAULT_FLAG_UNSHARE;
> >>> - pte_t pte;
> >>> + pte_t pte, newpte;
> >>> struct hstate *h = hstate_vma(vma);
> >>> struct page *old_page;
> >>> struct folio *new_folio;
> >>> @@ -5622,8 +5622,10 @@ static vm_fault_t hugetlb_wp(struct mm_struct *mm, struct vm_area_struct *vma,
> >>> mmu_notifier_invalidate_range(mm, range.start, range.end);
> >>> page_remove_rmap(old_page, vma, true);
> >>> hugepage_add_new_anon_rmap(new_folio, vma, haddr);
> >>> - set_huge_pte_at(mm, haddr, ptep,
> >>> - make_huge_pte(vma, &new_folio->page, !unshare));
> >>> + newpte = make_huge_pte(vma, &new_folio->page, !unshare);
> >>> + if (huge_pte_uffd_wp(pte))
> >>> + newpte = huge_pte_mkuffd_wp(newpte);
> >>> + set_huge_pte_at(mm, haddr, ptep, newpte);
> >>> folio_set_hugetlb_migratable(new_folio);
> >>> /* Make the old page be freed below */
> >>> new_folio = page_folio(old_page);
> >>
> >> Looks correct to me. Do we have a reproducer?
> >
> > I used a reproducer for the async mode I wrote (patch 2 attached, need to
> > change to VM_PRIVATE):
> >
> > https://lore.kernel.org/all/ZBNr4nohj%2FTw4Zhw@x1n/
> >
> > I don't think kernel kselftest can trigger it because we don't do strict
> > checks yet with uffd-wp bits. I've already started looking into cleanup
> > the test cases and I do plan to add new tests to cover this.
> >
> > Meanwhile, let's also wait for an ack from Muhammad. Even though the async
> > mode is not part of the code base, it'll be a good test for verifying every
> > single uffd-wp bit being set or cleared as expected.
> I've tested by applying this patch. But the bug is still there. Just like
> Peter has mentioned, we are using our in progress patches related to
> pagemap_scan ioctl and userfaultd wp async patches to reproduce it.
>
> To reproduce please build kernel and run pagemap_ioctl test in mm in
> hugetlb_mem_reproducer branch:
> https://gitlab.collabora.com/usama.anjum/linux-mainline/-/tree/hugetlb_mem_reproducer
>
> In case you have any question on how to reproduce, please let me know. I'll
> try to provide a cleaner alternative.
Hmm, I think my current fix is incomplete if not wrong. The root cause
should still be valid, however I overlooked another path:
if (page_mapcount(old_page) == 1 && PageAnon(old_page)) {
if (!PageAnonExclusive(old_page))
page_move_anon_rmap(old_page, vma);
if (likely(!unshare))
set_huge_ptep_writable(vma, haddr, ptep);
delayacct_wpcopy_end();
return 0;
}
We should bail out early in this path, and it'll be even easier we always
bail out hugetlb_wp() as long as uffd-wp is detected because userfault
should always be handled before any decision to CoW.
v2 attached.. Please give it another shot.
Thanks,
--
Peter Xu
View attachment "0001-mm-hugetlb-Fix-uffd-wr-protection-for-CoW-optimizati.patch" of type "text/plain" (3586 bytes)
Powered by blists - more mailing lists