linux-kernel - Re: [PATCH 1/2] mm/userfaultfd: Fix uninitialized output field for -EAGAIN race

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: <23e2d207-58ac-49d3-b93e-4105a0624f9d@redhat.com>
Date: Fri, 25 Apr 2025 17:12:37 +0200
From: David Hildenbrand <david@...hat.com>
To: Peter Xu <peterx@...hat.com>, linux-kernel@...r.kernel.org,
 linux-mm@...ck.org
Cc: Mike Rapoport <rppt@...nel.org>, James Houghton <jthoughton@...gle.com>,
 Suren Baghdasaryan <surenb@...gle.com>,
 Axel Rasmussen <axelrasmussen@...gle.com>,
 Andrew Morton <akpm@...ux-foundation.org>,
 linux-stable <stable@...r.kernel.org>, Andrea Arcangeli <aarcange@...hat.com>
Subject: Re: [PATCH 1/2] mm/userfaultfd: Fix uninitialized output field for
 -EAGAIN race

On 24.04.25 23:57, Peter Xu wrote:
> While discussing some userfaultfd relevant issues recently, Andrea noticed
> a potential ABI breakage with -EAGAIN on almost all userfaultfd ioctl()s.

I guess we talk about e.g., "man UFFDIO_COPY" documentation:

"The copy field is used by the kernel to return the number of bytes that 
was actually copied,  or an  error  (a  negated errno-style value).  The 
copy field is output-only; it is not read by the UFFDIO_COPY operation."

I assume -EINVAL/-ESRCH/-EFAULT are excluded from that rule, because 
there is no sense in user-space trying again on these errors either way. 
Well, there are cases where we would store -EFAULT, when we receive it 
from mfill_atomic_copy().

So if we store -EAGAIN to copy.copy it says "we didn't copy anything". 
(probably just storing 0 would have been better, but I am sure there was 
a reason to indicate negative errors in addition to returning an error)

> 
> Quote from Andrea, explaining how -EAGAIN was processed, and how this
> should fix it (taking example of UFFDIO_COPY ioctl):
> 
>    The "mmap_changing" and "stale pmd" conditions are already reported as
>    -EAGAIN written in the copy field, this does not change it. This change
>    removes the subnormal case that left copy.copy uninitialized and required
>    apps to explicitly set the copy field to get deterministic
>    behavior (which is a requirement contrary to the documentation in both
>    the manpage and source code). In turn there's no alteration to backwards
>    compatibility as result of this change because userland will find the
>    copy field consistently set to -EAGAIN, and not anymore sometime -EAGAIN
>    and sometime uninitialized.
> 
>    Even then the change only can make a difference to non cooperative users
>    of userfaultfd, so when UFFD_FEATURE_EVENT_* is enabled, which is not
>    true for the vast majority of apps using userfaultfd or this unintended
>    uninitialized field may have been noticed sooner.
> 
> Meanwhile, since this bug existed for years, it also almost affects all
> ioctl()s that was introduced later.  Besides UFFDIO_ZEROPAGE, these also
> get affected in the same way:
> 
>    - UFFDIO_CONTINUE
>    - UFFDIO_POISON
>    - UFFDIO_MOVE
> 
> This patch should have fixed all of them.
> 
> Fixes: df2cc96e7701 ("userfaultfd: prevent non-cooperative events vs mcopy_atomic races")
> Fixes: f619147104c8 ("userfaultfd: add UFFDIO_CONTINUE ioctl")
> Fixes: fc71884a5f59 ("mm: userfaultfd: add new UFFDIO_POISON ioctl")
> Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI")
> Cc: linux-stable <stable@...r.kernel.org>
> Cc: Mike Rapoport <rppt@...nel.org>
> Cc: Axel Rasmussen <axelrasmussen@...gle.com>
> Cc: Suren Baghdasaryan <surenb@...gle.com>
> Reported-by: Andrea Arcangeli <aarcange@...hat.com>
> Suggested-by: Andrea Arcangeli <aarcange@...hat.com>
> Signed-off-by: Peter Xu <peterx@...hat.com>
> ---
>   fs/userfaultfd.c | 28 ++++++++++++++++++++++------
>   1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index d80f94346199..22f4bf956ba1 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -1585,8 +1585,11 @@ static int userfaultfd_copy(struct userfaultfd_ctx *ctx,
>   	user_uffdio_copy = (struct uffdio_copy __user *) arg;
>   
>   	ret = -EAGAIN;
> -	if (atomic_read(&ctx->mmap_changing))
> +	if (unlikely(atomic_read(&ctx->mmap_changing))) {
> +		if (unlikely(put_user(ret, &user_uffdio_copy->copy)))
> +			return -EFAULT;
>   		goto out;
> +	}

Nit: It's weird that we do "return -EFAULT" in one case, in the other we 
do "goto out;" which ends up doing a "return ret" ...

Maybe to keep it consistent:

ret = -EAGAIN;
if (unlikely(atomic_read(&ctx->mmap_changing))) {
	if (unlikely(put_user(ret, &user_uffdio_copy->copy)))
		ret = -EFAULT;
    	goto out;
}


In all of these functions, we should probably just get rid of the "goto 
out" and just return directly. We have a weird mixture of "goto out;" 
and return; ... a different cleanup.


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

-- 
Cheers,

David / dhildenb