seccomp: Support atomic "addfd + send"

[rata]

Here it is a c&p of the commit msg. Github doesn't show the "1" and friends correctly, but that is common use on the linux commit msg IIRC.

Alban Crequy reported a race condition userspace faces when we want to
add an fd and make the same syscall return that value1.

Currently two different ioctl() calls are needed by the process handling
the syscalls (agent) for another userspace process (target). Therefore,
it is possible for the agent to do the first ioctl to add a file
descriptor but the target is interrupted (EINTR) before the agent does
the second ioctl() call.

This patch adds a flag to the ADDFD ioctl() so it adds the fd and
returns that value atomically to the target program, as suggested by
Kees Cook2. This is done by simply allowing
seccomp_do_user_notification() to add the fd and return it in this
case. Therefore, in this case the target wakes up from the wait in
seccomp_do_user_notification() either to interrupt the syscall or to add
the fd and return it.

The struct seccomp_notif_resp, used when doing SECCOMP_IOCTL_NOTIF_SEND
ioctl() to send a response to the target, has three more fields that we
don't allow to set when doing the addfd ioctl() to also return. The
reasons to disallow each field are:

• val: This will be set to the new allocated fd.
• error: If this is non-zero, the value is ignored. Therefore,
  it is pointless in this case as we want to return the value.
• flags: The only flag is to let userspace continue to execute the
  syscall. This seems pointless, as we want the syscall to return the
  allocated fd.

How to use

See the selftest example

Testing done

Used the selftest example and a local test to read the fd here.

Missing stuff:

• Test it with Alban golang examples that hit the race condition
• Rebase to send for 5.11/5.12 (this is currently based on linux 5.10-rc5, this is of course not good)
• Remove printk. Left them in case you test it, they were useful for me
• Add selftests
• Should I use my personal or kinvolk email? --> switched to kinvolk email.

[alban]

It would be good to test that the fd was installed at the correct place in the fdtable (addfd.newfd = 0;):

+	EXPECT_EQ(fd, 0);
 	EXPECT_EQ(filecmp(getpid(), pid, memfd, fd), 0);

[rata]

That won't work, this won't be fd 0. To specify a remote fd number to use, you need to set SECCOMP_ADDFD_FLAG_SETFD. This is another flag and that setting this flag only does what we want is tested a few lines above this: https://github.com/kinvolk/linux/pull/6/files#diff-8933b3a4375dff05fe0d6851a10c22a2f291f9edaa8592c229d73f861eaf73b8L4035-L4040

[alban]

oh, I see... then, could you add a similar test on the correct value of fd? I see this piece of code a bit earlier in this file:

	/*
	 * The child has fds 0(stdin), 1(stdout), 2(stderr), 3(memfd),
	 * 4(listener), so the newly allocated fd should be 5.
	 */
	EXPECT_EQ(fd, 5);

[rata]

Oh, sure, added that!

[alban]

You could add a check that syscall(__NR_getppid), F_GETFD) returns the fd number requested in the other side. In this case, 0 (line 4074 addfd.newfd = 0;). I suggest to try 43, so that we test that the fd is correctly returned by the syscall.

[rata]

Not 0 in this case, that is ignored. Also as said in the other comment the setting fd func is tested. I can also add a test combining them, but I won't like to mix the tests of different flags.

[alban]

I think it's good to have a test that combine the too.

[alban]

Why 0?

Can we add a unit test to check that the return value and the errno of the syscall is correct in this case?

I assume it should return:

• return value: -1
• errno: EMFILE ("The per-process limit on the number of open file descriptors has been reached")

[rata]

Yeah, if you set error to -errno, the call to syscall_set_return_value() at the end of this function will handle that (return -1 and set errno to the target). Here is the function doc: https://github.com/torvalds/linux/blob/19c329f6808995b142b3966301f217c831e7cf31/include/asm-generic/syscall.h#L85-L100

If you grep the kernel tree for usage of syscall_set_return_value() you will find that it is always used like this (error set to -errno and value is 0). I did the same for consistency, and tested and this works fine.

However, I noticed that if I don't set these values here, the proper thing is returned to the target if I add more fds that we can: -1 and errno set to too many files. I didn't fully understand if that was happening by chance or it was on purpose (as in that case we call syscall_set_return_value() with err=0 and val=-errno). To be safe I used the same convention it is used in the rest of the kernel code, as also the documentation of syscall_set_return_value() t say what we do here should work but not other things. But planned to ask in the cover letter.

[rata]

Regarding the test, I wanted to ask if this looks good and I can add more tests in follow-up patches. What do you think? I wanted to avoid spending more time on tests now, before knowing the overall impression of the patch

[alban]

Fine for me to have this first iteration. You could list in the cover letter the list of missing tests, and mark this patch set as RFC.

[alban]

I think the cover letter should mention that the caveat that this patch set does not solve addfd + send for syscalls that install new fds but don't return the fd number in the return value, such as:

• socketpair()
• pipe()
• recvmsg() with SCM_RIGHTS

[rata]

This is merged upstream, closing