Message-ID: <20250708230513.42922-1-nkapron@google.com>
Date: Tue,  8 Jul 2025 23:04:52 +0000
From: Neill Kapron <nkapron@...gle.com>
To: netdev@...r.kernel.org
Cc: nkapron@...gle.com, kernel-team@...roid.com, cmllamas@...gle.com, 
	jstultz@...gle.com, Nick Hawkins <nick.hawkins@....com>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, 
	Kuniyuki Iwashima <kuniyu@...gle.com>, Al Viro <viro@...iv.linux.org.uk>, 
	Dmitry Safonov <0x7f454c46@...il.com>, Anastasia Kovaleva <a.kovaleva@...ro.com>, 
	Jinjie Ruan <ruanjinjie@...wei.com>, Siddh Raman Pant <siddh.raman.pant@...cle.com>, 
	linux-kernel@...r.kernel.org
Subject: [RFC net] netlink: lock nl_cb_mutex in netlink_release

While investigating the following bug report on a 6.6 kernel (similar to
[1]), I believe I have identified a race condition between
__netlink_dump_start(), netlink_release(), and netlink_recvmsg() (which
calls netlink_dump()).

My understanding is that __netlink_dump_start() locks nl_cb_mutex, sets
cb_running and calls netlink_dump(), but this call into netlink_dump()
unlocks nl_cb_mutex and returns before clearing cb_running. Then,
netlink_recvmsg() is called, which calls back into netlink_dump(), which
then locks nl_cb_mutex, checks to make sure cb_running is set, then
proceeds with the dump.

It is at this point after the netlink_dump() checks for cb_running that
netlink_release() is called, which tears everything down. While the change
in [2] clears cb_running, it does so without holding the lock. This causes
the NULL pointer dereference in netlink_dump().

I think this should be resolved by locking the
nl_cb_mutex in netlink_release() as seen in the change below.

By locking this mutex early in netlink_release(), then performing the
cleanup and setting nlk->cb_running to false prior to releasing the
lock, netlink_dump() should be blocked waiting for the lock. Once
netlink_dump() acquires the lock, it will fail the check for
nlk->cb_running, and therefore jump to the errout_skb label. Since *skb
is NULL at this point, the call to kfree_skb() should be harmless.

Additionally, I believe [2] should be backported to stable trees, as it
does not exist on 6.6 where this bug was seen. It should be noted that
the fixes tag was removed for this patch per the request in [3], as it
was considered cleanup since 'code can't be run once we're in release',
however in both [1] and the trace collected below, the USB NCM gadget is
running, and I am suspicious that a cable unplug event asyncronously
causes netlink_release to be called during the netlink_dump(). I will send
a separate request to stable for this to be backported.

Note, I was unable to reproduce this bug, and therefore don't have a
good way to test this patch, but it seems to be a likely solution. That
being said, I do not have extensive background with netlink, and may
have overlooked something. Thanks for the thorough review.

Abbreviated trace below:
_______________________________________________________________________

Unable to handle kernel NULL pointer dereference at virtual address 00
...
 pstate: 63400005 (nZCv daif +PAN -UAO +TCO +DIT -SSBS BTYPE=--)
 pc : __pi_strlen (arch/arm64/lib/strlen.S:87)
 lr : nla_put_string (include/net/netlink.h:1523)
 sp : ffffffc09e59b7d0
 x29: ffffffc09e59b7d0 x28: 0000000000000043 x27: ffffff8976deb000
 x26: 00000000c71085df x25: 0000000000000010 x24: 00000000ffffffff
 x23: 0000000000000000 x22: 0000000000000000 x21: ffffff89dfb97300
 x20: 0000000000000038 x19: 0000000000000000 x18: ffffffd1032dc5c0
 x17: 00000000d6618229 x16: 0000000000000018 x15: 0000000000000000
 x14: 0000000000000000 x13: 0000000000000000 x12: 00000000000029b8
 x11: 0000000000003e80 x10: 0000000000000b38 x9 : 000000000a370000
 x8 : 0101010101010101 x7 : 0000000000000000 x6 : 0000000000000000
 x5 : ffffff800a3729b8 x4 : 0000000000000000 x3 : 0000000000000000
 x2 : 0000000000000000 x1 : 0000000000000038 x0 : 0000000000000000
Call trace:
 __pi_strlen (arch/arm64/lib/strlen.S:87)
 rtnl_fill_ifinfo (net/core/rtnetlink.c:1935)
 rtnl_dump_ifinfo (net/core/rtnetlink.c:2276)
 netlink_dump (net/netlink/af_netlink.c:2257)
 netlink_recvmsg (net/netlink/af_netlink.c:1978)
 __sys_recvfrom (net/socket.c:1047 net/socket.c:1069 net/socket.c:2256)
 __arm64_sys_recvfrom (net/socket.c:2274 net/socket.c:2270 net/socket.c:2270)
 invoke_syscall (arch/arm64/kernel/syscall.c:0 arch/arm64/kernel/syscall.c:51)
 el0_svc_common (arch/arm64/kernel/syscall.c:0)
 do_el0_svc (arch/arm64/kernel/syscall.c:154)
 el0_svc (arch/arm64/kernel/entry-common.c:136
  arch/arm64/kernel/entry-common.c:147 arch/arm64/kernel/entry-common.c:684)
 el0t_64_sync_handler (arch/arm64/kernel/entry-common.c:0)
 el0t_64_sync (arch/arm64/kernel/entry.S:599)
 Code: 92400c04 b200c3e8 f100009f 5400088c (a9400c02)
_______________________________________________________________________

[1] https://lore.kernel.org/all/bug-219826-208809@https.bugzilla.kernel.org%2F/
[2] https://lore.kernel.org/all/aff028e3eb2b768b9895fa6349fa1981ae22f098.camel@oracle.com/
[3] https://lore.kernel.org/all/20250214170631.6badcc24@kernel.org/

Reported-by: Nick Hawkins <nick.hawkins@....com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219826
Fixes: 438989137acd ("netlink: Unset cb_running when terminating dump on release")
Fixes: 1904fb9ebf91 ("netlink: terminate outstanding dump on socket close")
Signed-off-by: Neill Kapron <nkapron@...gle.com>
---
 net/netlink/af_netlink.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 79fbaf7333ce..142529c08300 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -724,9 +724,11 @@ static int netlink_release(struct socket *sock)
 	if (!sk)
 		return 0;
 
+	nlk = nlk_sk(sk);
+	mutex_lock(&nlk->nl_cb_mutex);
+
 	netlink_remove(sk);
 	sock_orphan(sk);
-	nlk = nlk_sk(sk);
 
 	/*
 	 * OK. Socket is unlinked, any packets that arrive now
@@ -773,6 +775,7 @@ static int netlink_release(struct socket *sock)
 		WRITE_ONCE(nlk->cb_running, false);
 	}
 
+	mutex_unlock(&nlk->nl_cb_mutex);
 	module_put(nlk->module);
 
 	if (netlink_is_kernel(sk)) {
-- 
2.50.0.727.gbf7dc18ff4-goog

Powered by blists - more mailing lists