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