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

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

[Jun Yang]

From: Jun Yang <junvyyang@tencent.com>

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.

xfrm_migrate() then stores the same state more than once in x_cur[].
After the lookup references are dropped, the first delete can release
the table's last reference. The garbage collector can free the state
before a subsequent delete accesses it.

Reject a state already present in x_cur[] before cloning it again. Drop
the extra lookup reference and use the existing rollback path to remove
any clones installed for earlier entries.

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 | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 9ea60c7717ff..472f65616e28 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -4669,7 +4669,7 @@ int xfrm_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
 		 struct xfrm_encap_tmpl *encap, u32 if_id,
 		 struct netlink_ext_ack *extack, struct xfrm_user_offload *xuo)
 {
-	int i, err, nx_cur = 0, nx_new = 0;
+	int i, j, err, nx_cur = 0, nx_new = 0;
 	struct xfrm_policy *pol = NULL;
 	struct xfrm_state *x, *xc;
 	struct xfrm_state *x_cur[XFRM_MAX_DEPTH];
@@ -4698,6 +4698,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;
+			}
 			x_cur[nx_cur] = x;
 			nx_cur++;
 			mp->encap = encap;
-- 
2.43.7

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

[netdev-bot+sashiko]

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