[PATCH] xfrm: fix suspicious RCU usage in xfrm_nlmsg_multicast

[PATCH] xfrm: fix suspicious RCU usage in xfrm_nlmsg_multicast

[Ömer Mete Kaya]

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
Signed-off-by: Ömer Mete Kaya <omermetekaya0@gmail.com>
---
This patch intentionally centralizes the RCU read-side critical
section in xfrm_nlmsg_multicast() rather than adding
rcu_read_lock()/rcu_read_unlock() to each of the 11 affected call
sites, since every caller has the same requirement.

If keeping the locking at each call site better matches the
subsystem's conventions, I'm happy to rework the patch accordingly.

Reproduced and verified on a locally built upstream kernel
(v7.2.0-rc5) under QEMU/KVM, using the syzbot-provided .config and
C reproducer: the warning triggered on every run before this patch,
and did not trigger over 30+ runs after. xfrm_notify_userpolicy()
(exercised via "ip xfrm policy getdefault") was also verified to
still work correctly after removing its now-redundant RCU lock pair.

 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