[PATCH ipsec v2 0/2] xfrm: fix offload state use racing with device teardown

[PATCH ipsec v2 0/2] xfrm: fix offload state use racing with device teardown

[Cen Zhang]

NETDEV_DOWN and NETDEV_UNREGISTER flush a device's offloaded SAs with
xfrm_dev_state_flush(). It deletes each SA under x->lock and then frees
its offload binding with xfrm_dev_state_free() without that lock: the
driver frees its offload state, xso.dev is cleared and the device
reference is dropped. Netlink requests that hold only a reference to
the SA can still be using the binding after they checked xso.dev:

  1/2 The offload statistics refresh in GETSA, ALLOCSPI replies and SA
      notifications, and in the expiry timer, calls into the driver.
      Refresh the statistics only of SAs that are not dead, under
      x->lock, which orders the refresh with the teardown.

  2/2 XFRM_MSG_MIGRATE_STATE reads the ifindex through xso.dev. Use the
      ifindex cached at attachment, as state dumps already do.

Cen Zhang (2):
  xfrm: protect offload statistics updates from device teardown
  xfrm: use cached offload ifindex when migrating an SA

 net/xfrm/xfrm_state.c |  4 ++--
 net/xfrm/xfrm_user.c  | 14 +++++++++++---
 2 files changed, 13 insertions(+), 5 deletions(-)

---
Changes in v2:
- Address the incomplete RCU protection raised by Sashiko:
  https://sashiko.dev/#/patchset/97864
- Patch 1: refresh the statistics under x->lock and only for SAs that are
  not dead, instead of using RCU, which covered neither the driver offload
  state nor the cleared xso.dev; fix the same race in the expiry timer.
- Add patch 2 for the same race in xfrm_do_migrate_state(), also raised by
  Sashiko.
- Trim the crash reports.

Link to v1: https://lore.kernel.org/r/pm-xfrm-core-objects-candidate-0050-v3-dbfb7194ffb750193d02@gmail.com

[PATCH ipsec v2 1/2] xfrm: protect offload statistics updates from device teardown

[Cen Zhang]

copy_to_user_state() refreshes the statistics of an offloaded SA through
the driver's xdo_dev_state_update_stats() callback. GETSA and SA
notifications run it with only a reference to the xfrm_state, which does
not keep the offload binding alive, and the netlink configuration mutex
does not serialize them with the netdevice notifier.

NETDEV_DOWN and NETDEV_UNREGISTER flush the device's states with
xfrm_dev_state_flush(). It deletes each state under x->lock and then
calls xfrm_dev_state_free() without that lock, which lets the driver
free its offload state, clears xso.dev and drops the device reference.
A refresh that has already seen xso.dev set can then run the driver
callback on freed offload state or after xso.dev became NULL, and can
read the net_device after unregistration has freed it:

  GETSA                              Device teardown (RTNL)
  -----                              ----------------------
  look up and reference x
  copy_to_user_state(x)
    x->xso.dev is set
                                     xfrm_dev_state_flush()
                                       xfrm_state_delete(x)
                                       xfrm_dev_state_free(x)
                                         xdo_dev_state_free()
                                         clear x->xso.dev
                                         netdev_put(dev)
                                     unregister frees dev
    xfrm_dev_state_update_stats(x)
      reads dev->xfrmdev_ops

The expiry timer has the same problem. It refreshes the statistics
before it checks for a dead state, and it can still fire after the
flush has deleted the state.

Every path that frees the offload state of a reachable SA marks the SA
dead under x->lock first. Refresh the statistics only for SAs that are
not dead, with x->lock held, so a refresh either completes before the
teardown or sees the dead state. GETSA, ALLOCSPI replies and SA
notifications now take x->lock around the copy. Expire notifications
already run under x->lock. Dumps run under xfrm_state_lock while the SA
is still linked, and deletion needs that lock before the offload state
can be freed. The timer now checks for a dead state before the refresh.
A dead SA keeps the counters from its last refresh.

