userfaultfd: preserve write protection across UFFDIO_MOVE

[PATCH] userfaultfd: preserve write protection across UFFDIO_MOVE

Posted by Gregory Price 2 months ago

move_present_ptes() unconditionally makes the destination PTE writable,
dropping uffd-wp write-protection from the source PTE.

The original intent was to follow mremap() behavior, but mremap()'s
move_ptes() preserves the source write state unconditionally.

Modify uffd to preserve the source write state and check the uffd-wp
condition of the source before setting writable on the destination.

Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI")
Cc: stable@vger.kernel.org
Signed-off-by: Gregory Price <gourry@gourry.net>
---
 mm/userfaultfd.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index e6dfd5f28acd..783ca68aed88 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -1123,7 +1123,10 @@ static long move_present_ptes(struct mm_struct *mm,
 			orig_dst_pte = pte_mksoft_dirty(orig_dst_pte);
 		if (pte_dirty(orig_src_pte))
 			orig_dst_pte = pte_mkdirty(orig_dst_pte);
-		orig_dst_pte = pte_mkwrite(orig_dst_pte, dst_vma);
+		if (pte_write(orig_src_pte))
+			orig_dst_pte = pte_mkwrite(orig_dst_pte, dst_vma);
+		if (pte_uffd_wp(orig_src_pte))
+			orig_dst_pte = pte_mkuffd_wp(orig_dst_pte);
 		set_pte_at(mm, dst_addr, dst_pte, orig_dst_pte);
 
 		src_addr += PAGE_SIZE;
-- 
2.52.0

Re: [PATCH] userfaultfd: preserve write protection across UFFDIO_MOVE

Posted by Andrew Morton 2 months ago

On Thu,  9 Apr 2026 11:28:22 -0400 Gregory Price <gourry@gourry.net> wrote:

> move_present_ptes() unconditionally makes the destination PTE writable,
> dropping uffd-wp write-protection from the source PTE.
> 
> The original intent was to follow mremap() behavior, but mremap()'s
> move_ptes() preserves the source write state unconditionally.
> 
> Modify uffd to preserve the source write state and check the uffd-wp
> condition of the source before setting writable on the destination.

Please can we have a description of the userspace-visible impact of the
bug.

> Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI")
> Cc: stable@vger.kernel.org

especially when cc:stable, thanks.

> Signed-off-by: Gregory Price <gourry@gourry.net>
>
> ...
>
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c
> @@ -1123,7 +1123,10 @@ static long move_present_ptes(struct mm_struct *mm,
>  			orig_dst_pte = pte_mksoft_dirty(orig_dst_pte);
>  		if (pte_dirty(orig_src_pte))
>  			orig_dst_pte = pte_mkdirty(orig_dst_pte);
> -		orig_dst_pte = pte_mkwrite(orig_dst_pte, dst_vma);
> +		if (pte_write(orig_src_pte))
> +			orig_dst_pte = pte_mkwrite(orig_dst_pte, dst_vma);
> +		if (pte_uffd_wp(orig_src_pte))
> +			orig_dst_pte = pte_mkuffd_wp(orig_dst_pte);
>  		set_pte_at(mm, dst_addr, dst_pte, orig_dst_pte);
>  

(presently wondering if this is backward compatible)

Re: [PATCH] userfaultfd: preserve write protection across UFFDIO_MOVE

Posted by Gregory Price 2 months ago

On Sun, Apr 12, 2026 at 11:18:07AM -0700, Andrew Morton wrote:
> On Thu,  9 Apr 2026 11:28:22 -0400 Gregory Price <gourry@gourry.net> wrote:
> 
> > move_present_ptes() unconditionally makes the destination PTE writable,
> > dropping uffd-wp write-protection from the source PTE.
> > 
> > The original intent was to follow mremap() behavior, but mremap()'s
> > move_ptes() preserves the source write state unconditionally.
> > 
> > Modify uffd to preserve the source write state and check the uffd-wp
> > condition of the source before setting writable on the destination.
> 
> Please can we have a description of the userspace-visible impact of the
> bug.
>

Simply:

  UFFDIO_MOVE silently drops write protection from the source PTE when
  moving pages to a destination, leading to missing write-protect faults
  after the page has been moved.

I ran into this while futzing around with some user space management of
VM memory, and expecting a move to continue firing WP faults after.

But Sashiko actually made a useful (though obtuse) observation which
has made me realize _MOVE is actually ambiguous on what to do with
source region UFFD modes.

> > +		if (pte_uffd_wp(orig_src_pte))
> > +			orig_dst_pte = pte_mkuffd_wp(orig_dst_pte);

This line assumes the destination must have intended to be WP, and the
the result is essentially stale uffd wp bits in the opposite case (a
user not intending to carry over WP now carries it over).

tl;dr: this is more of a semantic change than I'd intended, and the
existing tests did not catch it.

The correct solution here is to make a UFFDIO_MOVE_MODE_WP flag to copy
the UFFDIO_COPY_MODE_WP pattern.  Otherwise:

> (presently wondering if this is backward compatible)

Yes, you're right to wonder - this does break backward compatibilty.

Will come back around with UFFDIO_MOVE_MODE_WP.

~Gregory

Re: [PATCH] userfaultfd: preserve write protection across UFFDIO_MOVE

Posted by Mike Rapoport 2 months ago

On Thu, Apr 09, 2026 at 11:28:22AM -0400, Gregory Price wrote:
> move_present_ptes() unconditionally makes the destination PTE writable,
> dropping uffd-wp write-protection from the source PTE.
> 
> The original intent was to follow mremap() behavior, but mremap()'s
> move_ptes() preserves the source write state unconditionally.
> 
> Modify uffd to preserve the source write state and check the uffd-wp
> condition of the source before setting writable on the destination.
> 
> Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI")
> Cc: stable@vger.kernel.org
> Signed-off-by: Gregory Price <gourry@gourry.net>

Reviewed-by: Mike Rapoport (Microsoft) <rppt@kernel.org>

> ---
>  mm/userfaultfd.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> index e6dfd5f28acd..783ca68aed88 100644
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c
> @@ -1123,7 +1123,10 @@ static long move_present_ptes(struct mm_struct *mm,
>  			orig_dst_pte = pte_mksoft_dirty(orig_dst_pte);
>  		if (pte_dirty(orig_src_pte))
>  			orig_dst_pte = pte_mkdirty(orig_dst_pte);
> -		orig_dst_pte = pte_mkwrite(orig_dst_pte, dst_vma);
> +		if (pte_write(orig_src_pte))
> +			orig_dst_pte = pte_mkwrite(orig_dst_pte, dst_vma);
> +		if (pte_uffd_wp(orig_src_pte))
> +			orig_dst_pte = pte_mkuffd_wp(orig_dst_pte);
>  		set_pte_at(mm, dst_addr, dst_pte, orig_dst_pte);
>  
>  		src_addr += PAGE_SIZE;
> -- 
> 2.52.0
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH] userfaultfd: preserve write protection across UFFDIO_MOVE

Posted by Suren Baghdasaryan 2 months ago

On Thu, Apr 9, 2026 at 8:28 AM Gregory Price <gourry@gourry.net> wrote:
>
> move_present_ptes() unconditionally makes the destination PTE writable,
> dropping uffd-wp write-protection from the source PTE.
>
> The original intent was to follow mremap() behavior, but mremap()'s
> move_ptes() preserves the source write state unconditionally.
>
> Modify uffd to preserve the source write state and check the uffd-wp
> condition of the source before setting writable on the destination.
>
> Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI")
> Cc: stable@vger.kernel.org
> Signed-off-by: Gregory Price <gourry@gourry.net>

Reviewed-by: Suren Baghdasaryan <surenb@google.com>

> ---
>  mm/userfaultfd.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> index e6dfd5f28acd..783ca68aed88 100644
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c
> @@ -1123,7 +1123,10 @@ static long move_present_ptes(struct mm_struct *mm,
>                         orig_dst_pte = pte_mksoft_dirty(orig_dst_pte);
>                 if (pte_dirty(orig_src_pte))
>                         orig_dst_pte = pte_mkdirty(orig_dst_pte);
> -               orig_dst_pte = pte_mkwrite(orig_dst_pte, dst_vma);
> +               if (pte_write(orig_src_pte))
> +                       orig_dst_pte = pte_mkwrite(orig_dst_pte, dst_vma);
> +               if (pte_uffd_wp(orig_src_pte))
> +                       orig_dst_pte = pte_mkuffd_wp(orig_dst_pte);
>                 set_pte_at(mm, dst_addr, dst_pte, orig_dst_pte);
>
>                 src_addr += PAGE_SIZE;
> --
> 2.52.0
>