netdev - Re: [PATCH net 4/4] selftests/net: mptcp: fix uninitialized variable warnings

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]

Message-ID: <7418fa0c-c0c2-4615-ba55-f148ceb82328@kernel.org>
Date: Mon, 27 Nov 2023 13:29:46 +0100
From: Matthieu Baerts <matttbe@...nel.org>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>, netdev@...r.kernel.org
Cc: davem@...emloft.net, kuba@...nel.org, edumazet@...gle.com,
 pabeni@...hat.com, linux-kselftest@...r.kernel.org,
 Willem de Bruijn <willemb@...gle.com>, Florian Westphal <fw@...len.de>,
 MPTCP Upstream <mptcp@...ts.linux.dev>
Subject: Re: [PATCH net 4/4] selftests/net: mptcp: fix uninitialized variable
 warnings

Hi Willem,

(+ cc MPTCP list)

On 24/11/2023 18:15, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@...gle.com>
> 
> Same init_rng() in both tests. The function reads /dev/urandom to
> initialize srand(). In case of failure, it falls back onto the
> entropy in the uninitialized variable. Not sure if this is on purpose.
> But failure reading urandom should be rare, so just fail hard. While
> at it, convert to getrandom(). Which man 4 random suggests is simpler
> and more robust.
> 
>     mptcp_inq.c:525:6:
>     mptcp_connect.c:1131:6:
> 
>     error: variable 'foo' is used uninitialized
>     whenever 'if' condition is false
>     [-Werror,-Wsometimes-uninitialized]

Thank you for the patch!

It looks good to me:

Reviewed-by: Matthieu Baerts <matttbe@...nel.org>

> Fixes: 048d19d444be ("mptcp: add basic kselftest for mptcp")
> Fixes: b51880568f20 ("selftests: mptcp: add inq test case")
> Cc: Florian Westphal <fw@...len.de>
> Signed-off-by: Willem de Bruijn <willemb@...gle.com>
> 
> ----
> 
> When input is randomized because this is expected to meaningfully
> explore edge cases, should we also add
> 1. logging the random seed to stdout and
> 2. adding a command line argument to replay from a specific seed
> I can do this in net-next, if authors find it useful in this case.

I think we should have done that from the beginning, otherwise we cannot
easily reproduce these edge cases. To be honest, I don't think this
technique helped to find bugs, and it was probably used here as a good
habit to increase the coverage. But on the other hand, we might not
realise some inputs are randomised and can cause instabilities in the
tests because we don't print anything about that.

So I would say that the minimal thing to do is to log the random seed.
But it might not be that easy to do, for example 'mptcp_connect' is used
a lot of time by the .sh scripts: printing this seed number each time
'mptcp_connect' is started will "flood" the logs. Maybe we should only
print that at the end, in case of errors: e.g. in xerror() and
die_perror() for example, but I see 'exit(1)' is directly used in other
places...

That's more code to change, but if it is still OK for you to do that,
please also note that you will need to log this to stderr: mptcp_connect
prints what has been received from the other peer to stdout.

Because it is more than just adding a 'printf()', I just created a
ticket in our bug tracker, so anybody can look at that and check all the
details about that:

https://github.com/multipath-tcp/mptcp_net-next/issues/462

> ---
>  tools/testing/selftests/net/mptcp/mptcp_connect.c | 11 ++++-------
>  tools/testing/selftests/net/mptcp/mptcp_inq.c     | 11 ++++-------
>  2 files changed, 8 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> index c7f9ebeebc2c5..d2043ec3bf6d6 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c

(...)

> @@ -1125,15 +1126,11 @@ int main_loop_s(int listensock)
>  
>  static void init_rng(void)
>  {
> -	int fd = open("/dev/urandom", O_RDONLY);
>  	unsigned int foo;
>  
> -	if (fd > 0) {

I just realised that here, we could have fd == 0 which is a valid value.
I don't think we would have that when executing the selftests, but
that's another reason to change this :)

> -		int ret = read(fd, &foo, sizeof(foo));
> -
> -		if (ret < 0)
> -			srand(fd + foo);
> -		close(fd);
> +	if (getrandom(&foo, sizeof(foo), 0) == -1) {
> +		perror("getrandom");
> +		exit(1);
>  	}
>  
>  	srand(foo);

Cheers,
Matt