[PATCH v2 0/4] Atomic addfd send and reply

[PATCH v2 0/4] Atomic addfd send and reply

[Sargun Dhillon]

This is somewhat of a respin of "Handle seccomp notification preemption"
but without the controversial parts.


This patchset addresses a race condition we've dealt with recently with
seccomp. Specifically programs interrupting syscalls while they're in
progress. This was exacerbated by Golang's recent adoption of "async
preemption", in which they try to interrupt any syscall that's been running
for more than 10ms during GC. During certain syscalls, it's non-trivial to
write them in a reetrant manner in userspace (socket).

It focuses on one use cases, which is adding file descriptors to a process
"atomically" during the seccomp reply, as opposed to discretizing the calls
which may result in a potential file descriptor leak and inconsistent
program state.

Changes since v1:
 * Clarifcation to commit comments

Rodrigo Campos (2):
  seccomp: Support atomic "addfd + send reply"
  selftests/seccomp: Add test for atomic addfd+send

Sargun Dhillon (2):
  Documentation: seccomp: Fix user notification documentation
  seccomp: Refactor notification handler to prepare for new semantics

 .../userspace-api/seccomp_filter.rst          | 28 +++++--
 include/uapi/linux/seccomp.h                  |  1 +
 kernel/seccomp.c                              | 79 ++++++++++++++-----
 tools/testing/selftests/seccomp/seccomp_bpf.c | 38 +++++++++
 4 files changed, 120 insertions(+), 26 deletions(-)

-- 
2.25.1

[PATCH v2 1/4] Documentation: seccomp: Fix user notification documentation

[Sargun Dhillon]

The documentation had some previously incorrect information about how
userspace notifications (and responses) were handled due to a change
from a previously proposed patchset.

Signed-off-by: Sargun Dhillon <sargun@sargun.me>
Acked-by: Tycho Andersen <tycho@tycho.pizza>
---
 Documentation/userspace-api/seccomp_filter.rst | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/Documentation/userspace-api/seccomp_filter.rst b/Documentation/userspace-api/seccomp_filter.rst
