xfrm: hold net_device reference under RCU in bundle creation
Subsystems (3)
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.
--- net/xfrm/xfrm_policy.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)
net/xfrm/xfrm_policy.c +15 -3
@@ -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
Steffen Klassert
maintainerQuoted 47 lines · Cen Zhang (Microsoft)
xfrm_bundle_create()andxfrm_create_dummy_bundle()readdst->devinto a local pointer without taking a device reference, then pass it toxfrm_fill_dst(). A concurrentRTM_DELLINKreplacesdst->devviadst_dev_put()and frees the oldnet_device, causing a use-after-free whenxfrm6_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/0x2dd0Fix by reading
dst->devunderrcu_read_lock()viadst_dev_rcu()and takingdev_hold()before leaving the RCU critical section. To properly release the reference, adddev_put()on both the success and error paths, and initialize dev to NULL to preventdev_put()on an uninitialized pointer whenput_statesfalls through tofree_dstbefore 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]Reported-by Xiang Mei (Microsoft) <[email protected]>Reported-by Cen Zhang (Microsoft) <[email protected]>Signed-off-by Cen Zhang (Microsoft) <[email protected]>--- net/xfrm/xfrm_policy.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)net/xfrm/xfrm_policy.c +1
-1@@ -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
-0if (!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.
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