netdev - Re: [PATCH] selftests/seccomp: fix check of fds being assigned

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: <202111180933.BE5101720@keescook>
Date:   Thu, 18 Nov 2021 09:37:03 -0800
From:   Kees Cook <keescook@...omium.org>
To:     Andrea Righi <andrea.righi@...onical.com>
Cc:     Andy Lutomirski <luto@...capital.net>,
        Will Drewry <wad@...omium.org>, Shuah Khan <shuah@...nel.org>,
        Christian Brauner <christian.brauner@...ntu.com>,
        Alexei Starovoitov <ast@...nel.org>,
        linux-kselftest@...r.kernel.org, netdev@...r.kernel.org,
        bpf@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] selftests/seccomp: fix check of fds being assigned

On Mon, Nov 15, 2021 at 05:52:27PM +0100, Andrea Righi wrote:
> There might be an arbitrary free open fd slot when we run the addfd
> sub-test, so checking for progressive numbers of file descriptors
> starting from memfd is not always a reliable check and we could get the
> following failure:
> 
>   #  RUN           global.user_notification_addfd ...
>   # seccomp_bpf.c:3989:user_notification_addfd:Expected listener (18) == nextfd++ (9)

What injected 9 extra fds into this test?

>   # user_notification_addfd: Test terminated by assertion
> 
> Simply check if memfd and listener are valid file descriptors and start
> counting for progressive file checking with the listener fd.

Hm, so I attempted to fix this once already:
93e720d710df ("selftests/seccomp: More closely track fds being assigned")
so I'm not sure the proposed patch really improves it in the general
case.

> Fixes: 93e720d710df ("selftests/seccomp: More closely track fds being assigned")
> Signed-off-by: Andrea Righi <andrea.righi@...onical.com>
> ---
>  tools/testing/selftests/seccomp/seccomp_bpf.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
> index d425688cf59c..4f37153378a1 100644
> --- a/tools/testing/selftests/seccomp/seccomp_bpf.c
> +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
> @@ -3975,18 +3975,17 @@ TEST(user_notification_addfd)
>  	/* There may be arbitrary already-open fds at test start. */
>  	memfd = memfd_create("test", 0);
>  	ASSERT_GE(memfd, 0);
> -	nextfd = memfd + 1;
>  
>  	ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
>  	ASSERT_EQ(0, ret) {
>  		TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!");
>  	}
>  
> -	/* fd: 4 */
>  	/* Check that the basic notification machinery works */
>  	listener = user_notif_syscall(__NR_getppid,
>  				      SECCOMP_FILTER_FLAG_NEW_LISTENER);
> -	ASSERT_EQ(listener, nextfd++);
> +	ASSERT_GE(listener, 0);
> +	nextfd = listener + 1;

e.g. if there was a hole in the fd map for memfd, why not listener too?

Should the test dup2 memfd up to fd 100 and start counting there or
something? What is the condition that fills the fds for this process?

-Kees

>  
>  	pid = fork();
>  	ASSERT_GE(pid, 0);
> -- 
> 2.32.0
> 

-- 
Kees Cook