Re: [PATCH net] xfrm: delete migrated old states before dropping their references

[netdev-bot+sashiko@kernel.org]

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

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

- [High] This patch fixes the Stage 4 x_cur[] path but leaves a matching
  use-after-free on the restore_state error path in xfrm_migrate()…

--

xfrm: delete migrated old states before dropping their references

This patch reorders Stage 4 of xfrm_migrate() so the old states in x_cur[]
are deleted before their lookup references are dropped. The states then
stay alive during xfrm_states_delete(), even if an expiry timer or a
concurrent deletion has already removed them from the state tables.

> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index cea6842e6454d..39ca04432220b 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -4726,8 +4726,8 @@ int xfrm_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
>  
>  	/* Stage 4 - delete old state(s) */
>  	if (nx_cur) {
> -		xfrm_states_put(x_cur, nx_cur);
>  		xfrm_states_delete(x_cur, nx_cur);
> +		xfrm_states_put(x_cur, nx_cur);
>  	}

[Severity: High]
This fixes the x_cur[] path. Does the restore_state error path in
xfrm_migrate() still have the same use-after-free for the x_new[] states?

net/xfrm/xfrm_policy.c:xfrm_migrate() {
    ...
restore_state:
	if (pol)
		xfrm_pol_put(pol);
	if (nx_cur)
		xfrm_states_put(x_cur, nx_cur);
	if (nx_new)
		xfrm_states_delete(x_new, nx_new);
    ...
}

In Stage 2 the new state is stored without taking a reference:

	xc = xfrm_state_migrate(x, mp, net, extack);
	if (xc) {
		x_new[nx_new] = xc;
		nx_new++;

By this point xfrm_state_migrate()->xfrm_state_migrate_install() has
already published xc with xfrm_state_insert() or xfrm_state_add(). The
state tables now own its only reference, and __xfrm_state_insert() has
armed mtimer.

Once published, xc can be found by SPI lookup on the data path. Its curlft
was copied from the old SA by xfrm_migrate_sync(). That means it can be
deleted without xfrm_cfg_mutex, for example:

  xfrm_state_check_expire()   /* hard byte/packet limit reached */
    hrtimer_start(&x->mtimer, 0, HRTIMER_MODE_REL_SOFT);
  xfrm_timer_handler()
    __xfrm_state_delete()
      xfrm_state_put()        /* drops the only reference */

The 1s mtimer reaching hard_add_expires with the copied add_time has the
same effect.

Userspace can still reach restore_state after xc has been published. One
way is xfrm_policy_migrate() returning -ENODATA (no template matches) or
-ENOENT (dead policy). Another is a later entry's xfrm_state_migrate()
returning NULL, for example -EEXIST from xfrm_state_add(). That later
entry's xfrm_state_migrate_create() allocates with GFP_KERNEL and can
sleep, which makes the window wider.

If xc has already been freed by then, xfrm_state_delete(xc) would take
xc->lock and read xc->km.state on freed memory. If the slab has been
reused, this could also lead to list corruption and another put.

Reordering cannot fix this one, because xfrm_migrate() never holds its own
reference on the x_new[] entries. Could a reference be taken on xc before
it is installed? The parallel path xfrm_do_migrate_state() in xfrm_user.c
already does this:

	xfrm_state_hold(xc);
	err = xfrm_state_migrate_install(x, xc, &m, extack);

That reference would then be dropped after xfrm_states_delete() in
restore_state, or on the success path.

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