All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mm/userfaultfd: Reset ptes when close() for wr-protected ones
@ 2024-04-22 13:33 Peter Xu
  2024-04-22 15:09 ` David Hildenbrand
  2024-04-22 19:47 ` Andrew Morton
  0 siblings, 2 replies; 4+ messages in thread
From: Peter Xu @ 2024-04-22 13:33 UTC (permalink / raw)
  To: linux-kernel, linux-mm
  Cc: peterx, Nadav Amit, Andrew Morton, David Hildenbrand,
	syzbot+d8426b591c36b21c750e

Userfaultfd unregister includes a step to remove wr-protect bits from all
the relevant pgtable entries, but that only covered an explicit
UFFDIO_UNREGISTER ioctl, not a close() on the userfaultfd itself.  Cover
that too.

Link: https://lore.kernel.org/all/000000000000ca4df20616a0fe16@google.com/
Analyzed-by: David Hildenbrand <david@redhat.com>
Reported-by: syzbot+d8426b591c36b21c750e@syzkaller.appspotmail.com
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 fs/userfaultfd.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index 3e6ddda6f159..d2c3879745e5 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -898,6 +898,10 @@ static int userfaultfd_release(struct inode *inode, struct file *file)
 			prev = vma;
 			continue;
 		}
+		/* Reset ptes for the whole vma range if wr-protected */
+		if (userfaultfd_wp(vma))
+			uffd_wp_range(vma, vma->vm_start,
+				      vma->vm_end - vma->vm_start, false);
 		new_flags = vma->vm_flags & ~__VM_UFFD_FLAGS;
 		vma = vma_modify_flags_uffd(&vmi, prev, vma, vma->vm_start,
 					    vma->vm_end, new_flags,
-- 
2.44.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] mm/userfaultfd: Reset ptes when close() for wr-protected ones
  2024-04-22 13:33 [PATCH] mm/userfaultfd: Reset ptes when close() for wr-protected ones Peter Xu
@ 2024-04-22 15:09 ` David Hildenbrand
  2024-04-22 19:47 ` Andrew Morton
  1 sibling, 0 replies; 4+ messages in thread
From: David Hildenbrand @ 2024-04-22 15:09 UTC (permalink / raw)
  To: Peter Xu, linux-kernel, linux-mm
  Cc: Nadav Amit, Andrew Morton, syzbot+d8426b591c36b21c750e

On 22.04.24 15:33, Peter Xu wrote:
> Userfaultfd unregister includes a step to remove wr-protect bits from all
> the relevant pgtable entries, but that only covered an explicit
> UFFDIO_UNREGISTER ioctl, not a close() on the userfaultfd itself.  Cover
> that too.
> 
> Link: https://lore.kernel.org/all/000000000000ca4df20616a0fe16@google.com/
> Analyzed-by: David Hildenbrand <david@redhat.com>
> Reported-by: syzbot+d8426b591c36b21c750e@syzkaller.appspotmail.com
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   fs/userfaultfd.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index 3e6ddda6f159..d2c3879745e5 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -898,6 +898,10 @@ static int userfaultfd_release(struct inode *inode, struct file *file)
>   			prev = vma;
>   			continue;
>   		}
> +		/* Reset ptes for the whole vma range if wr-protected */
> +		if (userfaultfd_wp(vma))
> +			uffd_wp_range(vma, vma->vm_start,
> +				      vma->vm_end - vma->vm_start, false);
>   		new_flags = vma->vm_flags & ~__VM_UFFD_FLAGS;
>   		vma = vma_modify_flags_uffd(&vmi, prev, vma, vma->vm_start,
>   					    vma->vm_end, new_flags,

LGTM

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] mm/userfaultfd: Reset ptes when close() for wr-protected ones
  2024-04-22 13:33 [PATCH] mm/userfaultfd: Reset ptes when close() for wr-protected ones Peter Xu
  2024-04-22 15:09 ` David Hildenbrand
@ 2024-04-22 19:47 ` Andrew Morton
  2024-04-22 21:08   ` Peter Xu
  1 sibling, 1 reply; 4+ messages in thread
From: Andrew Morton @ 2024-04-22 19:47 UTC (permalink / raw)
  To: Peter Xu
  Cc: linux-kernel, linux-mm, Nadav Amit, David Hildenbrand,
	syzbot+d8426b591c36b21c750e

On Mon, 22 Apr 2024 09:33:11 -0400 Peter Xu <peterx@redhat.com> wrote:

> Userfaultfd unregister includes a step to remove wr-protect bits from all
> the relevant pgtable entries, but that only covered an explicit
> UFFDIO_UNREGISTER ioctl, not a close() on the userfaultfd itself.  Cover
> that too.

We should include a description of the userspace-visible effects of the
bug, please.  Always.

I see it triggers a WARN, but so what - why ca't we simply delete the
WARN statement if that's the only effect?  Presumably there are other
consequences - what are they?

Also, a WARN-triggering bug should be fixed in -stable kernels so we'll
need a FIXES:, please?


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] mm/userfaultfd: Reset ptes when close() for wr-protected ones
  2024-04-22 19:47 ` Andrew Morton
