linux-kernel - Re: [PATCH 2/3] 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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <84cf5418-42e9-4ec5-bd87-17ba91995c47@redhat.com>
Date: Wed, 4 Jun 2025 15:23:38 +0200
From: David Hildenbrand <david@...hat.com>
To: Tal Zussman <tz2294@...umbia.edu>,
 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>,
 Pavel Emelyanov <xemul@...allels.com>, Andrea Arcangeli <aarcange@...hat.com>
Cc: linux-mm@...ck.org, linux-kernel@...r.kernel.org,
 linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH 2/3] userfaultfd: prevent unregistering VMAs through a
 different userfaultfd

On 04.06.25 00:14, Tal Zussman wrote:
> Currently, a VMA registered with a uffd can be unregistered through a
> different uffd asssociated with the same mm_struct.
> 
> Change this behavior to be stricter by requiring VMAs to be unregistered
> through the same uffd they were registered with.
> 
> While at it, correct the comment for the no userfaultfd case. This seems
> to be a copy-paste artifact from the analagous userfaultfd_register()
> check.

I consider it a BUG that should be fixed. Hoping Peter can share his 
opinion.

> 
> Fixes: 86039bd3b4e6 ("userfaultfd: add new syscall to provide memory externalization")
> Signed-off-by: Tal Zussman <tz2294@...umbia.edu>
> ---
>   fs/userfaultfd.c | 15 +++++++++++++--
>   1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index 22f4bf956ba1..9289e30b24c4 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -1477,6 +1477,16 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
>   		if (!vma_can_userfault(cur, cur->vm_flags, wp_async))
>   			goto out_unlock;
>   
> +		/*
> +		 * 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.
> +		 */
> +		if (cur->vm_userfaultfd_ctx.ctx &&
> +		    cur->vm_userfaultfd_ctx.ctx != ctx)
> +			goto out_unlock;

So we allow !cur->vm_userfaultfd_ctx.ctx to allow unregistering when 
there was nothing registered.

A bit weird to set "found = true" in that case. Maybe it's fine, just 
raising it ...

> +
>   		found = true;
>   	} for_each_vma_range(vmi, cur, end);
>   	BUG_ON(!found);
> @@ -1491,10 +1501,11 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
>   		cond_resched();
>   
>   		BUG_ON(!vma_can_userfault(vma, vma->vm_flags, wp_async));
> +		BUG_ON(vma->vm_userfaultfd_ctx.ctx &&
> +		       vma->vm_userfaultfd_ctx.ctx != ctx);
>   

No new BUG_ON please. VM_WARN_ON_ONCE() if we really care. After all, we 
checked this above ...

>   		/*
> -		 * 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.
>   		 */
>   		if (!vma->vm_userfaultfd_ctx.ctx)
>   			goto skip;
> 

-- 
Cheers,

David / dhildenb