[PATCH v2] xfrm: fix suspicious RCU usage in xfrm_nlmsg_multicast

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: [email protected]
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 <[email protected]>
---
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