Re: [PATCH v1] ucount: take the rlimit reference before checking the limit

[Bradley Morgan <brads@mainlining.org>]

On 18 October 2026 00:48:34 BST, Nickolai Zeldovich
<nickolai@csail.mit.edu> wrote:
>On a kernel with user namespaces enabled (CONFIG_USER_NS=y, the default on
>most distributions) an unprivileged user can free a live struct ucounts,
>and the user_namespace it pins, while they are still in use, giving a use-
>after-free on the next signal-accounting access or an rcuref "imbalanced
>put()" warning. It needs no privilege beyond creating a user namespace: in
>a namespace whose RLIMIT_SIGPENDING is 0, race a thread queueing real-time
>signals (which obey the limit and fail at that namespace's ucounts level)
>against one taking a synchronous fault such as SIGSEGV (delivered with
>override_rlimit set, crossing the same level without the limit check);
>both charge the same per-namespace SIGPENDING counter. The defect has been
>present since inc_rlimit_get_ucounts() was introduced in v5.15.
>
>inc_rlimit_get_ucounts() walks the ucounts chain and, for each level,
>increments iter->rlimit[type] and then grabs one extra reference on the
>ucounts when the counter goes from 0 to 1 -- the reference that pins the
>ucounts for as long as the rlimit count stays non-zero, released by
>dec_rlimit_put_ucounts() when the counter returns to 0. The reference is
>taken only by the thread that observes new == 1, and only after the limit
>check: a level that fails "new > max" jumps to dec_unwind, which decrements
>iter->rlimit[type] again but takes no reference.
>
>That ordering is unsafe under concurrency. The thread that performs the
>0 -> 1 transition is not guaranteed to keep its increment:
>
>  CPU0 (fails the limit)              CPU1 (overrides it)
>  new = add_return(1) -> 1
>                                      new = add_return(1) -> 2
>                                      new != 1, no get_ucounts()
>  new > max -> dec_unwind
>  sub_return(1) -> 1   (no put)
>
>The count is left at 1 with no reference held: CPU0 took the increment to
>1
>but backed out without taking the reference, while CPU1's surviving
>increment never took one. When CPU1 later frees its charge,
>dec_rlimit_put_ucounts() drops the count 1 -> 0 and calls put_ucounts()
>for
>a reference nobody ever took. struct ucounts is freed with kfree_rcu()
>when
>its last reference goes away, so this stray put drops a live ucounts and
>frees it while it is still charged and still on the ucounts hash, giving a
>use-after-free of the ucounts (and of the user_namespace it pinned). The
>symmetric interleaving instead leaks a reference (CPU0 succeeds and takes
>the reference, CPU1 fails and unwinds the level to 0 with no put), pinning
>the ucounts and its user_namespace forever.
>
>The invariant the code wants is "rlimit[type] > 0 implies exactly one
>ucounts reference held for that level".
>
>Take the reference on the 0 -> 1 transition before the limit is decided,
>so
>the thread that raises the count from zero owns that level's reference
>even
>when its own request is rejected; the rejecting path then unwinds iter
>through do_dec_rlimit_put_ucounts(), which releases that reference when
>the
>counter returns to 0. The get_ucounts() failure path is unchanged: no
>reference was taken, so iter is decremented without a put.
>
>Fixes: 15bc01effefe ("ucounts: Fix signal ucount refcounting")
>Assisted-by: LLM
>Signed-off-by: Nickolai Zeldovich <nickolai@csail.mit.edu>
>---
>
>Notes:
>    A reproducer, available on request, wins the race within seconds
>    under KASAN or rcuref debugging.
>
> kernel/ucount.c | 25 ++++++++++++++++---------
> 1 file changed, 16 insertions(+), 9 deletions(-)
>
>diff --git a/kernel/ucount.c b/kernel/ucount.c
>index ec8b1445e287..44a78ae80579 100644
>--- a/kernel/ucount.c
>+++ b/kernel/ucount.c
>@@ -308,23 +308,30 @@ long inc_rlimit_get_ucounts(struct ucounts *ucounts, enum rlimit_type type,
> 
> 	for (iter = ucounts; iter; iter = iter->ns->ucounts) {
> 		long new = atomic_long_add_return(1, &iter->rlimit[type]);
>-		if (new < 0 || new > max)
>+		/*
>+		 * Grab the extra reference when this walk observes the 0 -> 1
>+		 * transition, before the limit is checked: the thread that
>+		 * raises the count from zero owns this level's reference, even
>+		 * when the limit check below rejects the request. Otherwise a
>+		 * concurrent walk that raised the count past 1 is left holding
>+		 * the surviving increment with no reference, and its later
>+		 * dec_rlimit_put_ucounts() drops a reference nobody took.
>+		 */

I think this is way too long.

/* Hold extra ref on 0->1 transition so failure doesn't leave unheld ref
for concurrent dec */ ?

Feel free to bikeshed, as usual, because sometimes my comments are
^W^Wadmittantly crap^W^W


>+		if (new == 1 && !get_ucounts(iter))
> 			goto dec_unwind;
>+		if (new < 0 || new > max) {
>+			/* undo iter too, releasing the reference taken above */
>+			do_dec_rlimit_put_ucounts(ucounts, iter->ns->ucounts, type);
>+			return 0;
>+		}
> 		if (iter == ucounts)
> 			ret = new;
> 		if (!override_rlimit)
> 			max = get_userns_rlimit_max(iter->ns, type);
>-		/*
>-		 * Grab an extra ucount reference for the caller when
>-		 * the rlimit count was previously 0.
>-		 */
>-		if (new != 1)
>-			continue;
>-		if (!get_ucounts(iter))
>-			goto dec_unwind;
> 	}
> 	return ret;
> dec_unwind:
>+	/* get_ucounts() failed for iter: undo iter without a put */
> 	dec = atomic_long_sub_return(1, &iter->rlimit[type]);
> 	WARN_ON_ONCE(dec < 0);
> 	do_dec_rlimit_put_ucounts(ucounts, iter, type);

Yeah, the rest makes sense. Just change that.

>
>base-commit: 70dd4ee72eb2d5793c62e71c14e3aba78e5a4e56
>

--- Thanks!
"I'm not a very positive person" - Linus torvalds