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

[netdev-bot+sashiko@kernel.org]

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