[PATCH ipsec] xfrm: fix output state cache TOCTOU in xfrm_state_find()

[PATCH ipsec] xfrm: fix output state cache TOCTOU in xfrm_state_find()

[Tristan Madani]

From: Tristan Madani <tristan@talencesecurity.com>

xfrm_state_find() checks x->km.state outside the xfrm_state_lock before
inserting the state into the policy output state cache. Between the check
and the lock acquisition, __xfrm_state_delete() can transition the state
to XFRM_STATE_DEAD. The dead state is then inserted into the cache
because the stale validity result is trusted after the lock is acquired.

A subsequent call traversing the cache can then access a freed state,
since xfrm_state_gc_destroy() does not unlink from state_cache_list.

This is the output state cache counterpart of the input cache race
fixed by commit ddd3d01329203 ("xfrm: Fix xfrm state cache insertion
race"). Apply the same pattern: move the km.state validity check inside
the lock so the check and the insertion are atomic with respect to
__xfrm_state_delete().

Fixes: 0045e3d80613 ("xfrm: Cache used outbound xfrm states at the policy.")
Cc: stable@vger.kernel.org
Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
---
 net/xfrm/xfrm_state.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index e45aa1ed5b965..03baa50f0df7c 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1632,10 +1632,11 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
 		*err = acquire_in_progress ? -EAGAIN : error;
 	}
 
-	if (x && x->km.state == XFRM_STATE_VALID && !cached &&
+	if (x && !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))
+		if (x->km.state == XFRM_STATE_VALID &&
+		    hlist_unhashed(&x->state_cache))
 			hlist_add_head_rcu(&x->state_cache, &pol->state_cache_list);
 		spin_unlock_bh(&net->xfrm.xfrm_state_lock);
 	}
-- 
2.47.3

Re: [PATCH ipsec] xfrm: fix output state cache TOCTOU in xfrm_state_find()

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH ipsec] xfrm: fix output state cache TOCTOU in xfrm_state_find()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The race this patch says it fixes cannot happen on this tree,
  because the code the patch changes never runs.

--

xfrm: fix output state cache TOCTOU in xfrm_state_find()

This commit moves the x->km.state == XFRM_STATE_VALID check in
xfrm_state_find() inside xfrm_state_lock, so that inserting a state into
pol->state_cache_list cannot race with __xfrm_state_delete(). It is
described as the output-cache counterpart of commit ddd3d01329203 ("xfrm:
Fix xfrm state cache insertion race"), and it is tagged for stable.

> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index 0555e5d796ede..581e6d0d3ae3d 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -1651,10 +1651,11 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
>  		*err = acquire_in_progress ? -EAGAIN : error;
>  	}
>
> -	if (x && x->km.state == XFRM_STATE_VALID && !cached &&
> +	if (x && !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))
> +		if (x->km.state == XFRM_STATE_VALID &&
> +		    hlist_unhashed(&x->state_cache))
>  			hlist_add_head_rcu(&x->state_cache, &pol->state_cache_list);
>  		spin_unlock_bh(&net->xfrm.xfrm_state_lock);
>  	}

[Severity: Medium]
Can this block run at all? Earlier in xfrm_state_find(), the cached label
sets the flag on every path:

net/xfrm/xfrm_state.c:xfrm_state_find() {
    ...
	if (best)
		goto cached;

	hlist_for_each_entry_rcu(x, &pol->state_cache_list, state_cache) {
		...
	}

cached:
	cached = true;
	if (best)
		goto found;
    ...
}

On a cache hit the code jumps to the label. On a cache miss it falls
through to it. No goto skips the assignment, and cached is only ever set
to false at its declaration.

That makes the !cached test above always false. The hlist_add_head_rcu()
here is the only place that adds to pol->state_cache_list, so the list
always stays empty.

If that's right, the scenario in the commit message cannot happen on this
tree, with or without the patch:

  "Between the check and the lock acquisition, __xfrm_state_delete() can
  transition the state to XFRM_STATE_DEAD. The dead state is then
  inserted into the cache"

  "A subsequent call traversing the cache can then access a freed state"

The unconditional cached = true came in with 0045e3d80613 itself, together
with the !cached guard. So it looks like no tree with this feature can
reach the insertion, and the Fixes: and Cc: stable tags point to a race
that can't be triggered.

This is a separate issue that the patch doesn't introduce, but it follows
that the outbound per-policy cache from 0045e3d80613 is never filled.
Every outbound lookup then falls back to the full bydst and wildcard hash
search.

Doing the km.state check under the lock is still the right pattern if the
insertion is ever made reachable. Should this patch be described as
hardening instead? Or should it be paired with a change that sets cached
only when the state actually came from a state_cache_list search?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023195323.3528192-1-tristmd%40gmail.com