[PATCH ipsec 0/2] xfrm: fix state lifetimes in xfrm_migrate()

[PATCH ipsec 0/2] xfrm: fix state lifetimes in xfrm_migrate()

[Cen Zhang]

xfrm_migrate() keeps the old states in x_cur[] and their successors in
x_new[], but it does not hold a reference to every state it later
deletes. An SA with a hard lifetime can expire and be freed by the timer
and the state GC in that window, and the following xfrm_state_delete()
takes the lock of freed memory:

  1/2 The success path drops the lookup references of the old states
      before it deletes them. Delete them first.

  2/2 xfrm_state_migrate() returns the published successor without a
      caller reference, so the rollback deletes states it does not
      hold. Take that reference before publishing and drop it after the
      migration commits or rolls back.

Both windows date back to the initial migration support and are reached
by XFRM_MSG_MIGRATE and PF_KEY's SADB_X_MIGRATE.

Cen Zhang (2):
  xfrm: keep old state references until migration deletion completes
  xfrm: hold migrated states through rollback

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

---

[PATCH ipsec 1/2] xfrm: keep old state references until migration deletion completes

[Cen Zhang]

xfrm_migrate() must retain the references returned by
xfrm_migrate_state_find() until it finishes using the old states. The
successful migration path instead calls xfrm_states_put() before
xfrm_states_delete(), leaving the latter to use unreferenced pointers
from x_cur[].

With CONFIG_XFRM_MIGRATE enabled, a privileged XFRM_MSG_MIGRATE or
SADB_X_MIGRATE request can reach this path for an SA with a finite hard
lifetime and no other users. If migration is preempted after the put,
the old SA can expire and be reclaimed before deletion resumes:

  Migration task                 Hard-expiry timer / GC worker
  --------------                 -----------------------------
  xfrm_migrate_state_find()
    hold old SA in x_cur[]
  migrate state and policy
  xfrm_states_put(x_cur, ...)
    drop lookup reference
                                 xfrm_timer_handler()
                                   __xfrm_state_delete(old SA)
                                     drop SAD reference
                                 xfrm_state_gc_task()
                                   synchronize_rcu()
                                   free old SA
  xfrm_states_delete(x_cur, ...)
    xfrm_state_delete(old SA)
      spin_lock_bh(&x->lock) /* use after free */

The configuration mutex does not serialize migration with the timer or
GC worker, and migration holds no RCU read lock across this interval.
xfrm_state_delete() accesses the freed lock before it can check whether
the state is already dead. Expiry before the migration put can open the
same window, with that put dropping the last reference.

Delete the old states before putting their lookup references. Each state
then remains allocated through the lock access and the dead-state check,
including when expiry has already removed it from the SAD.

