netdev - Re: [PATCH net] af_unix: Fix uninit-value in __unix_walk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 25 Jun 2024 12:58:48 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <syoshida@...hat.com>
CC: <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
	<kuniyu@...zon.com>, <linux-kernel@...r.kernel.org>,
	<netdev@...r.kernel.org>, <pabeni@...hat.com>, <syzkaller@...glegroups.com>
Subject: Re: [PATCH net] af_unix: Fix uninit-value in __unix_walk_scc()

From: Shigeru Yoshida <syoshida@...hat.com>
Date: Wed, 26 Jun 2024 00:27:13 +0900
> KMSAN reported uninit-value access in __unix_walk_scc() [1].
> 
> In the list_for_each_entry_reverse() loop, when the vertex's index equals
> it's scc_index, the loop uses the variable vertex as a temporary variable
> that points to a vertex in scc. And when the loop is finished, the variable
> vertex points to the list head, in this case scc, which is a local variable
> on the stack.

Thanks for the fix !

More precisely, it's not even scc and might underflow the call
stack of __unix_walk_scc():

  container_of(&scc, struct unix_vertex, scc_entry)


> 
> However, the variable vertex is used under the label prev_vertex. So if the
> edge_stack is not empty and the function jumps to the prev_vertex label,
> the function will access invalid data on the stack. This causes the
> uninit-value access issue.
> 
> Fix this by introducing a new temporary variable for the loop.
> 
> [1]
> BUG: KMSAN: uninit-value in __unix_walk_scc net/unix/garbage.c:478 [inline]
> BUG: KMSAN: uninit-value in unix_walk_scc net/unix/garbage.c:526 [inline]
> BUG: KMSAN: uninit-value in __unix_gc+0x2589/0x3c20 net/unix/garbage.c:584
>  __unix_walk_scc net/unix/garbage.c:478 [inline]

Could you validate the test case below without/with your patch
and post it within v2 with your SOB tag ?

I ran the test below and confrimed the bug with a manual WARN_ON()
but didn't see KMSAN splat, so what version of clang do you use ?

---8<---
From: Kuniyuki Iwashima <kuniyu@...zon.com>
Date: Tue, 25 Jun 2024 19:46:59 +0000
Subject: [PATCH] selftest: af_unix: Add test case for backtrack after
 finalising SCC.

syzkaller reported a KMSAN splat in __unix_walk_scc() while backtracking
edge_stack after finalising SCC.

Let's add a test case exercising the path.

Signed-off-by: Kuniyuki Iwashima <kuniyu@...zon.com>

diff --git a/tools/testing/selftests/net/af_unix/scm_rights.c b/tools/testing/selftests/net/af_unix/scm_rights.c
index 2bfed46e0b19..d66336256580 100644
--- a/tools/testing/selftests/net/af_unix/scm_rights.c
+++ b/tools/testing/selftests/net/af_unix/scm_rights.c
@@ -14,12 +14,12 @@
 
 FIXTURE(scm_rights)
 {
-	int fd[16];
+	int fd[32];
 };
 
 FIXTURE_VARIANT(scm_rights)
 {
-	char name[16];
+	char name[32];
 	int type;
 	int flags;
 	bool test_listener;
@@ -172,6 +172,8 @@ static void __create_sockets(struct __test_metadata *_metadata,
 			     const FIXTURE_VARIANT(scm_rights) *variant,
 			     int n)
 {
+	ASSERT_LE(n * 2, sizeof(self->fd) / sizeof(self->fd[0]));
+
 	if (variant->test_listener)
 		create_listeners(_metadata, self, n);
 	else
@@ -283,4 +285,23 @@ TEST_F(scm_rights, cross_edge)
 	close_sockets(8);
 }
 
+TEST_F(scm_rights, backtrack_from_scc)
+{
+	create_sockets(10);
+
+	send_fd(0, 1);
+	send_fd(0, 4);
+	send_fd(1, 2);
+	send_fd(2, 3);
+	send_fd(3, 1);
+
+	send_fd(5, 6);
+	send_fd(5, 9);
+	send_fd(6, 7);
+	send_fd(7, 8);
+	send_fd(8, 6);
+
+	close_sockets(10);
+}
+
 TEST_HARNESS_MAIN
---8<---