index bd9165241b6c..6efb41cc8072 100644
--- a/Documentation/userspace-api/seccomp_filter.rst
+++ b/Documentation/userspace-api/seccomp_filter.rst
@@ -250,14 +250,14 @@ Users can read via ``ioctl(SECCOMP_IOCTL_NOTIF_RECV)``  (or ``poll()``) on a
 seccomp notification fd to receive a ``struct seccomp_notif``, which contains
 five members: the input length of the structure, a unique-per-filter ``id``,
 the ``pid`` of the task which triggered this request (which may be 0 if the
-task is in a pid ns not visible from the listener's pid namespace), a ``flags``
-member which for now only has ``SECCOMP_NOTIF_FLAG_SIGNALED``, representing
-whether or not the notification is a result of a non-fatal signal, and the
-``data`` passed to seccomp. Userspace can then make a decision based on this
-information about what to do, and ``ioctl(SECCOMP_IOCTL_NOTIF_SEND)`` a
-response, indicating what should be returned to userspace. The ``id`` member of
-``struct seccomp_notif_resp`` should be the same ``id`` as in ``struct
-seccomp_notif``.
+task is in a pid ns not visible from the listener's pid namespace). The
+notification also contains the ``data`` passed to seccomp, and a filters flag.
+The structure should be zeroed out prior to calling the ioctl.
+
+Userspace can then make a decision based on this information about what to do,
+and ``ioctl(SECCOMP_IOCTL_NOTIF_SEND)`` a response, indicating what should be
+returned to userspace. The ``id`` member of ``struct seccomp_notif_resp`` should
+be the same ``id`` as in ``struct seccomp_notif``.
 
 It is worth noting that ``struct seccomp_data`` contains the values of register
 arguments to the syscall, but does not contain pointers to memory. The task's
-- 
2.25.1

[PATCH v2 2/4] seccomp: Refactor notification handler to prepare for new semantics

[Sargun Dhillon]

This refactors the user notification code to have a do / while loop around
the completion condition. This has a small change in semantic, in that
previously we ignored addfd calls upon wakeup if the notification had been
responded to, but instead with the new change we check for an outstanding
addfd calls prior to returning to userspace.

Rodrigo Campos also identified a bug that can result in addfd causing
an early return, when the supervisor didn't actually handle the
syscall [1].

[1]: https://lore.kernel.org/lkml/20210413160151.3301-1-rodrigo@kinvolk.io/

Fixes: 7cf97b125455 ("seccomp: Introduce addfd ioctl to seccomp user notifier")
Signed-off-by: Sargun Dhillon <sargun@sargun.me>
Acked-by: Tycho Andersen <tycho@tycho.pizza>
---
 kernel/seccomp.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 1d60fc2c9987..93684cc63285 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -1098,28 +1098,30 @@ static int seccomp_do_user_notification(int this_syscall,
 
 	up(&match->notif->request);
 	wake_up_poll(&match->wqh, EPOLLIN | EPOLLRDNORM);
-	mutex_unlock(&match->notify_lock);
 
 	/*
 	 * This is where we wait for a reply from userspace.
 	 */
-wait:
-	err = wait_for_completion_interruptible(&n.ready);
-	mutex_lock(&match->notify_lock);
-	if (err == 0) {
-		/* Check if we were woken up by a addfd message */
+	do {
+		mutex_unlock(&match->notify_lock);
+		err = wait_for_completion_interruptible(&n.ready);
+		mutex_lock(&match->notify_lock);
+		if (err != 0)
+			goto interrupted;
+
 		addfd = list_first_entry_or_null(&n.addfd,
 						 struct seccomp_kaddfd, list);
-		if (addfd && n.state != SECCOMP_NOTIFY_REPLIED) {
+		/* Check if we were woken up by a addfd message */
+		if (addfd)
 			seccomp_handle_addfd(addfd);
-			mutex_unlock(&match->notify_lock);
-			goto wait;
-		}
-		ret = n.val;
-		err = n.error;
-		flags = n.flags;
-	}
 
+	}  while (n.state != SECCOMP_NOTIFY_REPLIED);
+
+	ret = n.val;
+	err = n.error;
+	flags = n.flags;
+
+interrupted:
 	/* If there were any pending addfd calls, clear them out */
 	list_for_each_entry_safe(addfd, tmp, &n.addfd, list) {
 		/* The process went away before we got a chance to handle it */
-- 
2.25.1

[PATCH v2 3/4] seccomp: Support atomic "addfd + send reply"

[Sargun Dhillon]

From: Rodrigo Campos <rodrigo@kinvolk.io>

Alban Crequy reported a race condition userspace faces when we want to
add some fds and make the syscall return them[1] using seccomp notify.

The problem is that currently two different ioctl() calls are needed by
the process handling the syscalls (agent) for another userspace process
(target): SECCOMP_IOCTL_NOTIF_ADDFD to allocate the fd and
SECCOMP_IOCTL_NOTIF_SEND to return that value. 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 Cook[2]. 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.

This "allocate an fd and return" functionality is useful for syscalls
that return a file descriptor only, like connect(2). Other syscalls that
return a file descriptor but not as return value (or return more than
one fd), like socketpair(), pipe(), recvmsg with SCM_RIGHTs, will not
work with this flag.

This effectively combines SECCOMP_IOCTL_NOTIF_ADDFD and
SECCOMP_IOCTL_NOTIF_SEND into an atomic opteration. The notification's
return value, nor error can be set by the user. Upon successful invocation
of the SECCOMP_IOCTL_NOTIF_ADDFD ioctl with the SECCOMP_ADDFD_FLAG_SEND
flag, the notifying process's errno will be 0, and the return value will
be the file descriptor number that was installed.

[1]: https://lore.kernel.org/lkml/CADZs7q4sw71iNHmV8EOOXhUKJMORPzF7thraxZYddTZsxta-KQ@mail.gmail.com/
[2]: https://lore.kernel.org/lkml/202012011322.26DCBC64F2@keescook/

Signed-off-by: Rodrigo Campos <rodrigo@kinvolk.io>
Signed-off-by: Sargun Dhillon <sargun@sargun.me>
Acked-by: Tycho Andersen <tycho@tycho.pizza>
---
 .../userspace-api/seccomp_filter.rst          | 12 +++++
 include/uapi/linux/seccomp.h                  |  1 +
 kernel/seccomp.c                              | 49 +++++++++++++++++--
 3 files changed, 58 insertions(+), 4 deletions(-)

diff --git a/Documentation/userspace-api/seccomp_filter.rst b/Documentation/userspace-api/seccomp_filter.rst
index 6efb41cc8072..d61219889e49 100644
--- a/Documentation/userspace-api/seccomp_filter.rst
+++ b/Documentation/userspace-api/seccomp_filter.rst
@@ -259,6 +259,18 @@ and ``ioctl(SECCOMP_IOCTL_NOTIF_SEND)`` a response, indicating what should be
 returned to userspace. The ``id`` member of ``struct seccomp_notif_resp`` should
 be the same ``id`` as in ``struct seccomp_notif``.
 
+Userspace can also add file descriptors to the notifying process via
+``ioctl(SECCOMP_IOCTL_NOTIF_ADDFD)``. The ``id`` member of
+``struct seccomp_notif_addfd`` should be the same ``id`` as in
+``struct seccomp_notif``. The ``newfd_flags`` flag may be used to set flags
+like O_EXEC on the file descriptor in the notifying process. If the supervisor
+wants to inject the file descriptor with a specific number, the
+``SECCOMP_ADDFD_FLAG_SETFD`` flag can be used, and set the ``newfd`` member to
+the specific number to use. If that file descriptor is already open in the
+notifying process it will be replaced. The supervisor can also add an FD, and
+respond atomically by using the ``SECCOMP_ADDFD_FLAG_SEND`` flag and the return
+value will be the injected file descriptor number.
+
 It is worth noting that ``struct seccomp_data`` contains the values of register
 arguments to the syscall, but does not contain pointers to memory. The task's
 memory is accessible to suitably privileged traces via ``ptrace()`` or
diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
index 6ba18b82a02e..78074254ab98 100644
--- a/include/uapi/linux/seccomp.h
+++ b/include/uapi/linux/seccomp.h
@@ -115,6 +115,7 @@ struct seccomp_notif_resp {
 
 /* valid flags for seccomp_notif_addfd */
 #define SECCOMP_ADDFD_FLAG_SETFD	(1UL << 0) /* Specify remote fd */
+#define SECCOMP_ADDFD_FLAG_SEND		(1UL << 1) /* Addfd and return it, atomically */
 
 /**
  * struct seccomp_notif_addfd
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 93684cc63285..3636f9584991 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -107,6 +107,7 @@ struct seccomp_knotif {
  *      installing process should allocate the fd as normal.
  * @flags: The flags for the new file descriptor. At the moment, only O_CLOEXEC
  *         is allowed.
+ * @ioctl_flags: The flags used for the seccomp_addfd ioctl.
  * @ret: The return value of the installing process. It is set to the fd num
  *       upon success (>= 0).
  * @completion: Indicates that the installing process has completed fd
@@ -118,6 +119,7 @@ struct seccomp_kaddfd {
 	struct file *file;
 	int fd;
 	unsigned int flags;
+	__u32 ioctl_flags;
 
 	/* To only be set on reply */
 	int ret;
@@ -1062,14 +1064,35 @@ static u64 seccomp_next_notify_id(struct seccomp_filter *filter)
 	return filter->notif->next_id++;
 }
 
-static void seccomp_handle_addfd(struct seccomp_kaddfd *addfd)
+static void seccomp_handle_addfd(struct seccomp_kaddfd *addfd, struct seccomp_knotif *n)
 {
+	int fd;
+
 	/*
 	 * Remove the notification, and reset the list pointers, indicating
 	 * that it has been handled.
 	 */
 	list_del_init(&addfd->list);
-	addfd->ret = receive_fd_replace(addfd->fd, addfd->file, addfd->flags);
+	fd = receive_fd_replace(addfd->fd, addfd->file, addfd->flags);
+
+	addfd->ret = fd;
+
+	if (addfd->ioctl_flags & SECCOMP_ADDFD_FLAG_SEND) {
+		/* If we fail reset and return an error to the notifier */
+		if (fd < 0) {
+			n->state = SECCOMP_NOTIFY_SENT;
+		} else {
+			/* Return the FD we just added */
+			n->flags = 0;
+			n->error = 0;
+			n->val = fd;
+		}
+	}
+
+	/*
+	 * Mark the notification as completed. From this point, addfd mem
+	 * might be invalidated and we can't safely read it anymore.
+	 */
 	complete(&addfd->completion);
 }
 
@@ -1113,7 +1136,7 @@ static int seccomp_do_user_notification(int this_syscall,
 						 struct seccomp_kaddfd, list);
 		/* Check if we were woken up by a addfd message */
 		if (addfd)
-			seccomp_handle_addfd(addfd);
+			seccomp_handle_addfd(addfd, &n);
 
 	}  while (n.state != SECCOMP_NOTIFY_REPLIED);
 
@@ -1574,7 +1597,7 @@ static long seccomp_notify_addfd(struct seccomp_filter *filter,
 	if (addfd.newfd_flags & ~O_CLOEXEC)
 		return -EINVAL;
 
-	if (addfd.flags & ~SECCOMP_ADDFD_FLAG_SETFD)
+	if (addfd.flags & ~(SECCOMP_ADDFD_FLAG_SETFD | SECCOMP_ADDFD_FLAG_SEND))
 		return -EINVAL;
 
 	if (addfd.newfd && !(addfd.flags & SECCOMP_ADDFD_FLAG_SETFD))
@@ -1584,6 +1607,7 @@ static long seccomp_notify_addfd(struct seccomp_filter *filter,
 	if (!kaddfd.file)
 		return -EBADF;
 
+	kaddfd.ioctl_flags = addfd.flags;
 	kaddfd.flags = addfd.newfd_flags;
 	kaddfd.fd = (addfd.flags & SECCOMP_ADDFD_FLAG_SETFD) ?
 		    addfd.newfd : -1;
@@ -1609,6 +1633,23 @@ static long seccomp_notify_addfd(struct seccomp_filter *filter,
 		goto out_unlock;
 	}
 
+	if (addfd.flags & SECCOMP_ADDFD_FLAG_SEND) {
+		/*
+		 * Disallow queuing an atomic addfd + send reply while there are
+		 * some addfd requests still to process.
+		 *
+		 * There is no clear reason to support it and allows us to keep
+		 * the loop on the other side straight-forward.
+		 */
+		if (!list_empty(&knotif->addfd)) {
+			ret = -EBUSY;
+			goto out_unlock;
+		}
+
+		/* Allow exactly only one reply */
+		knotif->state = SECCOMP_NOTIFY_REPLIED;
+	}
+
 	list_add(&kaddfd.list, &knotif->addfd);
 	complete(&knotif->ready);
 	mutex_unlock(&filter->notify_lock);
-- 
2.25.1

[PATCH v2 4/4] selftests/seccomp: Add test for atomic addfd+send

[Sargun Dhillon]

From: Rodrigo Campos <rodrigo@kinvolk.io>

This just adds a test to verify that when using the new introduced flag
to ADDFD, a valid fd is added and returned as the syscall result.

Signed-off-by: Rodrigo Campos <rodrigo@kinvolk.io>
Signed-off-by: Sargun Dhillon <sargun@sargun.me>
Acked-by: Tycho Andersen <tycho@tycho.pizza>
---
 tools/testing/selftests/seccomp/seccomp_bpf.c | 38 +++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 98c3b647f54d..e2ba7adc2694 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -235,6 +235,10 @@ struct seccomp_notif_addfd {
 };
 #endif
 
+#ifndef SECCOMP_ADDFD_FLAG_SEND
+#define SECCOMP_ADDFD_FLAG_SEND	(1UL << 1) /* Addfd and return it, atomically */
+#endif
+
 struct seccomp_notif_addfd_small {
 	__u64 id;
 	char weird[4];
@@ -3976,8 +3980,14 @@ TEST(user_notification_addfd)
 	ASSERT_GE(pid, 0);
 
 	if (pid == 0) {
+		/* fds will be added and this value is expected */
 		if (syscall(__NR_getppid) != USER_NOTIF_MAGIC)
 			exit(1);
+
+		/* Atomic addfd+send is received here. Check it is a valid fd */
+		if (fcntl(syscall(__NR_getppid), F_GETFD) == -1)
+			exit(1);
+
 		exit(syscall(__NR_getppid) != USER_NOTIF_MAGIC);
 	}
 
@@ -4056,6 +4066,30 @@ TEST(user_notification_addfd)
 	ASSERT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
 	ASSERT_EQ(addfd.id, req.id);
 
+	/* Verify we can do an atomic addfd and send */
+	addfd.newfd = 0;
+	addfd.flags = SECCOMP_ADDFD_FLAG_SEND;
+	fd = ioctl(listener, SECCOMP_IOCTL_NOTIF_ADDFD, &addfd);
+
+	/* Child has fds 0-6 and 42 used, we expect the lower fd available: 7 */
+	EXPECT_EQ(fd, 7);
+	EXPECT_EQ(filecmp(getpid(), pid, memfd, fd), 0);
+
+	/*
+	 * This sets the ID of the ADD FD to the last request plus 1. The
+	 * notification ID increments 1 per notification.
+	 */
+	addfd.id = req.id + 1;
+
+	/* This spins until the underlying notification is generated */
+	while (ioctl(listener, SECCOMP_IOCTL_NOTIF_ADDFD, &addfd) != -1 &&
+	       errno != -EINPROGRESS)
+		nanosleep(&delay, NULL);
+
+	memset(&req, 0, sizeof(req));
+	ASSERT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
+	ASSERT_EQ(addfd.id, req.id);
+
 	resp.id = req.id;
 	resp.error = 0;
 	resp.val = USER_NOTIF_MAGIC;
@@ -4116,6 +4150,10 @@ TEST(user_notification_addfd_rlimit)
 	EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_ADDFD, &addfd), -1);
 	EXPECT_EQ(errno, EMFILE);
 
+	addfd.flags = SECCOMP_ADDFD_FLAG_SEND;
+	EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_ADDFD, &addfd), -1);
+	EXPECT_EQ(errno, EMFILE);
+
 	addfd.newfd = 100;
 	addfd.flags = SECCOMP_ADDFD_FLAG_SETFD;
 	EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_ADDFD, &addfd), -1);