The report below was collected on an earlier kernel with a built-in test
device that supplies no-op offload add and delete callbacks. Scheduling
probes paused GETSA after it sampled the device, then ran normal
link-down and link-delete requests before resuming the read.

KASAN report as below:

    ==================================================================
    BUG: KASAN: slab-use-after-free in copy_to_user_state+0x6ce/0x6e0
    Read of size 8 at addr ffff888104be0308 by task ip/446

    Call Trace:
     <TASK>
     dump_stack_lvl+0x93/0xd0
     print_report+0xce/0x630
     kasan_report+0xe0/0x110
     copy_to_user_state+0x6ce/0x6e0
     copy_to_user_state_extra+0x8a/0x1450
     dump_one_state+0x1fd/0x440
     xfrm_state_netlink+0xda/0x150
     xfrm_get_sa+0x12d/0x320
     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
     </TASK>

    Allocated by task 420:
     kasan_save_stack+0x33/0x60
     kasan_save_track+0x14/0x30
     __kasan_kmalloc+0xaa/0xb0
     __kvmalloc_node_noprof+0x353/0x930
     alloc_netdev_mqs+0xa0/0x1250
     rtnl_create_link+0xb8e/0xeb0
     rtnl_newlink+0xb7b/0x1f40
     rtnetlink_rcv_msg+0x7b9/0xce0
     netlink_rcv_skb+0x133/0x390
     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 451:
     kasan_save_stack+0x33/0x60
     kasan_save_track+0x14/0x30
     kasan_save_free_info+0x3b/0x60
     __kasan_slab_free+0x5f/0x80
     kfree+0x307/0x580
     device_release+0xd7/0x280
     kobject_put+0x1ad/0x610
     netdev_run_todo+0x927/0x13d0
     rtnl_dellink+0x3aa/0xa30
     rtnetlink_rcv_msg+0x7b9/0xce0
     netlink_rcv_skb+0x133/0x390
     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

    The buggy address belongs to the object at ffff888104be0000
     which belongs to the cache kmalloc-4k of size 4096
    The buggy address is located 776 bytes inside of
     freed 4096-byte region [ffff888104be0000, ffff888104be1000)
    ==================================================================

Fixes: 07b87f9eea0c ("xfrm: Fix unregister netdevice hang on hardware offload.")
Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
---
Assisted-by: LLM

 net/xfrm/xfrm_state.c |  4 ++--
 net/xfrm/xfrm_user.c  | 10 +++++++++-
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 0555e5d796ed..1aeb6208ea8f 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -649,10 +649,10 @@ static enum hrtimer_restart xfrm_timer_handler(struct hrtimer *me)
 	int err = 0;
 
 	spin_lock(&x->lock);
-	xfrm_dev_state_update_stats(x);
-
 	if (x->km.state == XFRM_STATE_DEAD)
 		goto out;
