linux-kernel - 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] [day] [month] [year] [list]

Date: Thu, 27 Jun 2024 01:04:11 +0900 (JST)
From: Shigeru Yoshida <syoshida@...hat.com>
To: kuniyu@...zon.com
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
 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()

On Tue, 25 Jun 2024 12:58:48 -0700, Kuniyuki Iwashima wrote:
> 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 ?

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@...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<---
>