-- 
2.25.1

Re: [PATCH v2 0/4] Atomic addfd send and reply

[Andy Lutomirski]

On 5/17/21 12:39 PM, Sargun Dhillon wrote:
> This is somewhat of a respin of "Handle seccomp notification preemption"
> but without the controversial parts.

Looks like a good solution to me, at least for this specific problem.

Re: [PATCH v2 0/4] Atomic addfd send and reply

[Christian Brauner]

On Mon, May 17, 2021 at 12:39:04PM -0700, Sargun Dhillon wrote:
> This is somewhat of a respin of "Handle seccomp notification preemption"
> but without the controversial parts.
> 
> 
> This patchset addresses a race condition we've dealt with recently with
> seccomp. Specifically programs interrupting syscalls while they're in
> progress. This was exacerbated by Golang's recent adoption of "async
> preemption", in which they try to interrupt any syscall that's been running
> for more than 10ms during GC. During certain syscalls, it's non-trivial to
> write them in a reetrant manner in userspace (socket).
> 
> It focuses on one use cases, which is adding file descriptors to a process
> "atomically" during the seccomp reply, as opposed to discretizing the calls
> which may result in a potential file descriptor leak and inconsistent
> program state.

Looks good,
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>

Re: [PATCH v2 3/4] seccomp: Support atomic "addfd + send reply"

