Skip to contentSkip to conversation

xfrm: hold net_device reference under RCU in bundle creation

PATCH Superseded Cen Zhang (Microsoft) 2 participants 3 messages Aug 20 – Aug 26

Subsystems (3)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 xfrm: hold net_device reference under RCU in bundle creation

xfrm_bundle_create() and xfrm_create_dummy_bundle() read dst->dev into a local pointer without taking a device reference, then pass it to xfrm_fill_dst(). A concurrent RTM_DELLINK replaces dst->dev via dst_dev_put() and frees the old net_device, causing a use-after-free when xfrm6_fill_dst() later dereferences the stale dev pointer.

  BUG: KASAN: slab-use-after-free in xfrm6_fill_dst+0x82c/0x860
  (net/ipv6/xfrm6_policy.c:86 netdev_hold())
  Read of size 8 at addr ffff8880142fe588 by task exploit/153
  Call Trace:
   xfrm6_fill_dst+0x82c/0x860
   xfrm_resolve_and_create_bundle+0x21d4/0x2bd0
   xfrm_lookup_with_ifid+0x485/0x1640
   ip6_dst_lookup_flow+0x19b/0x1e0
   udpv6_sendmsg+0x1443/0x2dd0

Fix by reading dst->dev under rcu_read_lock() via dst_dev_rcu() and taking dev_hold() before leaving the RCU critical section. To properly release the reference, add dev_put() on both the success and error paths, and initialize dev to NULL to prevent dev_put() on an uninitialized pointer when put_states falls through to free_dst before dev is assigned.

Fixes 25ee3286dcbc ("[IPSEC]: Merge common code into xfrm_bundle_create")
Fixes a0073fe18e71 ("xfrm: Add a state resolution packet queue")
Reported-by [email protected]
---
 net/xfrm/xfrm_policy.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)
net/xfrm/xfrm_policy.c +15 -3
index 932a313b9460..13585ff24aba 100644
@@ -2668,7 +2668,7 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
 	const struct xfrm_mode *inner_mode;
 	struct net *net = xp_net(policy);
 	unsigned long now = jiffies;
-	struct net_device *dev;
+	struct net_device *dev = NULL;
 	struct xfrm_dst *xdst_prev = NULL;
 	struct xfrm_dst *xdst0 = NULL;
 	int i = 0;
@@ -2770,7 +2770,11 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
 	xdst0->path = dst;
 
 	err = -ENODEV;
-	dev = dst->dev;
+	rcu_read_lock();
+	dev = dst_dev_rcu(dst);
+	if (dev)
+		dev_hold(dev);
+	rcu_read_unlock();
 	if (!dev)
 		goto free_dst;
 
@@ -2789,6 +2793,7 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
 		trailer_len -= xdst_prev->u.dst.xfrm->props.trailer_len;
 	}
 
+	dev_put(dev);
 	return &xdst0->u.dst;
 
 put_states:
@@ -2798,6 +2803,8 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
 	if (xdst0)
 		dst_release_immediate(&xdst0->u.dst);
 
+	if (dev)
+		dev_put(dev);
 	return ERR_PTR(err);
 }
 
