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/[email protected]/
Analyzed-by: David Hildenbrand <[email protected]>
Reported-by: [email protected]
Signed-off-by: Peter Xu <[email protected]>
---
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
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/[email protected]/
> Analyzed-by: David Hildenbrand <[email protected]>
> Reported-by: [email protected]
> Signed-off-by: Peter Xu <[email protected]>
> ---
> 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 <[email protected]>
--
Cheers,
David / dhildenb
On Mon, 22 Apr 2024 09:33:11 -0400 Peter Xu <[email protected]> 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?
On Mon, Apr 22, 2024 at 12:47:19PM -0700, Andrew Morton wrote:
> On Mon, 22 Apr 2024 09:33:11 -0400 Peter Xu <[email protected]> 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 <[email protected]>
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