[Christian Brauner]

On Mon, May 17, 2021 at 12:39:07PM -0700, Sargun Dhillon wrote:
> From: Rodrigo Campos <rodrigo@kinvolk.io>
> 
> Alban Crequy reported a race condition userspace faces when we want to
> add some fds and make the syscall return them[1] using seccomp notify.
> 
> The problem is that currently two different ioctl() calls are needed by
> the process handling the syscalls (agent) for another userspace process
> (target): SECCOMP_IOCTL_NOTIF_ADDFD to allocate the fd and
> SECCOMP_IOCTL_NOTIF_SEND to return that value. 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 Cook[2]. 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.
> 
> This "allocate an fd and return" functionality is useful for syscalls
> that return a file descriptor only, like connect(2). Other syscalls that
> return a file descriptor but not as return value (or return more than
> one fd), like socketpair(), pipe(), recvmsg with SCM_RIGHTs, will not
> work with this flag.
> 
> This effectively combines SECCOMP_IOCTL_NOTIF_ADDFD and
> SECCOMP_IOCTL_NOTIF_SEND into an atomic opteration. The notification's
> return value, nor error can be set by the user. Upon successful invocation
> of the SECCOMP_IOCTL_NOTIF_ADDFD ioctl with the SECCOMP_ADDFD_FLAG_SEND
> flag, the notifying process's errno will be 0, and the return value will
> be the file descriptor number that was installed.
> 
> [1]: https://lore.kernel.org/lkml/CADZs7q4sw71iNHmV8EOOXhUKJMORPzF7thraxZYddTZsxta-KQ@mail.gmail.com/
> [2]: https://lore.kernel.org/lkml/202012011322.26DCBC64F2@keescook/
> 
> Signed-off-by: Rodrigo Campos <rodrigo@kinvolk.io>
> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> Acked-by: Tycho Andersen <tycho@tycho.pizza>
> ---
>  .../userspace-api/seccomp_filter.rst          | 12 +++++
>  include/uapi/linux/seccomp.h                  |  1 +
>  kernel/seccomp.c                              | 49 +++++++++++++++++--
>  3 files changed, 58 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/userspace-api/seccomp_filter.rst b/Documentation/userspace-api/seccomp_filter.rst
> index 6efb41cc8072..d61219889e49 100644
> --- a/Documentation/userspace-api/seccomp_filter.rst
> +++ b/Documentation/userspace-api/seccomp_filter.rst
> @@ -259,6 +259,18 @@ and ``ioctl(SECCOMP_IOCTL_NOTIF_SEND)`` a response, indicating what should be
>  returned to userspace. The ``id`` member of ``struct seccomp_notif_resp`` should
>  be the same ``id`` as in ``struct seccomp_notif``.
>  
> +Userspace can also add file descriptors to the notifying process via
> +``ioctl(SECCOMP_IOCTL_NOTIF_ADDFD)``. The ``id`` member of
> +``struct seccomp_notif_addfd`` should be the same ``id`` as in
> +``struct seccomp_notif``. The ``newfd_flags`` flag may be used to set flags
> +like O_EXEC on the file descriptor in the notifying process. If the supervisor

