LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] selftests/seccomp: fix check of fds being assigned
@ 2021-11-15 16:52 Andrea Righi
  2021-11-16  9:33 ` Christian Brauner
  2021-11-18 17:37 ` Kees Cook
  0 siblings, 2 replies; 4+ messages in thread
From: Andrea Righi @ 2021-11-15 16:52 UTC (permalink / raw)
  To: Kees Cook
  Cc: Andy Lutomirski, Will Drewry, Shuah Khan, Christian Brauner,
	Alexei Starovoitov, linux-kselftest, netdev, bpf, linux-kernel

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

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;
 
 	pid = fork();
 	ASSERT_GE(pid, 0);
-- 
2.32.0


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] selftests/seccomp: fix check of fds being assigned
  2021-11-15 16:52 [PATCH] selftests/seccomp: fix check of fds being assigned Andrea Righi
@ 2021-11-16  9:33 ` Christian Brauner
  2021-11-18 17:37 ` Kees Cook
  1 sibling, 0 replies; 4+ messages in thread
From: Christian Brauner @ 2021-11-16  9:33 UTC (permalink / raw)
  To: Andrea Righi
  Cc: Kees Cook, Andy Lutomirski, Will Drewry, Shuah Khan,
	Alexei Starovoitov, linux-kselftest, netdev, bpf, linux-kernel

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

Thanks!
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] selftests/seccomp: fix check of fds being assigned
  2021-11-15 16:52 [PATCH] selftests/seccomp: fix check of fds being assigned Andrea Righi
  2021-11-16  9:33 ` Christian Brauner
@ 2021-11-18 17:37 ` Kees Cook
  2021-11-26  9:01   ` Andrea Righi
  1 sibling, 1 reply; 4+ messages in thread
From: Kees Cook @ 2021-11-18 17:37 UTC (permalink / raw)
  To: Andrea Righi
  Cc: Andy Lutomirski, Will Drewry, Shuah Khan, Christian Brauner,
	Alexei Starovoitov, linux-kselftest, netdev, bpf, linux-kernel

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

-Kees

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

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] selftests/seccomp: fix check of fds being assigned
  2021-11-18 17:37 ` Kees Cook
@ 2021-11-26  9:01   ` Andrea Righi
  0 siblings, 0 replies; 4+ messages in thread
From: Andrea Righi @ 2021-11-26  9:01 UTC (permalink / raw)
  To: Kees Cook
  Cc: Andy Lutomirski, Will Drewry, Shuah Khan, Christian Brauner,
	Alexei Starovoitov, linux-kselftest, netdev, bpf, linux-kernel

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-11-26  9:03 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-15 16:52 [PATCH] selftests/seccomp: fix check of fds being assigned Andrea Righi
2021-11-16  9:33 ` Christian Brauner
2021-11-18 17:37 ` Kees Cook
2021-11-26  9:01   ` Andrea Righi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).