@ 2024-04-22 21:08   ` Peter Xu
  0 siblings, 0 replies; 4+ messages in thread
From: Peter Xu @ 2024-04-22 21:08 UTC (permalink / raw)
  To: Andrew Morton
  Cc: linux-kernel, linux-mm, Nadav Amit, David Hildenbrand,
	syzbot+d8426b591c36b21c750e

On Mon, Apr 22, 2024 at 12:47:19PM -0700, Andrew Morton wrote:
> On Mon, 22 Apr 2024 09:33:11 -0400 Peter Xu <peterx@redhat.com> wrote:
> 
> > Userfaultfd unregister includes a step to remove wr-protect bits from all
> > the relevant pgtable entries, but that only covered an explicit
> > UFFDIO_UNREGISTER ioctl, not a close() on the userfaultfd itself.  Cover
> > that too.
> 
> We should include a description of the userspace-visible effects of the
> bug, please.  Always.

Ah, this one is a bit special so I didn't consider copying stable at all,
but I'll be more verbose next time..

The only user visible side effect is the user can observe leftover
wr-protect bits even if the user close()ed on an userfaultfd when releasing
the last reference of it.  However hopefully that should be harmless, and
nothing bad should happen even if so.

This change is now more important after the recent page-table-check patch
we merged in mm-unstable (446dd9ad37d0 ("mm/page_table_check: support
userfault wr-protect entries")), as we'll do sanity check on uffd-wp bits
without vma context.  So it's better if we can 100% guarantee no uffd-wp
bit leftovers, to make sure each report will be valid.

> 
> I see it triggers a WARN, but so what - why ca't we simply delete the
> WARN statement if that's the only effect?  Presumably there are other
> consequences - what are they?

Because that's newly added and we want to keep using those WARNINGs to trap
real bugs (and I'd expect new reports coming after this one.. we at least
have one real bug to fix somewhere..).

> 
> Also, a WARN-triggering bug should be fixed in -stable kernels so we'll
> need a FIXES:, please?

This only triggers due to the most recently added WARNING, so I assume it
shouldn't trigger in any old kernels, even Linus's tree shouldn't trigger
because the WARNING isn't there.

Though maybe it's indeed better to also pick this one up for stable, as it
does similar thing as what below commit does, however just to cover close()
too which was overlooked:

commit f369b07c861435bd812a9d14493f71b34132ed6f
Author: Peter Xu <peterx@redhat.com>
Date:   Thu Aug 11 16:13:40 2022 -0400

    mm/uffd: reset write protection when unregister with wp-mode

So I think that Fixes should be:

Fixes: f369b07c8614 ("mm/uffd: reset write protection when unregister with wp-mode")

Thanks,

-- 
Peter Xu


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2024-04-22 21:08 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-22 13:33 [PATCH] mm/userfaultfd: Reset ptes when close() for wr-protected ones Peter Xu
2024-04-22 15:09 ` David Hildenbrand
2024-04-22 19:47 ` Andrew Morton
2024-04-22 21:08   ` Peter Xu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.