nit:
s/O_EXEC/O_CLOEXEC/

Re: [PATCH v2 2/4] seccomp: Refactor notification handler to prepare for new semantics

[Rodrigo Campos]

On Mon, May 17, 2021 at 9:39 PM Sargun Dhillon <sargun@sargun.me> wrote:
>
> This refactors the user notification code to have a do / while loop around
> the completion condition. This has a small change in semantic, in that
> previously we ignored addfd calls upon wakeup if the notification had been
> responded to, but instead with the new change we check for an outstanding
> addfd calls prior to returning to userspace.

I understand why this was a readability improvement on the old
patchset (that included the wait_killable semantics), as it completely
changed the loop. But now we only have the atomic addfd+send reply
that does minimal changes to this part (add a param to a function).

Is it worth changing the semantics?

> Rodrigo Campos also identified a bug that can result in addfd causing
> an early return, when the supervisor didn't actually handle the
> syscall [1].
>
> [1]: https://lore.kernel.org/lkml/20210413160151.3301-1-rodrigo@kinvolk.io/

I was about to resend this, but I'd like to know what others think.

I'm okay with applying any patches to solve the issue (mine linked
there or this one), slightly in favor of mine as the diff is way
simpler to backport (applies to 5.9+ kernels) and I don't see a reason
to change semantics. But no strong opinion.

Opinions?


Best,
Rodrigo

Re: [PATCH v2 2/4] seccomp: Refactor notification handler to prepare for new semantics

[Sargun Dhillon]

On Tue, May 25, 2021 at 9:04 AM Rodrigo Campos <rodrigo@kinvolk.io> wrote:
>
> On Mon, May 17, 2021 at 9:39 PM Sargun Dhillon <sargun@sargun.me> wrote:
> >
> > This refactors the user notification code to have a do / while loop around
> > the completion condition. This has a small change in semantic, in that
> > previously we ignored addfd calls upon wakeup if the notification had been
> > responded to, but instead with the new change we check for an outstanding
> > addfd calls prior to returning to userspace.
>
> I understand why this was a readability improvement on the old
> patchset (that included the wait_killable semantics), as it completely
> changed the loop. But now we only have the atomic addfd+send reply
> that does minimal changes to this part (add a param to a function).
>
> Is it worth changing the semantics?
>
I think that as we add more complexity around different things that
can cause the notification to change (status), that this is better,
but I understand wanting to hold off.

> > Rodrigo Campos also identified a bug that can result in addfd causing
> > an early return, when the supervisor didn't actually handle the
> > syscall [1].
> >
> > [1]: https://lore.kernel.org/lkml/20210413160151.3301-1-rodrigo@kinvolk.io/
>
> I was about to resend this, but I'd like to know what others think.
>
> I'm okay with applying any patches to solve the issue (mine linked
> there or this one), slightly in favor of mine as the diff is way
> simpler to backport (applies to 5.9+ kernels) and I don't see a reason
> to change semantics. But no strong opinion.
>
> Opinions?
>
>
> Best,
> Rodrigo

Re: [PATCH v2 0/4] Atomic addfd send and reply