+
+	xfrm_dev_state_update_stats(x);
 	if (x->km.state == XFRM_STATE_EXPIRED)
 		goto expired;
 	if (x->lft.hard_add_expires_seconds) {
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index a5fbe38839c4..781d4ec565aa 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1179,7 +1179,11 @@ static void copy_to_user_state(struct xfrm_state *x, struct xfrm_usersa_info *p)
 	memcpy(&p->id, &x->id, sizeof(p->id));
 	memcpy(&p->sel, &x->sel, sizeof(p->sel));
 	memcpy(&p->lft, &x->lft, sizeof(p->lft));
-	if (x->xso.dev)
+	/* Device teardown marks the state dead under x->lock before it frees
+	 * the offload state. Callers hold x->lock, or xfrm_state_lock while
+	 * the state is still linked.
+	 */
+	if (x->xso.dev && x->km.state != XFRM_STATE_DEAD)
 		xfrm_dev_state_update_stats(x);
 	memcpy(&p->curlft, &x->curlft, sizeof(p->curlft));
 	put_unaligned(x->stats.replay_window, &p->stats.replay_window);
@@ -1633,7 +1637,9 @@ static struct sk_buff *xfrm_state_netlink(struct sk_buff *in_skb,
 	info.nlmsg_seq = seq;
 	info.nlmsg_flags = 0;
 
+	spin_lock_bh(&x->lock);
 	err = dump_one_state(x, 0, &info);
+	spin_unlock_bh(&x->lock);
 	if (err) {
 		kfree_skb(skb);
 		return ERR_PTR(err);
@@ -4167,7 +4173,9 @@ static int xfrm_notify_sa(struct xfrm_state *x, const struct km_event *c)
 
 		p = nla_data(attr);
 	}
+	spin_lock_bh(&x->lock);
 	err = copy_to_user_state_extra(x, p, skb);
+	spin_unlock_bh(&x->lock);
 	if (err)
 		goto out_free_skb;
 

[PATCH ipsec v2 2/2] xfrm: use cached offload ifindex when migrating an SA

[Cen Zhang]

An xfrm_state reference keeps its offload record alive, but does not
prevent xfrm_dev_state_free() from clearing xso.dev and dropping the
device reference. xfrm_do_migrate_state() checks xso.dev before
xso_to_xuo() dereferences it to obtain the offload ifindex.

With XFRM offload and migration enabled, XFRM_MSG_MIGRATE_STATE inherits
the old SA's offload configuration if the request omits
XFRMA_OFFLOAD_DEV and does not set XFRM_MIGRATE_STATE_CLEAR_OFFLOAD.
Device unregister can flush that SA between the check and the read.
The supplied report used a scheduling probe to delay migration in this
interval while in-tree netdevsim was removed through sysfs.

    Migration (xfrm_cfg_mutex)       Device unregister (RTNL)
    xfrm_state_lookup(): hold x
    test x->xso.dev != NULL
                                    xfrm_dev_state_flush()
                                      xfrm_state_delete(x)
                                      xfrm_dev_state_free(x)
                                        xso->dev = NULL
                                        netdev_put(dev)
    xso_to_xuo(&x->xso, &xuo)
      xso->dev->ifindex: NULL deref

The netlink configuration mutex does not serialize with the notifier,
and migration takes the state lock only after this read. The result is
a NULL pointer dereference even though migration still holds the SA.

Use the ifindex cached in the offload record at attachment, as state
dumps already do. The SA reference protects this snapshot throughout
the read, so serialization no longer depends on the device lifetime.
Annotate the binding check with READ_ONCE() to match the concurrent
WRITE_ONCE() in xfrm_dev_state_free().

Oops report as below:

    Oops: general protection fault, probably for non-canonical address
    0xdffffc000000001c: 0000 [#1] SMP KASAN NOPTI
    KASAN: null-ptr-deref in range [0x00000000000000e0-0x00000000000000e7]
    RIP: 0010:xfrm_do_migrate_state+0x1f06/0x2c50
    Call Trace:
     <TASK>
     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>

Fixes: a9d155ea9b44 ("xfrm: add XFRM_MSG_MIGRATE_STATE for single SA migration")
Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
---
Assisted-by: LLM

 net/xfrm/xfrm_user.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 781d4ec565aa..1cea24380797 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1240,7 +1240,7 @@ static void xso_to_xuo_ifindex(const struct xfrm_dev_offload *xso, int ifindex,
 static void xso_to_xuo(const struct xfrm_dev_offload *xso,
 		       struct xfrm_user_offload *xuo)
 {
-	xso_to_xuo_ifindex(xso, xso->dev->ifindex, xuo);
+	xso_to_xuo_ifindex(xso, READ_ONCE(xso->ifindex), xuo);
 }
 #endif
 
@@ -3462,7 +3462,7 @@ static int xfrm_do_migrate_state(struct sk_buff *skb, struct nlmsghdr *nlh,
 	} else {
 		bool inherit_offload = !(um->flags & XFRM_MIGRATE_STATE_CLEAR_OFFLOAD);
 
-		if (inherit_offload && x->xso.dev) {
+		if (inherit_offload && READ_ONCE(x->xso.dev)) {
 			xso_to_xuo(&x->xso, &xuo);
 			m.xuo = &xuo;
 		}