Re: [PATCH net] xfrm: reject duplicate state resolution in xfrm_migrate()

[netdev-bot+sashiko@kernel.org]

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

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

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] The patch leaves the use-after-free it describes reachable,
  because it removes only one trigger.
- [Low] The commit message understates when duplicate state resolution
  can happen.

--

xfrm: reject duplicate state resolution in xfrm_migrate()

This patch makes xfrm_migrate() reject a migration entry that resolves to
an SA already stored in x_cur[]. It drops the extra lookup reference and
goes through the existing restore_state rollback. The goal is to prevent
the Stage 4 put-then-delete sequence from accessing a state after its
last reference is gone.

> xfrm_migrate_check() rejects identical migration entries, but entries
> with different old_reqid values can still resolve to the same state.
> In particular, old_reqid == 0 is a wildcard, so it can match the same SA
> as an entry carrying that SA's reqid.

[Severity: Low]
Is this description complete? Two entries with the same non-zero
old_reqid can also resolve to the same SA.

xfrm_migrate_check() compares the addresses with memcmp() over the whole
xfrm_address_t:

net/xfrm/xfrm_policy.c:xfrm_migrate_check() {
    ...
	if (!memcmp(&m[i].old_daddr, &m[j].old_daddr,
		    sizeof(m[i].old_daddr)) &&
	    !memcmp(&m[i].old_saddr, &m[j].old_saddr,
		    sizeof(m[i].old_saddr)) &&
    ...
}

copy_from_user_migrate() copies all 16 bytes from userspace:

net/xfrm/xfrm_user.c:copy_from_user_migrate() {
    ...
	memcpy(&ma->old_daddr, &um->old_daddr, sizeof(ma->old_daddr));
	memcpy(&ma->old_saddr, &um->old_saddr, sizeof(ma->old_saddr));
    ...
}

For AF_INET, xfrm_migrate_state_find() uses the family-aware hash
functions and xfrm_addr_equal(..., m->old_family), so only a4 is
compared. Two entries that differ only in the unused upper 12 address
bytes pass xfrm_migrate_check() and still resolve to the same SA.

The new x_cur[j] == x check catches this case too, so the code is fine
here. Could the commit message describe the more general mismatch between
the memcmp() based uniqueness check and the lookup semantics? That would
help stable backporters see which inputs trigger the bug.

> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index cea6842e6454d..41ea5da825b64 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c

[ ... ]

> @@ -4702,6 +4702,17 @@ int xfrm_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
>  	/* Stage 2 - find and update state(s) */
>  	for (i = 0, mp = m; i < num_migrate; i++, mp++) {
>  		if ((x = xfrm_migrate_state_find(mp, net, if_id))) {
> +			for (j = 0; j < nx_cur; j++) {
> +				if (x_cur[j] == x)
> +					break;
> +			}
> +			if (j < nx_cur) {
> +				xfrm_state_put(x);
> +				NL_SET_ERR_MSG(extack,
> +					       "Multiple MIGRATE entries resolve to the same SA");
> +				err = -EINVAL;
> +				goto restore_state;
> +			}

[Severity: High]
Does this fully close the use-after-free in Stage 4? Stage 4 still drops
every lookup reference before it deletes the same 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);
	}
    ...
}

Duplicates in x_cur[] are one way for an entry to have no table reference
left at that point. A concurrent hard expiry looks like another.
xfrm_timer_handler() takes only x->lock, not xfrm_cfg_mutex, and on hard
expiry does:

	err = __xfrm_state_delete(x);

The data path can arm this right away. xfrm_state_check_expire() sets
XFRM_STATE_EXPIRED and starts mtimer with a zero delay once the hard byte
or packet limit is reached.

If __xfrm_state_delete() runs between the Stage 2 lookup and Stage 4, the
lookup hold is the only reference left. The sequence would then be:

xfrm_migrate()
  xfrm_states_put()
    xfrm_state_put()          /* refcnt -> 0 */
      __xfrm_state_destroy()  /* queue on gc list, schedule gc */
  <task preempted>
                              xfrm_state_gc_task()
                                synchronize_rcu()
                                xfrm_state_gc_destroy(x)  /* freed */
  xfrm_states_delete()
    xfrm_state_delete()
      spin_lock_bh(&x->lock)  /* freed memory */

Could Stage 4 call xfrm_states_delete() first and xfrm_states_put()
afterwards? With that order the lookup holds keep each state alive across
xfrm_state_delete(), and a second delete of a state that is already DEAD
just returns -ESRCH. xfrm_do_migrate_state() in xfrm_user.c already
follows this order: it calls __xfrm_state_delete(x) and then
xfrm_state_put(x).

Rejecting duplicates still seems worth keeping so one SA is not cloned
twice. On its own, though, it does not seem to cover the concurrent
expiry case.

>  			x_cur[nx_cur] = x;
>  			nx_cur++;
>  			mp->encap = encap;

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