xfrm_nlmsg_multicast() dereferences net->xfrm.nlsk via rcu_dereference() and is documented as requiring the RCU read lock, but 11 of its 12 call sites in this file do not hold it. Move the RCU read-side critical section inside xfrm_nlmsg_multicast() itself instead of adding it to each call site individually. This is safe: xfrm_get_translator() takes its own nested RCU read lock internally, and nlmsg_multicast() is called with GFP_ATOMIC, whose only conditional yield() in netlink_broadcast_filtered() is gated on blocking being allowed, which GFP_ATOMIC never permits. The redundant rcu_read_lock()/rcu_read_unlock() pair in xfrm_notify_userpolicy(), the one caller that already took the lock, is removed accordingly. Reported-by: syzbot+d3bc2f2eb498a0175940@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=d3bc2f2eb498a0175940 Fixes: 21ee543edc0d ("xfrm: fix race between netns cleanup and state expire notification") Signed-off-by: Ömer Mete Kaya --- v2: Added Fixes: tag pointing to the commit that introduced xfrm_nlmsg_multicast() and its RCU-locking contract, as requested by Steffen. net/xfrm/xfrm_user.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c index d6db63304..1f683516f 100644 --- a/net/xfrm/xfrm_user.c +++ b/net/xfrm/xfrm_user.c @@ -1622,31 +1622,35 @@ static struct sk_buff *xfrm_state_netlink(struct sk_buff *in_skb, } /* A wrapper for nlmsg_multicast() checking that nlsk is still available. - * Must be called with RCU read lock. + * Takes the RCU read lock internally around the multicast. */ static inline int xfrm_nlmsg_multicast(struct net *net, struct sk_buff *skb, - u32 pid, unsigned int group) + u32 pid, unsigned int group) { - struct sock *nlsk = rcu_dereference(net->xfrm.nlsk); + struct sock *nlsk; struct xfrm_translator *xtr; + int err; + rcu_read_lock(); + nlsk = rcu_dereference(net->xfrm.nlsk); if (!nlsk) { + rcu_read_unlock(); kfree_skb(skb); return -EPIPE; } - xtr = xfrm_get_translator(); if (xtr) { - int err = xtr->alloc_compat(skb, nlmsg_hdr(skb)); - + err = xtr->alloc_compat(skb, nlmsg_hdr(skb)); xfrm_put_translator(xtr); if (err) { + rcu_read_unlock(); kfree_skb(skb); return err; } } - - return nlmsg_multicast(nlsk, skb, pid, group, GFP_ATOMIC); + err = nlmsg_multicast(nlsk, skb, pid, group, GFP_ATOMIC); + rcu_read_unlock(); + return err; } static inline unsigned int xfrm_spdinfo_msgsize(void) @@ -2536,9 +2540,7 @@ static int xfrm_notify_userpolicy(struct net *net) nlmsg_end(skb, nlh); - rcu_read_lock(); err = xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_POLICY); - rcu_read_unlock(); return err; } -- 2.55.0