[Kees Cook]

On Mon, 17 May 2021 12:39:04 -0700, Sargun Dhillon wrote:
> This is somewhat of a respin of "Handle seccomp notification preemption"
> but without the controversial parts.
> 
> 
> This patchset addresses a race condition we've dealt with recently with
> seccomp. Specifically programs interrupting syscalls while they're in
> progress. This was exacerbated by Golang's recent adoption of "async
> preemption", in which they try to interrupt any syscall that's been running
> for more than 10ms during GC. During certain syscalls, it's non-trivial to
> write them in a reetrant manner in userspace (socket).
> 
> [...]

Thanks for your patience on this series! I think this is a clear solution
for the race. Applied to for-next/seccomp, thanks!

[1/4] Documentation: seccomp: Fix user notification documentation
      https://git.kernel.org/kees/c/1e2ca403fa89
[2/4] seccomp: Refactor notification handler to prepare for new semantics
      https://git.kernel.org/kees/c/6a1e0616acde
[3/4] seccomp: Support atomic "addfd + send reply"
      https://git.kernel.org/kees/c/ba9ef89cf83e
[4/4] selftests/seccomp: Add test for atomic addfd+send
      https://git.kernel.org/kees/c/75c98a0d5d3a

-- 
Kees Cook

Re: [PATCH v2 2/4] seccomp: Refactor notification handler to prepare for new semantics

[Rodrigo Campos]

On Mon, May 17, 2021 at 9:39 PM Sargun Dhillon <sargun@sargun.me> wrote:
>
> This refactors the user notification code to have a do / while loop around
> the completion condition. This has a small change in semantic, in that
> previously we ignored addfd calls upon wakeup if the notification had been
> responded to, but instead with the new change we check for an outstanding
> addfd calls prior to returning to userspace.
>
> Rodrigo Campos also identified a bug that can result in addfd causing
> an early return, when the supervisor didn't actually handle the
> syscall [1].
>
> [1]: https://lore.kernel.org/lkml/20210413160151.3301-1-rodrigo@kinvolk.io/
>
> Fixes: 7cf97b125455 ("seccomp: Introduce addfd ioctl to seccomp user notifier")
> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> Acked-by: Tycho Andersen <tycho@tycho.pizza>

Kees, as I mentioned in the linked thread, this issue is present in
5.9+ kernels. Should we add the cc to stable for this patch? Or should
we cc to stable the one linked, that just fixes the issue without
semantic changes to userspace?

Just to be clear, the other patch that fixes the problem without
userspace visible changes is this:
https://lore.kernel.org/lkml/20210413160151.3301-1-rodrigo@kinvolk.io/


Best,
Rodrigo

Re: [PATCH v2 2/4] seccomp: Refactor notification handler to prepare for new semantics

[Kees Cook]

On Thu, May 27, 2021 at 01:51:13PM +0200, Rodrigo Campos wrote:
> On Mon, May 17, 2021 at 9:39 PM Sargun Dhillon <sargun@sargun.me> wrote:
> >
> > This refactors the user notification code to have a do / while loop around
> > the completion condition. This has a small change in semantic, in that
> > previously we ignored addfd calls upon wakeup if the notification had been
> > responded to, but instead with the new change we check for an outstanding
> > addfd calls prior to returning to userspace.
> >
> > Rodrigo Campos also identified a bug that can result in addfd causing
> > an early return, when the supervisor didn't actually handle the
> > syscall [1].
> >
> > [1]: https://lore.kernel.org/lkml/20210413160151.3301-1-rodrigo@kinvolk.io/
> >
> > Fixes: 7cf97b125455 ("seccomp: Introduce addfd ioctl to seccomp user notifier")
> > Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> > Acked-by: Tycho Andersen <tycho@tycho.pizza>
> 
> Kees, as I mentioned in the linked thread, this issue is present in
> 5.9+ kernels. Should we add the cc to stable for this patch? Or should
> we cc to stable the one linked, that just fixes the issue without
> semantic changes to userspace?

It sounds like the problem is with Go, using addfd, on 5.9-5.13 kernels,
yes? Would the semantic change be a problem there? (i.e. it sounds like
the semantic change was fine for the 5.14+ kernels, so I'm assuming it's
fine for earlier ones too.)

> Just to be clear, the other patch that fixes the problem without
> userspace visible changes is this:
> https://lore.kernel.org/lkml/20210413160151.3301-1-rodrigo@kinvolk.io/

I'd prefer to use the now-in-next fix if we can. Is it possible to build
a test case that triggers the race so we can have some certainty that
any fix in -stable covers it appropriately?

-Kees

-- 
Kees Cook

Re: [PATCH v2 2/4] seccomp: Refactor notification handler to prepare for new semantics

[Rodrigo Campos]

On Thu, May 27, 2021 at 8:42 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, May 27, 2021 at 01:51:13PM +0200, Rodrigo Campos wrote:
> >
> > Kees, as I mentioned in the linked thread, this issue is present in
> > 5.9+ kernels. Should we add the cc to stable for this patch? Or should
> > we cc to stable the one linked, that just fixes the issue without
> > semantic changes to userspace?
>
> It sounds like the problem is with Go, using addfd, on 5.9-5.13 kernels,
> yes?

Yes.

