| Message ID | 20240625152713.1147650-1-syoshida@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] af_unix: Fix uninit-value in __unix_walk_scc() | expand |
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<--- >
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)
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(-)