crypto: rng - Fix spurious EFAULT when the destination PTE is zapped

While GUP pinning makes it possible to pin the page _backing_ a user
address, it *doesn't* pin the page table entry (PTE) for that mapping. This
means the pinned physical page can be separated from the user address it
was backing, and even back a _different_ user address within the same
process. PTE zapping naturally happens during memory reclaim when memory
pressure is elevated, and can even be done directly by userspace via
madvise(MADV_DONTNEED).

Since the optimized per-CPU DRBG loop assumes copy_to_user_nofault() will
always succeed on a GUP-pinned page, it immediately bails out when the
nofault copy actually *does* fail for the reasons described above. This
results in either fewer than requested random bytes copied or, more
seriously, a spurious EFAULT returned to userspace when no random bytes
were copied.

As it turns out, there's no way to pin a PTE. That means it's not possible
to guarantee a 100% success rate for the copy_to_user_nofault() attempt.

Fix this by handling copy_to_user_nofault() errors correctly with a fall
back to a faultable copy attempt outside of the RNG lock. In order to
guarantee forward progress for the caller, an on-stack bounce buffer is
used to copy up to 256 bytes of the generated random bytes whenever this
happens rather than discarding the whole thing.

There's no need to use GUP pinning anymore since there's no use for having
a page pinned without pinning a PTE to go along with that page, hence the
page pinning is eliminated which saves a software page table walk that was
performed for _at least_ every destination page.

Reported-by: Kun Yi <kunyi@google.com>
Signed-off-by: Sultan Alsawaf <sultan@ciq.com>

Author: kerneltoast

crypto/rng.c

@@ -418,16 +418,30 @@ clear_rng_page(struct cpu_rng_inst *cri, size_t count)
 	count < PAGE_SIZE ? memset(cri->page, 0, count) : clear_page(cri->page);
 }
 