@@ -3058,11 +3065,16 @@ static struct xfrm_dst *xfrm_create_dummy_bundle(struct net *net,
 	xfrm_init_path((struct xfrm_dst *)dst1, dst, 0);
 
 	err = -ENODEV;
-	dev = dst->dev;
+	rcu_read_lock();
+	dev = dst_dev_rcu(dst);
+	if (dev)
+		dev_hold(dev);
+	rcu_read_unlock();
 	if (!dev)
 		goto free_dst;
 
 	err = xfrm_fill_dst(xdst, dev, fl);
+	dev_put(dev);
 	if (err)
 		goto free_dst;
 
Signature · 1 line
2.55.0
maintainer
#1 lore raw
Quoted 47 lines · Cen Zhang (Microsoft)

xfrm_bundle_create() and xfrm_create_dummy_bundle() read dst->dev into a local pointer without taking a device reference, then pass it to xfrm_fill_dst(). A concurrent RTM_DELLINK replaces dst->dev via dst_dev_put() and frees the old net_device, causing a use-after-free when xfrm6_fill_dst() later dereferences the stale dev pointer.

  BUG: KASAN: slab-use-after-free in xfrm6_fill_dst+0x82c/0x860
  (net/ipv6/xfrm6_policy.c:86 netdev_hold())
  Read of size 8 at addr ffff8880142fe588 by task exploit/153
  Call Trace:
   xfrm6_fill_dst+0x82c/0x860
   xfrm_resolve_and_create_bundle+0x21d4/0x2bd0
   xfrm_lookup_with_ifid+0x485/0x1640
   ip6_dst_lookup_flow+0x19b/0x1e0
   udpv6_sendmsg+0x1443/0x2dd0

Fix by reading dst->dev under rcu_read_lock() via dst_dev_rcu() and taking dev_hold() before leaving the RCU critical section. To properly release the reference, add dev_put() on both the success and error paths, and initialize dev to NULL to prevent dev_put() on an uninitialized pointer when put_states falls through to free_dst before dev is assigned.

Fixes 25ee3286dcbc ("[IPSEC]: Merge common code into xfrm_bundle_create")
Fixes a0073fe18e71 ("xfrm: Add a state resolution packet queue")
Reported-by [email protected]
---
 net/xfrm/xfrm_policy.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)
net/xfrm/xfrm_policy.c +1 -1
index 932a313b9460..13585ff24aba 100644
@@ -2668,7 +2668,7 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
 	const struct xfrm_mode *inner_mode;
 	struct net *net = xp_net(policy);
 	unsigned long now = jiffies;
-	struct net_device *dev;
+	struct net_device *dev = NULL;
 	struct xfrm_dst *xdst_prev = NULL;
 	struct xfrm_dst *xdst0 = NULL;
 	int i = 0;

net/xfrm/xfrm_policy.c static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy, · line 2777

Quoted 9 lines
net/xfrm/xfrm_policy.c +5 -1
@@ -2770,7 +2770,11 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
 	xdst0->path = dst;
 
 	err = -ENODEV;
-	dev = dst->dev;
+	rcu_read_lock();
+	dev = dst_dev_rcu(dst);
+	if (dev)
+		dev_hold(dev);
+	rcu_read_unlock();

Can't we move the rcu_read_unlock() to the end of the function and avoid the additional refcount?

Quoted 21 lines
net/xfrm/xfrm_policy.c +3 -0
 	if (!dev)
 		goto free_dst;
 
@@ -2789,6 +2793,7 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
 		trailer_len -= xdst_prev->u.dst.xfrm->props.trailer_len;
 	}
 
+	dev_put(dev);
 	return &xdst0->u.dst;
 
 put_states:
@@ -2798,6 +2803,8 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
 	if (xdst0)
 		dst_release_immediate(&xdst0->u.dst);
 
+	if (dev)
+		dev_put(dev);
 	return ERR_PTR(err);
 }
 

net/xfrm/xfrm_policy.c static struct xfrm_dst *xfrm_create_dummy_bundle(struct net *net, · line 3072

Quoted 9 lines
net/xfrm/xfrm_policy.c +5 -1
@@ -3058,11 +3065,16 @@ static struct xfrm_dst *xfrm_create_dummy_bundle(struct net *net,
 	xfrm_init_path((struct xfrm_dst *)dst1, dst, 0);
 
 	err = -ENODEV;
-	dev = dst->dev;
+	rcu_read_lock();
+	dev = dst_dev_rcu(dst);
+	if (dev)
+		dev_hold(dev);
+	rcu_read_unlock();

Same here.

#2 lore raw
Quoted 4 lines · Steffen Klassert

Can't we move the rcu_read_unlock() to the end of the function and avoid the additional refcount?

Same here.

Thanks for pointing out, will resend as v2 soon.

Cen