linux-kernel - Re: [PATCH v2 3/4] userfaultfd: prevent unregistering VMAs through a different userfaultfd

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: <CAKha_soChL=TmSAK_yBQYnyjdNRpjp121N5F8XRA=4O9Q+_wvw@mail.gmail.com>
Date: Tue, 17 Jun 2025 16:50:22 -0400
From: Tal Zussman <tz2294@...umbia.edu>
To: David Hildenbrand <david@...hat.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>, Peter Xu <peterx@...hat.com>,
        "Jason A. Donenfeld" <Jason@...c4.com>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
        Andrea Arcangeli <aarcange@...hat.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH v2 3/4] userfaultfd: prevent unregistering VMAs through a
 different userfaultfd

On Tue, Jun 10, 2025 at 3:31 AM David Hildenbrand <david@...hat.com> wrote:
>
> On 07.06.25 08:40, Tal Zussman wrote:
> > Currently, a VMA registered with a uffd can be unregistered through a
> > different uffd associated with the same mm_struct.
> >
> > The existing behavior is slightly broken and may incorrectly reject
> > unregistering some VMAs due to the following check:
> >
> > if (!vma_can_userfault(cur, cur->vm_flags, wp_async))
> > goto out_unlock;
> >
> > where wp_async is derived from ctx, not from cur. For example, a file-backed
> > VMA registered with wp_async enabled and UFFD_WP mode cannot be unregistered
> > through a uffd that does not have wp_async enabled.
> >
> > Rather than fix this and maintain this odd behavior, make unregistration
> > stricter by requiring VMAs to be unregistered through the same uffd they
> > were registered with. Additionally, reorder the WARN() checks to avoid
> > the aforementioned wp_async issue in the WARN()s.
> >
> > This change slightly modifies the ABI. It should not be backported to
> > -stable.
>
> Probably add that the expectation is that nobody really depends on this
> behavior, and that no such cases are known.
>
> >
> > While at it, correct the comment for the no userfaultfd case. This seems to
> > be a copy-paste artifact from the analogous userfaultfd_register() check.
> >
> > Fixes: 86039bd3b4e6 ("userfaultfd: add new syscall to provide memory externalization")
>
> Fixes should come before anything else in a series (Andrew even prefers
> a separate series for fixes vs. follow-up cleanups).
>
> > Signed-off-by: Tal Zussman <tz2294@...umbia.edu>
> > ---
> >   fs/userfaultfd.c | 17 +++++++++++++----
> >   1 file changed, 13 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> > index 80c95c712266..10e8037f5216 100644
> > --- a/fs/userfaultfd.c
> > +++ b/fs/userfaultfd.c
> > @@ -1466,6 +1466,16 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
> >   VM_WARN_ON_ONCE(!!cur->vm_userfaultfd_ctx.ctx ^
> >   !!(cur->vm_flags & __VM_UFFD_FLAGS));
> >
> > + /*
> > + * Check that this VMA isn't already owned by a different
> > + * userfaultfd. This provides for more strict behavior by
> > + * preventing a VMA registered with a userfaultfd from being
> > + * unregistered through a different userfaultfd.
> > + */
>
> Probably we can shorted to:
>
> /*
>   * Prevent unregistering through another userfaultfd than used for
>   * registering.
>   */
>
> ?
>
> > + if (cur->vm_userfaultfd_ctx.ctx &&
> > +    cur->vm_userfaultfd_ctx.ctx != ctx)
> > + goto out_unlock;
> > +
> >   /*
> >   * Check not compatible vmas, not strictly required
> >   * here as not compatible vmas cannot have an
> > @@ -1489,15 +1499,14 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
> >   for_each_vma_range(vmi, vma, end) {
> >   cond_resched();
> >
> > - VM_WARN_ON_ONCE(!vma_can_userfault(vma, vma->vm_flags, wp_async));
> > -
> >   /*
> > - * Nothing to do: this vma is already registered into this
> > - * userfaultfd and with the right tracking mode too.
> > + * Nothing to do: this vma is not registered with userfaultfd.
> >   */
>
> Maybe
>
> /* VMA not registered with userfaultfd. */
>
> The "skip" below is rather clear. :)
>
> >   if (!vma->vm_userfaultfd_ctx.ctx)
> >   goto skip;
> >
> > + VM_WARN_ON_ONCE(vma->vm_userfaultfd_ctx.ctx != ctx);
> > + VM_WARN_ON_ONCE(!vma_can_userfault(vma, vma->vm_flags, wp_async));
> >   WARN_ON(!(vma->vm_flags & VM_MAYWRITE));
> >
> >   if (vma->vm_start > start)
> >
>
> Acked-by: David Hildenbrand <david@...hat.com>

Thanks! Will update with the suggested comment + commit message changes and move
this patch before the VM_WARN changes.

> --
> Cheers,
>
> David / dhildenb
>