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.

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]
 unix_walk_scc net/unix/garbage.c:526 [inline]
 __unix_gc+0x2589/0x3c20 net/unix/garbage.c:584
 process_one_work kernel/workqueue.c:3231 [inline]
 process_scheduled_works+0xade/0x1bf0 kernel/workqueue.c:3312
 worker_thread+0xeb6/0x15b0 kernel/workqueue.c:3393
 kthread+0x3c4/0x530 kernel/kthread.c:389
 ret_from_fork+0x6e/0x90 arch/x86/kernel/process.c:147
 ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244

Uninit was stored to memory at:
 unix_walk_scc net/unix/garbage.c:526 [inline]
 __unix_gc+0x2adf/0x3c20 net/unix/garbage.c:584
 process_one_work kernel/workqueue.c:3231 [inline]
 process_scheduled_works+0xade/0x1bf0 kernel/workqueue.c:3312
 worker_thread+0xeb6/0x15b0 kernel/workqueue.c:3393
 kthread+0x3c4/0x530 kernel/kthread.c:389
 ret_from_fork+0x6e/0x90 arch/x86/kernel/process.c:147
 ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244

Local variable entries created at:
 ref_tracker_free+0x48/0xf30 lib/ref_tracker.c:222
 netdev_tracker_free include/linux/netdevice.h:4058 [inline]
 netdev_put include/linux/netdevice.h:4075 [inline]
 dev_put include/linux/netdevice.h:4101 [inline]
 update_gid_event_work_handler+0xaa/0x1b0 drivers/infiniband/core/roce_gid_mgmt.c:813

CPU: 1 PID: 12763 Comm: kworker/u8:31 Not tainted 6.10.0-rc4-00217-g35bb670d65fc #32
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-2.fc40 04/01/2014
Workqueue: events_unbound __unix_gc

Fixes: 3484f063172d ("af_unix: Detect Strongly Connected Components.")
Reported-by: syzkaller <syzkaller@googlegroups.com>
Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
---
 net/unix/garbage.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/net/unix/garbage.c b/net/unix/garbage.c
index dfe94a90ece4..23efb78fe9ef 100644
--- a/net/unix/garbage.c
+++ b/net/unix/garbage.c
@@ -476,6 +476,7 @@ static void __unix_walk_scc(struct unix_vertex *vertex, unsigned long *last_inde
 	}
 
 	if (vertex->index == vertex->scc_index) {
+		struct unix_vertex *v;
 		struct list_head scc;
 		bool scc_dead = true;
 
@@ -486,15 +487,15 @@ static void __unix_walk_scc(struct unix_vertex *vertex, unsigned long *last_inde
 		 */
 		__list_cut_position(&scc, &vertex_stack, &vertex->scc_entry);
 
-		list_for_each_entry_reverse(vertex, &scc, scc_entry) {
+		list_for_each_entry_reverse(v, &scc, scc_entry) {
 			/* Don't restart DFS from this vertex in unix_walk_scc(). */
-			list_move_tail(&vertex->entry, &unix_visited_vertices);
+			list_move_tail(&v->entry, &unix_visited_vertices);
 
 			/* Mark vertex as off-stack. */
-			vertex->index = unix_vertex_grouped_index;
+			v->index = unix_vertex_grouped_index;
 
 			if (scc_dead)
-				scc_dead = unix_vertex_dead(vertex);
+				scc_dead = unix_vertex_dead(v);
 		}
 
 		if (scc_dead)
-- 
2.45.2

From: Shigeru Yoshida <syoshida@redhat.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@amazon.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@amazon.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<---

On Tue, 25 Jun 2024 12:58:48 -0700, Kuniyuki Iwashima wrote:
> From: Shigeru Yoshida <syoshida@redhat.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 ?

Thank you for your comment!

I ran the test below without my patch several times, but it couldn't
catch KMSAN splat.

Perhaps this issue depends on the state of the stack. Even the repro
created by syzkaller takes a few minutes to catch the issue on my
environment.

I used the following version of clang:

$ clang --version
clang version 18.1.6 (Fedora 18.1.6-3.fc40)
Target: x86_64-redhat-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
Configuration file: /etc/clang/x86_64-redhat-linux-gnu-clang.cfg

Thanks,
Shigeru

> 
> ---8<---
> From: Kuniyuki Iwashima <kuniyu@amazon.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@amazon.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<---
>