linux-kernel - Re: [PATCH v3 1/2] mm/migrate: Fix read-only page got writable when recover 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] [thread-next>] [day] [month] [year] [list]

Message-Id: <20221201143058.80296541cc6802d1e5990033@linux-foundation.org>
Date:   Thu, 1 Dec 2022 14:30:58 -0800
From:   Andrew Morton <akpm@...ux-foundation.org>
To:     David Hildenbrand <david@...hat.com>
Cc:     Peter Xu <peterx@...hat.com>, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, Mike Rapoport <rppt@...ux.vnet.ibm.com>,
        Nadav Amit <nadav.amit@...il.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Ives van Hoorne <ives@...esandbox.io>,
        Axel Rasmussen <axelrasmussen@...gle.com>,
        Alistair Popple <apopple@...dia.com>, stable@...r.kernel.org
Subject: Re: [PATCH v3 1/2] mm/migrate: Fix read-only page got writable when
 recover pte

On Thu, 1 Dec 2022 16:42:52 +0100 David Hildenbrand <david@...hat.com> wrote:

> On 01.12.22 16:28, Peter Xu wrote:
> > 
> > I didn't reply here because I have already replied with the question in
> > previous version with a few attempts.  Quotting myself:
> > 
> > https://lore.kernel.org/all/Y3KgYeMTdTM0FN5W@x1n/
> > 
> >          The thing is recovering the pte into its original form is the
> >          safest approach to me, so I think we need justification on why it's
> >          always safe to set the write bit.
> > 
> > I've also got another longer email trying to explain why I think it's the
> > other way round to be justfied, rather than justifying removal of the write
> > bit for a read migration entry, here:
> > 
> 
> And I disagree for this patch that is supposed to fix this hunk:
> 
> 
> @@ -243,11 +243,15 @@ static bool remove_migration_pte(struct page *page, struct vm_area_struct *vma,
>                  entry = pte_to_swp_entry(*pvmw.pte);
>                  if (is_write_migration_entry(entry))
>                          pte = maybe_mkwrite(pte, vma);
> +               else if (pte_swp_uffd_wp(*pvmw.pte))
> +                       pte = pte_mkuffd_wp(pte);
>   
>                  if (unlikely(is_zone_device_page(new))) {
>                          if (is_device_private_page(new)) {
>                                  entry = make_device_private_entry(new, pte_write(pte));
>                                  pte = swp_entry_to_pte(entry);
> +                               if (pte_swp_uffd_wp(*pvmw.pte))
> +                                       pte = pte_mkuffd_wp(pte);
>                          }
>                  }

David, I'm unclear on what you mean by the above.  Can you please
expand?

> 
> There is really nothing to justify the other way around here.
> If it's broken fix it independently and properly backport it independenty.
> 
> But we don't know about any such broken case.
> 
> I have no energy to spare to argue further ;)

This is a silent data loss bug, which is about as bad as it gets. 
Under obscure conditions, fortunately.  But please let's keep working
it.  Let's aim for something minimal for backporting purposes.  We can
revisit any cleanliness issues later.

David, do you feel that the proposed fix will at least address the bug
without adverse side-effects?