| Message ID | 20240422133311.2987675-1-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/userfaultfd: Reset ptes when close() for wr-protected ones | expand |
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>
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?
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,
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,
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(+)