> Would the semantic change be a problem there? (i.e. it sounds like
> the semantic change was fine for the 5.14+ kernels, so I'm assuming it's
> fine for earlier ones too.)

No, I don't think it will cause any problem.

> > Just to be clear, the other patch that fixes the problem without
> > userspace visible changes is this:
> > https://lore.kernel.org/lkml/20210413160151.3301-1-rodrigo@kinvolk.io/
>
> I'd prefer to use the now-in-next fix if we can. Is it possible to build
> a test case that triggers the race so we can have some certainty that
> any fix in -stable covers it appropriately?

I've verified that Sargun's patch also solves the problem in mainline.
I have now also verified that it applies cleany and fixes the issue
for linux-stable/5.10.y and linux-stable/5.12.y too (without the patch
I see the problem, with the patch I don't see it).  5.11 is already
EOL, so I didn't try it (probably will work as well).

The test case that I have is quite a complicated one, though. I'm
using the PR we opened to runc to add support for seccomp notify[1]
and a seccomp agent slightly modified from the example in the PR with
some cgo to use addfd, and need to run it for several thousand
iterations, as the kernel needs to be interrupted in a specific line
and some kernel locks to be acquired in a specific order for this to
trigger. If you think it is important, I can try to cleanup the code
and share it, but the issue is basically what I explained here:
https://lore.kernel.org/lkml/20210413160151.3301-2-rodrigo@kinvolk.io/

Can we cc this patch to stable, then? :)


Best,
Rodrigo

[1]: https://github.com/opencontainers/runc/pull/2682

Re: [PATCH v2 2/4] seccomp: Refactor notification handler to prepare for new semantics

[Kees Cook]

On Fri, May 28, 2021 at 05:27:39PM +0200, Rodrigo Campos wrote:
> On Thu, May 27, 2021 at 8:42 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Thu, May 27, 2021 at 01:51:13PM +0200, Rodrigo Campos wrote:
> > >
> > > Kees, as I mentioned in the linked thread, this issue is present in
> > > 5.9+ kernels. Should we add the cc to stable for this patch? Or should
> > > we cc to stable the one linked, that just fixes the issue without
> > > semantic changes to userspace?
> >
> > It sounds like the problem is with Go, using addfd, on 5.9-5.13 kernels,
> > yes?
> 
> Yes.
> 
> > Would the semantic change be a problem there? (i.e. it sounds like
> > the semantic change was fine for the 5.14+ kernels, so I'm assuming it's
> > fine for earlier ones too.)
> 
> No, I don't think it will cause any problem.
> 
> > > Just to be clear, the other patch that fixes the problem without
> > > userspace visible changes is this:
> > > https://lore.kernel.org/lkml/20210413160151.3301-1-rodrigo@kinvolk.io/
> >
> > I'd prefer to use the now-in-next fix if we can. Is it possible to build
> > a test case that triggers the race so we can have some certainty that
> > any fix in -stable covers it appropriately?
> 
> I've verified that Sargun's patch also solves the problem in mainline.
> I have now also verified that it applies cleany and fixes the issue
> for linux-stable/5.10.y and linux-stable/5.12.y too (without the patch
> I see the problem, with the patch I don't see it).  5.11 is already
> EOL, so I didn't try it (probably will work as well).

Great! Thanks for doing that testing.

> The test case that I have is quite a complicated one, though. I'm
> using the PR we opened to runc to add support for seccomp notify[1]
> and a seccomp agent slightly modified from the example in the PR with
> some cgo to use addfd, and need to run it for several thousand
> iterations, as the kernel needs to be interrupted in a specific line
> and some kernel locks to be acquired in a specific order for this to
> trigger. If you think it is important, I can try to cleanup the code
> and share it, but the issue is basically what I explained here:
> https://lore.kernel.org/lkml/20210413160151.3301-2-rodrigo@kinvolk.io/

Okay; yeah, sounds like that'll be hard to port sanely. :)

> Can we cc this patch to stable, then? :)

Yup, sounds good to me. I will adjust the tags.

Thanks!

-Kees

> Best,
> Rodrigo
> 
> [1]: https://github.com/opencontainers/runc/pull/2682

-- 
Kees Cook

Re: [PATCH v2 2/4] seccomp: Refactor notification handler to prepare for new semantics

[Kees Cook]

