Date:   Fri, 26 Nov 2021 10:01:19 +0100
From:   Andrea Righi <andrea.righi@canonical.com>
To:     Kees Cook <keescook@chromium.org>
Cc:     Andy Lutomirski <luto@amacapital.net>,
        Will Drewry <wad@chromium.org>, Shuah Khan <shuah@kernel.org>,
        Christian Brauner <christian.brauner@ubuntu.com>,
        Alexei Starovoitov <ast@kernel.org>,
        linux-kselftest@vger.kernel.org, netdev@vger.kernel.org,
        bpf@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] selftests/seccomp: fix check of fds being assigned
Message-ID: <YaCiX+ypndYf/0QP@arighi-desktop>
References: <20211115165227.101124-1-andrea.righi@canonical.com>
 <202111180933.BE5101720@keescook>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <202111180933.BE5101720@keescook>
Precedence: bulk

On Thu, Nov 18, 2021 at 09:37:03AM -0800, Kees Cook wrote:
> 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?
> 

We run the kselftest inside a framework (bash/python scripts basically)
and this is what I see (I added a simple `ls -l /proc/pid/fd` in
seccomp_bpf.c after memfd is created):

11/26 08:50:08 DEBUG|     utils:0153| [stdout] # #  RUN           global.user_notification_addfd ...
11/26 08:50:08 DEBUG|     utils:0153| [stdout] # total 0
11/26 08:50:08 DEBUG|     utils:0153| [stdout] # lrwx------ 1 root root 64 Nov 26 08:50 0 -> /dev/pts/0
11/26 08:50:08 DEBUG|     utils:0153| [stdout] # l-wx------ 1 root root 64 Nov 26 08:50 1 -> pipe:[28844]
11/26 08:50:08 DEBUG|     utils:0153| [stdout] # lrwx------ 1 root root 64 Nov 26 08:50 10 -> /dev/pts/0
11/26 08:50:08 DEBUG|     utils:0153| [stdout] # lrwx------ 1 root root 64 Nov 26 08:50 11 -> /dev/pts/0
11/26 08:50:08 DEBUG|     utils:0153| [stdout] # l-wx------ 1 root root 64 Nov 26 08:50 12 -> /home/ubuntu/autotest/client/results/default/ubuntu_kernel_selftests.seccomp:seccomp_bpf/debug/ubuntu_kernel_selftests.seccomp:seccomp_bpf.DEBUG
11/26 08:50:08 DEBUG|     utils:0153| [stdout] # l-wx------ 1 root root 64 Nov 26 08:50 13 -> /home/ubuntu/autotest/client/results/default/ubuntu_kernel_selftests.seccomp:seccomp_bpf/debug/ubuntu_kernel_selftests.seccomp:seccomp_bpf.INFO
11/26 08:50:08 DEBUG|     utils:0153| [stdout] # l-wx------ 1 root root 64 Nov 26 08:50 14 -> /home/ubuntu/autotest/client/results/default/ubuntu_kernel_selftests.seccomp:seccomp_bpf/debug/ubuntu_kernel_selftests.seccomp:seccomp_bpf.WARNING
11/26 08:50:08 DEBUG|     utils:0153| [stdout] # l-wx------ 1 root root 64 Nov 26 08:50 15 -> /home/ubuntu/autotest/client/results/default/ubuntu_kernel_selftests.seccomp:seccomp_bpf/debug/ubuntu_kernel_selftests.seccomp:seccomp_bpf.ERROR
11/26 08:50:08 DEBUG|     utils:0153| [stdout] # l-wx------ 1 root root 64 Nov 26 08:50 16 -> pipe:[27608]
11/26 08:50:08 DEBUG|     utils:0153| [stdout] # l-wx------ 1 root root 64 Nov 26 08:50 17 -> pipe:[27609]
11/26 08:50:08 DEBUG|     utils:0153| [stdout] # l-wx------ 1 root root 64 Nov 26 08:50 2 -> pipe:[28844]
11/26 08:50:08 DEBUG|     utils:0153| [stdout] # l-wx------ 1 root root 64 Nov 26 08:50 3 -> pipe:[27803]
11/26 08:50:08 DEBUG|     utils:0153| [stdout] # l-wx------ 1 root root 64 Nov 26 08:50 4 -> pipe:[26387]
11/26 08:50:08 DEBUG|     utils:0153| [stdout] # l-wx------ 1 root root 64 Nov 26 08:50 5 -> /home/ubuntu/autotest/client/results/default/debug/client.WARNING
11/26 08:50:08 DEBUG|     utils:0153| [stdout] # l-wx------ 1 root root 64 Nov 26 08:50 6 -> /home/ubuntu/autotest/client/results/default/debug/client.ERROR
11/26 08:50:08 DEBUG|     utils:0153| [stdout] # lrwx------ 1 root root 64 Nov 26 08:50 7 -> /dev/pts/0
11/26 08:50:08 DEBUG|     utils:0153| [stdout] # lrwx------ 1 root root 64 Nov 26 08:50 8 -> /memfd:test (deleted)
11/26 08:50:08 DEBUG|     utils:0153| [stdout] # lrwx------ 1 root root 64 Nov 26 08:50 9 -> /dev/pts/0
11/26 08:50:08 DEBUG|     utils:0153| [stdout] # # seccomp_bpf.c:3993:user_notification_addfd:Expected listener (18) == nextfd++ (9)
11/26 08:50:09 DEBUG|     utils:0153| [stdout] # # user_notification_addfd: Test terminated by assertion
11/26 08:50:09 DEBUG|     utils:0153| [stdout] # #          FAIL  global.user_notification_addfd

As we can see memfd has been allocated in a hole (fd=8) and listener
will get fd=18, so checking for sequential fd numbers is not working in
this case.

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

I agree that my patch doesn't fix 100% of the cases, we may still have
fd holes.

> 
> > Fixes: 93e720d710df ("selftests/seccomp: More closely track fds being assigned")
> > Signed-off-by: Andrea Righi <andrea.righi@canonical.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?

How about getting the allocated fd numbers from /proc/pid/fd and
figuring out the next fd number taking also the holes into account?

Thanks,
-Andrea