KASAN report as below:

    ==================================================================
    BUG: KASAN: slab-use-after-free in _raw_spin_lock_bh+0x38/0x50
    Read of size 1 at addr ffff88810967af48 by task python3/443

    Call Trace:
     <TASK>
     dump_stack_lvl+0x93/0xd0
     print_report+0xce/0x630
     kasan_report+0xe0/0x110
     __kasan_check_byte+0x36/0x50
     lock_acquire+0x113/0x2f0
     _raw_spin_lock_bh+0x38/0x50
     xfrm_state_delete+0x26/0x60
     xfrm_migrate+0xd26/0x1950
     xfrm_do_migrate+0x797/0xad0
     xfrm_user_rcv_msg+0x3b3/0xc20
     netlink_rcv_skb+0x133/0x390
     xfrm_netlink_rcv+0x76/0x90
     netlink_unicast+0x504/0x840
     netlink_sendmsg+0x7f7/0xcf0
     __sys_sendto+0x425/0x470
     __x64_sys_sendto+0xe5/0x1c0
     do_syscall_64+0x115/0x6a0
     entry_SYSCALL_64_after_hwframe+0x77/0x7f
     </TASK>

    Allocated by task 424:
     kasan_save_stack+0x33/0x60
     kasan_save_track+0x14/0x30
     __kasan_slab_alloc+0x89/0x90
     kmem_cache_alloc_noprof+0x20b/0x5f0
     xfrm_state_alloc+0x28/0x5d0
     xfrm_add_sa+0xb81/0x5660
     xfrm_user_rcv_msg+0x3b3/0xc20
     netlink_rcv_skb+0x133/0x390
     xfrm_netlink_rcv+0x76/0x90
     netlink_unicast+0x504/0x840
     netlink_sendmsg+0x7f7/0xcf0
     ____sys_sendmsg+0x88b/0x9d0
     ___sys_sendmsg+0x125/0x1d0
     __sys_sendmsg+0x13e/0x1e0
     do_syscall_64+0x115/0x6a0
     entry_SYSCALL_64_after_hwframe+0x77/0x7f

    Freed by task 66:
     kasan_save_stack+0x33/0x60
     kasan_save_track+0x14/0x30
     kasan_save_free_info+0x3b/0x60
     __kasan_slab_free+0x5f/0x80
     kmem_cache_free+0x242/0x580
     xfrm_state_gc_task+0x534/0x780
     process_one_work+0x908/0x19c0
     worker_thread+0x65c/0xe40
     kthread+0x34f/0x460
     ret_from_fork+0x659/0x940
     ret_from_fork_asm+0x1a/0x30

    The buggy address belongs to the object at ffff88810967aec0
     which belongs to the cache xfrm_state of size 936
    The buggy address is located 136 bytes inside of
     freed 936-byte region [ffff88810967aec0, ffff88810967b268)
    ==================================================================

Fixes: 80c9abaabf42 ("[XFRM]: Extension for dynamic update of endpoint address(es)")
Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
---
Assisted-by: LLM

 net/xfrm/xfrm_policy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 0f6fcea28bcd..55f6bacc4e51 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);
 	}
 
 	/* Stage 5 - announce */

[PATCH ipsec 2/2] xfrm: hold migrated states through rollback

[Cen Zhang]

xfrm_migrate() needs to keep each new state alive until it either commits
or rolls back the migration. xfrm_state_migrate() publishes the successor
using its initial reference as SAD ownership, then returns it without a
caller reference. The pointer saved in x_new[] can therefore outlive the
state.

A legacy migration can find an existing policy with no matching templates,
install a successor and then fail xfrm_policy_migrate() with -ENODATA. If
the successor inherits a finite hard lifetime and the migration task is
delayed before rollback, expiry and GC can finish first:

  Migration task                  Timer / GC
  xfrm_state_migrate()
    publish xc, arm mtimer
  save xc in x_new[]
  xfrm_policy_migrate()
    return -ENODATA
  <task delayed>                  xfrm_timer_handler(xc)
                                    __xfrm_state_delete(xc)
                                    drop SAD reference
                                  xfrm_state_gc_task()
                                    synchronize_rcu()
                                    free xc
  xfrm_states_delete(x_new)
    xfrm_state_delete(xc)
      spin_lock_bh(&xc->lock)

The control mutex does not serialize the timer or GC, and migration holds
no RCU read lock across this interval. Rollback consequently accesses the
freed state's lock. This also affects the PF_KEY migration entry, which
uses the same helpers.

Take a caller reference before installing the successor, and release it
on installation failure or after the transaction succeeds. On rollback,
delete the successors before dropping their caller references. This keeps
each allocation alive even if its timer has already removed it from the
SAD.

The reported execution used trace probes and paused the netlink migration
before the rollback lock access while the normal timer and GC ran. The
userspace setup installed an SA with a five-second hard lifetime and a
policy without templates using ordinary XFRM requests.

