[PATCH net] af_unix: Fix uninit-value in __unix_walk_scc()

[Shigeru Yoshida]

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 <syzk...@googlegroups.com>
Signed-off-by: Shigeru Yoshida <syos...@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

[Kuniyuki Iwashima]

From: Shigeru Yoshida <syos...@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 <kun...@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 <kun...@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<---

[Shigeru Yoshida]

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

[Shigeru Yoshida]

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 (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]

[Shigeru Yoshida]

v1->v2: https://lore.kernel.org/all/20240625152713.1...@redhat.com/
- A bit of elaboration on the commit message, as suggested by Iwashima-san.
- Bundle a selftest written by Iwashima-san.

[Kuniyuki Iwashima]

From: Shigeru Yoshida <syos...@redhat.com>
Date: Wed, 3 Jul 2024 01:04:27 +0900

Reviewed-by: Kuniyuki Iwashima <kun...@amazon.com>

Thanks!

[patchwork-b...@kernel.org]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <ku...@kernel.org>:

On Wed, 3 Jul 2024 01:04:27 +0900 you wrote:
> 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 (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)).
>

> [...]

Here is the summary with links:
- [net,1/2] af_unix: Fix uninit-value in __unix_walk_scc()
https://git.kernel.org/netdev/net/c/927fa5b3e4f5
- [net,2/2] selftest: af_unix: Add test case for backtrack after finalising SCC.
https://git.kernel.org/netdev/net/c/2a79651bf2fa

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html