On Fri, May 28, 2021 at 05:27:39PM +0200, Rodrigo Campos wrote:
> On Thu, May 27, 2021 at 8:42 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Thu, May 27, 2021 at 01:51:13PM +0200, Rodrigo Campos wrote:
> > >
> > > Kees, as I mentioned in the linked thread, this issue is present in
> > > 5.9+ kernels. Should we add the cc to stable for this patch? Or should
> > > we cc to stable the one linked, that just fixes the issue without
> > > semantic changes to userspace?
> >
> > It sounds like the problem is with Go, using addfd, on 5.9-5.13 kernels,
> > yes?
> 
> Yes.
> 
> > Would the semantic change be a problem there? (i.e. it sounds like
> > the semantic change was fine for the 5.14+ kernels, so I'm assuming it's
> > fine for earlier ones too.)
> 
> No, I don't think it will cause any problem.
> 
> > > Just to be clear, the other patch that fixes the problem without
> > > userspace visible changes is this:
> > > https://lore.kernel.org/lkml/20210413160151.3301-1-rodrigo@kinvolk.io/
> >
> > I'd prefer to use the now-in-next fix if we can. Is it possible to build
> > a test case that triggers the race so we can have some certainty that
> > any fix in -stable covers it appropriately?
> 
> I've verified that Sargun's patch also solves the problem in mainline.
> I have now also verified that it applies cleany and fixes the issue
> for linux-stable/5.10.y and linux-stable/5.12.y too (without the patch
> I see the problem, with the patch I don't see it).  5.11 is already
> EOL, so I didn't try it (probably will work as well).

Oh, btw, may I add a Tested-by: from you for this fix?

Thanks!

-Kees

-- 
Kees Cook

Re: [PATCH v2 2/4] seccomp: Refactor notification handler to prepare for new semantics

[Rodrigo Campos]

On Fri, May 28, 2021 at 7:14 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Fri, May 28, 2021 at 05:27:39PM +0200, Rodrigo Campos wrote:
> > On Thu, May 27, 2021 at 8:42 PM Kees Cook <keescook@chromium.org> wrote:
> > >
> > > On Thu, May 27, 2021 at 01:51:13PM +0200, Rodrigo Campos wrote:
> > > >
> > > > Kees, as I mentioned in the linked thread, this issue is present in
> > > > 5.9+ kernels. Should we add the cc to stable for this patch? Or should
> > > > we cc to stable the one linked, that just fixes the issue without
> > > > semantic changes to userspace?
> > >
> > > It sounds like the problem is with Go, using addfd, on 5.9-5.13 kernels,
> > > yes?
> >
> > Yes.
> >
> > > Would the semantic change be a problem there? (i.e. it sounds like
> > > the semantic change was fine for the 5.14+ kernels, so I'm assuming it's
> > > fine for earlier ones too.)
> >
> > No, I don't think it will cause any problem.
> >
> > > > Just to be clear, the other patch that fixes the problem without
> > > > userspace visible changes is this:
> > > > https://lore.kernel.org/lkml/20210413160151.3301-1-rodrigo@kinvolk.io/
> > >
> > > I'd prefer to use the now-in-next fix if we can. Is it possible to build
> > > a test case that triggers the race so we can have some certainty that
> > > any fix in -stable covers it appropriately?
> >
> > I've verified that Sargun's patch also solves the problem in mainline.
> > I have now also verified that it applies cleany and fixes the issue
> > for linux-stable/5.10.y and linux-stable/5.12.y too (without the patch
> > I see the problem, with the patch I don't see it).  5.11 is already
> > EOL, so I didn't try it (probably will work as well).
>
> Oh, btw, may I add a Tested-by: from you for this fix?

Oh, right! Yes. Here it goes so it's simpler to add :)

Tested-by: Rodrigo Campos <rodrigo@kinvolk.io>


Thanks!

Re: [PATCH v2 2/4] seccomp: Refactor notification handler to prepare for new semantics

[Kees Cook]

On Sat, May 29, 2021 at 12:31:55AM +0200, Rodrigo Campos wrote:
> On Fri, May 28, 2021 at 7:14 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Fri, May 28, 2021 at 05:27:39PM +0200, Rodrigo Campos wrote:
> > > On Thu, May 27, 2021 at 8:42 PM Kees Cook <keescook@chromium.org> wrote:
> > > >
> > > > On Thu, May 27, 2021 at 01:51:13PM +0200, Rodrigo Campos wrote:
> > > > >
> > > > > Kees, as I mentioned in the linked thread, this issue is present in
> > > > > 5.9+ kernels. Should we add the cc to stable for this patch? Or should
> > > > > we cc to stable the one linked, that just fixes the issue without
> > > > > semantic changes to userspace?
> > > >
> > > > It sounds like the problem is with Go, using addfd, on 5.9-5.13 kernels,
> > > > yes?
> > >
> > > Yes.
> > >
> > > > Would the semantic change be a problem there? (i.e. it sounds like
> > > > the semantic change was fine for the 5.14+ kernels, so I'm assuming it's
> > > > fine for earlier ones too.)
> > >
> > > No, I don't think it will cause any problem.
> > >
> > > > > Just to be clear, the other patch that fixes the problem without
> > > > > userspace visible changes is this:
> > > > > https://lore.kernel.org/lkml/20210413160151.3301-1-rodrigo@kinvolk.io/
> > > >
> > > > I'd prefer to use the now-in-next fix if we can. Is it possible to build
> > > > a test case that triggers the race so we can have some certainty that
> > > > any fix in -stable covers it appropriately?
> > >
> > > I've verified that Sargun's patch also solves the problem in mainline.
> > > I have now also verified that it applies cleany and fixes the issue
> > > for linux-stable/5.10.y and linux-stable/5.12.y too (without the patch
> > > I see the problem, with the patch I don't see it).  5.11 is already
> > > EOL, so I didn't try it (probably will work as well).
> >
> > Oh, btw, may I add a Tested-by: from you for this fix?
> 
> Oh, right! Yes. Here it goes so it's simpler to add :)
> 
> Tested-by: Rodrigo Campos <rodrigo@kinvolk.io>

Thanks; this is in v5.13-rc4 now:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ddc473916955f7710d1eb17c1273d91c8622a9fe

-- 
Kees Cook