KASAN report as below:

    ==================================================================
    BUG: KASAN: slab-use-after-free in _raw_spin_lock_bh+0x38/0x50
    Read of size 1 at addr ffff8881076d6248 by task python3/417

    Call Trace:
     <TASK>
     dump_stack_lvl+0x93/0xd0
     print_report+0xce/0x630
     kasan_report+0xe0/0x110
     __kasan_check_byte+0x36/0x50
     lock_acquire+0x113/0x2f0
     _raw_spin_lock_bh+0x38/0x50
     xfrm_state_delete+0x26/0x60
     xfrm_migrate+0x11af/0x18a0
     xfrm_do_migrate+0x797/0xad0
     xfrm_user_rcv_msg+0x3b3/0xc20
     netlink_rcv_skb+0x133/0x390
     xfrm_netlink_rcv+0x76/0x90
     netlink_unicast+0x504/0x840
     netlink_sendmsg+0x7f7/0xcf0
     __sys_sendto+0x425/0x470
     __x64_sys_sendto+0xe5/0x1c0
     do_syscall_64+0x115/0x6a0
     entry_SYSCALL_64_after_hwframe+0x77/0x7f
     </TASK>

    Allocated by task 417:
     kasan_save_stack+0x33/0x60
     kasan_save_track+0x14/0x30
     __kasan_slab_alloc+0x89/0x90
     kmem_cache_alloc_noprof+0x20b/0x5f0
     xfrm_state_alloc+0x28/0x5d0
     xfrm_state_migrate_create+0x55/0x2470
     xfrm_state_migrate+0x37/0x450
     xfrm_migrate+0x80b/0x18a0
     xfrm_do_migrate+0x797/0xad0
     xfrm_user_rcv_msg+0x3b3/0xc20
     netlink_rcv_skb+0x133/0x390
     xfrm_netlink_rcv+0x76/0x90
     netlink_unicast+0x504/0x840
     netlink_sendmsg+0x7f7/0xcf0
     __sys_sendto+0x425/0x470
     __x64_sys_sendto+0xe5/0x1c0
     do_syscall_64+0x115/0x6a0
     entry_SYSCALL_64_after_hwframe+0x77/0x7f

    Freed by task 11:
     kasan_save_stack+0x33/0x60
     kasan_save_track+0x14/0x30
     kasan_save_free_info+0x3b/0x60
     __kasan_slab_free+0x5f/0x80
     kmem_cache_free+0x242/0x580
     xfrm_state_gc_task+0x533/0x730
     process_one_work+0x908/0x19c0
     worker_thread+0x65c/0xe40
     kthread+0x34f/0x460
     ret_from_fork+0x659/0x940
     ret_from_fork_asm+0x1a/0x30

    The buggy address belongs to the object at ffff8881076d61c0
     which belongs to the cache xfrm_state of size 936
    The buggy address is located 136 bytes inside of
     freed 936-byte region [ffff8881076d61c0, ffff8881076d6568)
    ==================================================================

Fixes: 80c9abaabf42 ("[XFRM]: Extension for dynamic update of endpoint address(es)")
Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
---
Assisted-by: LLM

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

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 55f6bacc4e51..cced7928e980 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -4733,6 +4733,7 @@ int xfrm_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
 	/* Stage 5 - announce */
 	km_migrate(sel, dir, type, m, num_migrate, k, net, encap);
 
+	xfrm_states_put(x_new, nx_new);
 	xfrm_pol_put(pol);
 
 	return 0;
@@ -4744,8 +4745,10 @@ int xfrm_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
 		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 0555e5d796ed..b9db6e903292 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -2238,8 +2238,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)
+	/* Hold a reference for the caller before publishing the state. */
+	xfrm_state_hold(xc);
+	if (xfrm_state_migrate_install(x, xc, m, extack) < 0) {
+		xfrm_state_put(xc);
 		return NULL;
+	}
 
 	return xc;
 }

Re: [PATCH ipsec 0/2] xfrm: fix state lifetimes in xfrm_migrate()

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.