+static __always_inline void
+no_reseed_clear_unlock(struct cpu_rng_inst *cri, size_t *page_dirty_len)
+{
+	/*
+	 * Clear the buffer of our latest random bytes before unlocking and
+	 * potentially migrating CPUs, in which case we wouldn't have the same
+	 * `cri` anymore.
+	 */
+	clear_rng_page(cri, *page_dirty_len);
+	unlock_local_rng(cri, false);
+	*page_dirty_len = 0;
+}
+
 static ssize_t crypto_devrandom_read_iter(struct iov_iter *iter, bool reseed)
 {
 	/* lock_local_rng() puts us in atomic context for !reseed on non-RT */
 	const bool atomic = !reseed && !IS_ENABLED(CONFIG_PREEMPT_RT);
 	const bool user_no_reseed = !reseed && user_backed_iter(iter);
+	/* Align the bounce buffer to a word to prevent alignment traps */
+	u8 bounce[SZ_256] __aligned(sizeof(long));
 	size_t ulen, page_dirty_len = 0;
 	struct cpu_rng_inst *cri;
 	struct crypto_rng *rng;
-	void __user *uaddr;
-	struct page *upage;
+	u8 __user *uaddr;
 	ssize_t ret = 0;
 
 	if (unlikely(!iov_iter_count(iter)))
@@ -462,21 +476,39 @@ static ssize_t crypto_devrandom_read_iter(struct iov_iter *iter, bool reseed)
 
 restart:
 	/*
-	 * Pin the user page backing the current user destination address,
-	 * potentially prefaulting to allocate a page for the destination. By
-	 * prefaulting without the RNG lock held, the DRBG won't be blocked by
-	 * time spent on page faults for this task, and thus the DRBG can still
-	 * be used by other tasks.
+	 * Prefault the current user destination address in case the page for
+	 * that virtual address is not resident. This is done by simply writing
+	 * a single zero byte to the destination, which forces a COW if the page
+	 * isn't resident, leaving us with a writable mapping. By prefaulting
+	 * without the RNG lock held, the DRBG won't be blocked by time spent on
+	 * page faults for this task, and thus the DRBG can still be used by
+	 * other tasks.
+	 *
+	 * Notably, this doesn't keep the page from getting evicted later on;
+	 * it's just an optimistic prefault under the assumption that the page
+	 * won't be evicted before we copy random bytes into it, which is often
+	 * but _not always_ the case.
+	 *
+	 * While GUP pinning makes it possible to pin the page _backing_ a user
+	 * address, it *doesn't* pin the page table entry (PTE) for that
+	 * mapping. This means the pinned physical page can be separated from
+	 * the user address it was backing, and even back a _different_ user
+	 * address within the same process. PTE zapping naturally happens during
+	 * memory reclaim when memory pressure is elevated, and can even be done
+	 * directly by userspace via madvise(MADV_DONTNEED). Since there's no
+	 * way to pin a PTE, that means it's not possible to guarantee a page
+	 * will be resident when copy_to_user_nofault() runs under the RNG lock
+	 * below; in other words, there's no way to guarantee a 100% success
+	 * rate for the copy_to_user_nofault() attempt.
 	 */
-	if (user_no_reseed && pin_user_pages_fast((unsigned long)uaddr, 1,
-						  FOLL_WRITE, &upage) != 1)
+	if (user_no_reseed && put_user((u8)0, uaddr))
 		goto exit;
 
 	cri = lock_local_rng(&rng, reseed);
 	if (IS_ERR(cri)) {
 		if (!ret)
 			ret = PTR_ERR(cri);
-		goto unpin_upage;
+		goto exit;
 	}
 
 	while (1) {
@@ -486,7 +518,7 @@ static ssize_t crypto_devrandom_read_iter(struct iov_iter *iter, bool reseed)
 
 		/*
 		 * Generate up to one page at a time, and align to a page
-		 * boundary so we only need to pin one user page at a time.
+		 * boundary to track page residence one page at a time.
 		 */
 		if (user_no_reseed)
 			i = min3(i, PAGE_SIZE - offset_in_page(uaddr), ulen);
@@ -523,33 +555,56 @@ static ssize_t crypto_devrandom_read_iter(struct iov_iter *iter, bool reseed)
 		 * the entire call; this is why a mutex is used instead of a
 		 * local lock for the reseed RNG, to permit sleeping without
 		 * yielding the DRBG instance.
+		 *
+		 * From this point on, err != 0 means the nofault copy failed.
 		 */
 		page_dirty_len = max(i, page_dirty_len);
-		if (user_no_reseed) {
-			err = copy_to_user_nofault(uaddr, cri->page, i);
-			if (err >= 0) {
-				iov_iter_advance(iter, i - err);
-				ret += i - err;
-			}
-			if (err)
-				break;
+		if (user_no_reseed &&
+		    !(err = copy_to_user_nofault(uaddr, cri->page, i))) {
+			iov_iter_advance(iter, i);
+			copied = i;
 		} else {
+			const void *src = cri->page;
+
 			/*
-			 * We know that copying from cri->page is safe, so use
-			 * _copy_to_iter() directly to skip check_copy_size().
+			 * Fall back to a faultable copy attempt when the
+			 * non-faulting attempt failed, likely because the PTE
+			 * was zapped. To ensure forward progress, some of the
+			 * generated bytes are retained in an on-stack bounce
+			 * buffer and copied out rather than just discarding all
+			 * of it and going back to `restart`.
 			 */
-			copied = _copy_to_iter(cri->page, i, iter);
-			ret += copied;
-			if (copied != i)
-				break;
+			if (unlikely(err)) {
+				i = min(i, sizeof(bounce));
+				memcpy(bounce, cri->page, i);
+				no_reseed_clear_unlock(cri, &page_dirty_len);
+				src = bounce;
+			}
+
+			/*
+			 * We know that copying from cri->page (or the bounce
+			 * buffer) is safe, so use _copy_to_iter() directly to
+			 * skip check_copy_size().
+			 */
+			copied = _copy_to_iter(src, i, iter);
+
+			/* Sanitize the on-stack bounce buffer if it was used */
+			if (err)
+				memzero_explicit(bounce, i);
 		}
+		ret += copied;
 
 		/*
-		 * Quit when either the requested number of bytes have been
-		 * generated or there is a pending signal.
+		 * Quit when the copy fell short, the requested number of bytes
+		 * have been generated, or there is a pending signal.
 		 */
-		if (!iov_iter_count(iter) || signal_pending(current))
+		if (copied != i || !iov_iter_count(iter) ||
+		    signal_pending(current)) {
+			/* Skip the unlock when already unlocked */
+			if (unlikely(err))
+				goto exit;
 			break;
+		}
 
 		/* Compute the next user destination address and length */
 		if (user_no_reseed) {
@@ -570,8 +625,6 @@ static ssize_t crypto_devrandom_read_iter(struct iov_iter *iter, bool reseed)
 				uaddr = iter_iov(iter)->iov_base;
 				ulen = iter_iov(iter)->iov_len;
 			}
-
-			unpin_user_page(upage);
 		}
 
 		/*
@@ -586,33 +639,27 @@ static ssize_t crypto_devrandom_read_iter(struct iov_iter *iter, bool reseed)
 		}
 
 		/*
-		 * Optimistically try to pin the next user page without
-		 * faulting, so we don't need to clear cri->page and drop the
-		 * lock on every iteration. If this fails, we fall back to
-		 * pinning with the option to prefault.
+		 * Optimistically check if the next user page is resident and
+		 * writable with a dummy copy_to_user_nofault() attempt to write
+		 * a single zero byte. If this fails, fall back to prefaulting
+		 * outside of the lock.
 		 */
-		if (user_no_reseed && !resched_without_lock &&
-		    pin_user_pages_fast_only((unsigned long)uaddr, 1,
-					     FOLL_WRITE, &upage) == 1)
+		if (!err && user_no_reseed && !resched_without_lock &&
+		    !copy_to_user_nofault(uaddr, &(u8){ 0 }, 1))
 			continue;
 
 		/*
 		 * Restart if either rescheduling is needed (and requires
-		 * dropping the lock since we're atomic) or the optimistic page
-		 * pinning attempt failed.
+		 * dropping the lock since we're atomic) or the destination page
+		 * wasn't resident when copy_to_user_nofault() was attempted.
 		 *
 		 * This always implies `reseed == false`, so unlock_local_rng()
-		 * can just be passed `false` for reseed to eliminate a branch.
+		 * can just be passed `false` for reseed to eliminate a branch,
+		 * hence using no_reseed_clear_unlock().
 		 */
 		if (resched_without_lock || user_no_reseed) {
-			/*
-			 * Clear the buffer of our latest random bytes before
-			 * unlocking and potentially migrating CPUs, in which
-			 * case we wouldn't have the same `cri` anymore.
-			 */
-			clear_rng_page(cri, page_dirty_len);
-			unlock_local_rng(cri, false);
-			page_dirty_len = 0;
+			if (!err)
+				no_reseed_clear_unlock(cri, &page_dirty_len);
 			if (resched_without_lock)
 				cond_resched();
 			goto restart;
@@ -622,9 +669,6 @@ static ssize_t crypto_devrandom_read_iter(struct iov_iter *iter, bool reseed)
 	if (page_dirty_len)
 		clear_rng_page(cri, page_dirty_len);
 	unlock_local_rng(cri, reseed);
-unpin_upage:
-	if (user_no_reseed)
-		unpin_user_page(upage);
 exit:
 	return ret ? ret : -EFAULT;
 }