[PATCH net] xfrm: hold migrated state references through xfrm_migrate()

[PATCH net] xfrm: hold migrated state references through xfrm_migrate()

[Jun Yang]

From: Jun Yang <junvyyang@tencent.com>

xfrm_state_migrate_install() transfers the cloned state's allocation
reference to the state table. xfrm_state_migrate() returns the state
without a caller reference, and xfrm_migrate() saves it in x_new[].

If a later migration step fails, xfrm_migrate() deletes those states
during rollback. A concurrent deletion, for example from the expiry
timer, can drop the table reference and free a state before rollback
calls xfrm_state_delete().

Take a caller reference before installing each clone. Drop it if
installation fails, after a successful migration, or after deleting the
new states on rollback.

Fixes: 80c9abaabf42 ("[XFRM]: Extension for dynamic update of endpoint address(es)")
Cc: stable@kernel.org
Reported-by: TencentOS Corvus AI <corvus@tencent.com>
Signed-off-by: Jun Yang <junvyyang@tencent.com>
---
A KASAN reproducer for this issue is available if requested.

 net/xfrm/xfrm_policy.c | 7 ++++++-
 net/xfrm/xfrm_state.c  | 6 +++++-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 9ea60c7717ff..06aa036701c3 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -4726,6 +4726,9 @@ int xfrm_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
 		xfrm_states_delete(x_cur, nx_cur);
 	}
 
+	if (nx_new)
+		xfrm_states_put(x_new, nx_new);
+
 	/* Stage 5 - announce */
 	km_migrate(sel, dir, type, m, num_migrate, k, net, encap);
 
@@ -4740,8 +4743,10 @@ restore_state:
 		xfrm_pol_put(pol);
 	if (nx_cur)
 		xfrm_states_put(x_cur, nx_cur);
-	if (nx_new)
+	if (nx_new) {
 		xfrm_states_delete(x_new, nx_new);
+		xfrm_states_put(x_new, nx_new);
+	}
 
 	return err;
 }
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index e45aa1ed5b96..aa97acaacd64 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -2220,8 +2220,12 @@ struct xfrm_state *xfrm_state_migrate(struct xfrm_state *x,
 
 	xfrm_migrate_sync(xc, x);
 
-	if (xfrm_state_migrate_install(x, xc, m, extack) < 0)
+	/* Keep a caller reference after installation transfers ownership. */
+	xfrm_state_hold(xc);
+	if (xfrm_state_migrate_install(x, xc, m, extack) < 0) {
+		xfrm_state_put(xc);
 		return NULL;
+	}
 
 	return xc;
 }
-- 
2.43.7

Re: [PATCH net] xfrm: hold migrated state references through xfrm_migrate()

[netdev-bot+sashiko]

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

Critical: 1 · High: 0 · Medium: 0 · 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).

- [Critical] This patch fixes put-before-delete only for x_new[].

--

xfrm: hold migrated state references through xfrm_migrate()

This patch makes xfrm_state_migrate() take a caller reference on the
cloned state before installing it. xfrm_migrate() then drops that
reference after a successful migration, or after deleting the new states
on rollback. The goal is to stop a concurrent deletion, such as one from
the expiry timer, from freeing a migrated state before xfrm_migrate()
calls xfrm_state_delete() on it.

> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index cea6842e6454d..89e5acf4c210b 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -4730,6 +4730,9 @@ int xfrm_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
>  		xfrm_states_delete(x_cur, nx_cur);
>  	}
>  
> +	if (nx_new)
> +		xfrm_states_put(x_new, nx_new);
> +

[Severity: Critical]
This fixes the ordering for x_new[]. Can the old states in x_cur[] still
hit the same use-after-free on the success path?

Stage 4 in xfrm_migrate() drops the caller reference first and only then
deletes the states:

net/xfrm/xfrm_policy.c:xfrm_migrate() {
    ...
	/* Stage 4 - delete old state(s) */
	if (nx_cur) {
		xfrm_states_put(x_cur, nx_cur);
		xfrm_states_delete(x_cur, nx_cur);
	}
    ...
}

The put loop releases the reference taken by xfrm_migrate_state_find().
After that, each x_cur[i] is held only by the table reference. Several
paths can drop that reference concurrently, and none of them take
xfrm_cfg_mutex:

  - xfrm_timer_handler()->__xfrm_state_delete() on hard expiry
  - a PF_KEY SADB_DELETE or flush, serialized by pfkey_mutex instead
  - a datapath byte or packet hard limit that fires the timer at once

If that happens between Stage 2 and the put loop, the put in Stage 4 is
the final one. If it happens between the put loop and the delete loop,
the concurrent deleter does the final put. Either way the state goes to
the GC worker:

__xfrm_state_destroy()
  -> schedule_work(&xfrm_state_gc_work)
     xfrm_state_gc_task()
       -> synchronize_rcu()
       -> xfrm_state_gc_destroy()

xfrm_migrate() is in preemptible process context and holds no RCU read
lock. Could the free therefore finish before this call reads freed
memory?

xfrm_states_delete(x_cur, nx_cur)
  -> xfrm_state_delete(x)
       spin_lock_bh(&x->lock);
       err = __xfrm_state_delete(x);

If the memory has been reused and km.state no longer reads as DEAD,
__xfrm_state_delete() also unlinks list and hash nodes in freed memory
and puts the state a second time.

nx_cur can be up to XFRM_MAX_DEPTH. The whole put loop finishes before
any delete runs, so the window for later entries also covers the
earlier deletes.

The commit message describes this same situation: a concurrent deletion
frees a migrated state before xfrm_migrate() calls xfrm_state_delete().
Should Stage 4 use the order the new restore_state path uses, that is,
xfrm_states_delete(x_cur, nx_cur) and then xfrm_states_put(x_cur,
nx_cur)?

The peer path xfrm_do_migrate_state() in net/xfrm/xfrm_user.c already
does this. It keeps its lookup reference on x across
__xfrm_state_delete(x) and drops it afterwards.

XFRM_MSG_MIGRATE only requires CAP_NET_ADMIN in the netlink socket's
user namespace. That means a user and net namespace can reach this
path, and the same user controls the SA lifetimes that drive the expiry
timer.

>  	/* Stage 5 - announce */
>  	km_migrate(sel, dir, type, m, num_migrate, k, net, encap);
>  

[ ... ]

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