Re: [PATCH 08/12] xfrm: use hlist_del_init_rcu for state_cache and state_cache_input

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
xfrm: use hlist_del_init_rcu for state_cache and state_cache_input

This switches the two remaining hlist_del_rcu() calls in
__xfrm_state_delete() (state_cache and state_cache_input) to
hlist_del_init_rcu(), so that all six unhash sites in that function leave
pprev == NULL.  The changelog describes this as closing a write
use-after-free that in turn produces a read use-after-free in
xfrm_input_state_lookup().

There are no Link: tags in the commit header.

Two questions below, one about the changelog and one about whether the
state_cache half of the change can have any effect.

> A second __xfrm_state_delete() therefore enters __hlist_del() on the
> already-deleted state_cache/state_cache_input nodes and does
> WRITE_ONCE(*pprev, next) through LIST_POISON2 — a write use-after-free
> once the slab is reused. The corruption can in turn cause a subsequent
> hlist_for_each_entry_rcu traversal to follow a dangling next pointer,
> producing the read use-after-free reported in xfrm_input_state_lookup().

[Severity: High]
Can a second __xfrm_state_delete() reach the unhash block at all?  The
whole body is gated on km.state, and the gate sets DEAD as its first
action:

net/xfrm/xfrm_state.c:__xfrm_state_delete() {
	if (x->km.state != XFRM_STATE_DEAD) {
		x->km.state = XFRM_STATE_DEAD;

		spin_lock(&net->xfrm.xfrm_state_lock);
		list_del(&x->km.all);
		hlist_del_init_rcu(&x->bydst);
		...
}

All four callers hold x->lock across that test-and-set:

	xfrm_state_delete()      spin_lock_bh(&x->lock) ... spin_unlock_bh()
	xfrm_timer_handler()     spin_lock(&x->lock), and also returns early
	                         on x->km.state == XFRM_STATE_DEAD
	xfrm_add_sa_expire()     spin_lock_bh(&x->lock) (net/xfrm/xfrm_user.c)
	xfrm_do_migrate_state()  spin_lock_bh(&x->lock) (net/xfrm/xfrm_user.c)

and no path resets km.state away from DEAD.  Wouldn't a second call
therefore return -ESRCH before touching any hlist?

Separately, is the described mechanism the one that would occur?  If the
gate were somehow bypassed, __hlist_del()'s very first store goes through
the poisoned pprev:

include/linux/list.h:__hlist_del() {
	struct hlist_node *next = n->next;
	struct hlist_node **pprev = n->pprev;

	WRITE_ONCE(*pprev, next);
	if (next)
		WRITE_ONCE(next->pprev, pprev);
}

Since hlist_del_rcu() leaves pprev == LIST_POISON2, which is a fixed
illegal address, isn't the result an immediate oops rather than "a write
use-after-free once the slab is reused"?  And as the store to
next->pprev is never reached, how would this corrupt a live chain so that
a later hlist_for_each_entry_rcu() follows a dangling next pointer?

If the read use-after-free in xfrm_input_state_lookup() is real, it looks
like it has some other cause (an xfrm_state still reachable from an RCU
list after being freed), which this change does not address.  Given the
two Fixes: tags and Cc: stable, could the changelog be reworded to
describe the change as consistency hardening, and could a Reported-by:,
Closes: or the KASAN splat/reproducer be included so the actual lifetime
bug can be tracked?

One more note on the stated threat model: if a double
__xfrm_state_delete() were possible, list_del(&x->km.all),
net->xfrm.state_num-- and xfrm_state_put(x) in the same body are still
non-idempotent, so would making only the hlist removals idempotent be
sufficient?  Turning the poisoned-pprev write into a silent no-op also
removes the oops that would point at the real problem.

> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index de097bba803b7..e45aa1ed5b965 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -826,9 +826,9 @@ int __xfrm_state_delete(struct xfrm_state *x)
>  		if (!hlist_unhashed(&x->byseq))
>  			hlist_del_init_rcu(&x->byseq);
>  		if (!hlist_unhashed(&x->state_cache))
> -			hlist_del_rcu(&x->state_cache);
> +			hlist_del_init_rcu(&x->state_cache);

[Severity: Medium]
Can !hlist_unhashed(&x->state_cache) ever be true here?  The only site in
the tree that hashes x->state_cache is at the tail of xfrm_state_find():

net/xfrm/xfrm_state.c:xfrm_state_find() {
	if (x && x->km.state == XFRM_STATE_VALID && !cached &&
	    (!(pol->flags & XFRM_POLICY_CPU_ACQUIRE) || x->pcpu_num == pcpu_id)) {
		spin_lock_bh(&net->xfrm.xfrm_state_lock);
		if (hlist_unhashed(&x->state_cache))
			hlist_add_head_rcu(&x->state_cache, &pol->state_cache_list);
		spin_unlock_bh(&net->xfrm.xfrm_state_lock);
	}

but cached is set at a label that is also the fall-through of the second
state_cache_list loop:

cached:
	cached = true;
	if (best)
		goto found;
	else if (error)
		best = NULL;

The only jump to that label is the cache-hit case "if (best) goto cached;"
after the first loop, and all other paths simply fall into it, so isn't
cached unconditionally true by the time the tail check runs?  That would
make the hlist_add_head_rcu(&x->state_cache, &pol->state_cache_list) call
unreachable, leave pol->state_cache_list permanently empty (the loop in
xfrm_policy_kill() then also always walks an empty list), and mean the
guard above can never be true.

The cached = true placement comes from 0045e3d80613 ("xfrm: Cache used
outbound xfrm states at the policy.") rather than from this patch, but it
appears to make the state_cache half of this change inert and contradicts
the changelog's claim that the state_cache node can be double-deleted.
Would setting cached = true only on the cache-hit branch, instead of at a
label that is also fallen into, be the right fix?

>  		if (!hlist_unhashed(&x->state_cache_input))
> -			hlist_del_rcu(&x->state_cache_input);
> +			hlist_del_init_rcu(&x->state_cache_input);
>  
>  		if (!hlist_unhashed(&x->byspi))
>  			hlist_del_init_rcu(&x->byspi);