[PATCH 0/4] Add the ability to get a pidfd on seccomp user notifications

[PATCH 0/4] Add the ability to get a pidfd on seccomp user notifications

[Sargun Dhillon]

This patchset adds the ability for users of the seccomp user notification API
to receive the pidfd of the process which triggered the notification. It is
an optional feature that users must opt into by setting a flag when they
call the ioctl. With enhancements to other APIs, it should decrease
the need for the cookie-checking mechanism.

Sargun Dhillon (4):
  pid: Add pidfd_create_file helper
  fork: Use newly created pidfd_create_file helper
  seccomp: Add SECCOMP_USER_NOTIF_FLAG_PIDFD to get pidfd on listener
    trap
  selftests/seccomp: test SECCOMP_USER_NOTIF_FLAG_PIDFD

 include/linux/pid.h                           |   1 +
 include/uapi/linux/seccomp.h                  |   4 +
 kernel/fork.c                                 |   4 +-
 kernel/pid.c                                  |  22 ++++
 kernel/seccomp.c                              |  68 ++++++++++-
 tools/testing/selftests/seccomp/seccomp_bpf.c | 110 ++++++++++++++++++
 6 files changed, 200 insertions(+), 9 deletions(-)

-- 
2.20.1

[PATCH 1/4] pid: Add pidfd_create_file helper

[Sargun Dhillon]

This helper allow for creation of pidfd files. The existing helper
(pidfd_create) creates file descriptors directly, which cannot
be used without race conditions when there is an intermediate
step between creation, and informing userspace the fd has been
created.

Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
 include/linux/pid.h |  1 +
 kernel/pid.c        | 22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/include/linux/pid.h b/include/linux/pid.h
index 998ae7d24450..70d4725cf8da 100644
--- a/include/linux/pid.h
+++ b/include/linux/pid.h
@@ -75,6 +75,7 @@ extern const struct file_operations pidfd_fops;
 struct file;
 
 extern struct pid *pidfd_pid(const struct file *file);
+extern struct file *pidfd_create_file(struct pid *pid);
 
 static inline struct pid *get_pid(struct pid *pid)
 {
diff --git a/kernel/pid.c b/kernel/pid.c
index 2278e249141d..2a34db290128 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -521,6 +521,28 @@ static int pidfd_create(struct pid *pid)
 	return fd;
 }
 
+/**
+ * pidfd_create_file() - Create a new pidfd file.
+ *
+ * @pid:  struct pid that the pidfd will reference
+ *
+ * This creates a new pidfd file.
+ *
+ * Return: On success, a cloexec pidfd file is returned
+ *         On error, an err ptr will be returned.
+ */
+struct file *pidfd_create_file(struct pid *pid)
+{
+	struct file *f;
+
+	f = anon_inode_getfile("[pidfd]", &pidfd_fops, get_pid(pid),
+			       O_RDWR | O_CLOEXEC);
+	if (IS_ERR(f))
+		put_pid(pid);
+
+	return f;
+}
+
 /**
  * pidfd_open() - Open new pid file descriptor.
  *
-- 
2.20.1

[PATCH 2/4] fork: Use newly created pidfd_create_file helper

[Sargun Dhillon]

Rather than duplicating the code to create a pidfd_file in kernel/fork.c,
use the helper in kernel/pid.c.

Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
 kernel/fork.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index 080809560072..181ab2958cad 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2106,14 +2106,12 @@ static __latent_entropy struct task_struct *copy_process(
 
 		pidfd = retval;
 
-		pidfile = anon_inode_getfile("[pidfd]", &pidfd_fops, pid,
-					      O_RDWR | O_CLOEXEC);
+		pidfile = pidfd_create_file(pid);
 		if (IS_ERR(pidfile)) {
 			put_unused_fd(pidfd);
 			retval = PTR_ERR(pidfile);
 			goto bad_fork_free_pid;
 		}
-		get_pid(pid);	/* held by pidfile now */
 
 		retval = put_user(pidfd, args->pidfd);
 		if (retval)
-- 
2.20.1

[PATCH 3/4] seccomp: Add SECCOMP_USER_NOTIF_FLAG_PIDFD to get pidfd on listener trap

[Sargun Dhillon]

This introduces the capability for users of seccomp's listener behaviour
to be able to receive the pidfd of the process that triggered the event.
Currently, this just opens the group leader of the thread that triggere
the event, as pidfds (currently) are limited to group leaders.

For actions which do not act on the process outside of the pidfd, there
is then no need to check the cookie to ensure validity of the request
throughout the listener's handling of it.

This can be extended later on as well when pidfd capabilities are added
to be able to have the listener imbue the pidfd with certain capabilities
when it is delivered to userspace.

It is the responsibility of the user to close the pidfd.

Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
 include/uapi/linux/seccomp.h |  4 +++
 kernel/seccomp.c             | 68 ++++++++++++++++++++++++++++++++----
 2 files changed, 66 insertions(+), 6 deletions(-)

diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
index be84d87f1f46..64f6fc5c95f1 100644
--- a/include/uapi/linux/seccomp.h
+++ b/include/uapi/linux/seccomp.h
@@ -69,11 +69,15 @@ struct seccomp_notif_sizes {
 	__u16 seccomp_data;
 };
 
+/* Valid flags for struct seccomp_notif */
+#define SECCOMP_USER_NOTIF_FLAG_PIDFD	(1UL << 0) /* populate pidfd */
+
 struct seccomp_notif {
 	__u64 id;
 	__u32 pid;
 	__u32 flags;
 	struct seccomp_data data;
+	__u32 pidfd;
 };
 
 /*
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index b6ea3dcb57bf..93f9cf45ce07 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -1019,21 +1019,61 @@ static int seccomp_notify_release(struct inode *inode, struct file *file)
 	return 0;
 }
 
+
+static long __seccomp_notify_recv_pidfd(void __user *buf,
+					struct seccomp_notif *unotif,
+					struct task_struct *group_leader)
+{
+	struct file *pidfd_file;
+	struct pid *pid;
+	int fd;
+
+	pid = get_task_pid(group_leader, PIDTYPE_PID);
+	pidfd_file = pidfd_create_file(pid);
+	put_pid(pid);
+	if (IS_ERR(pidfd_file))
+		return PTR_ERR(pidfd_file);
+
+	fd = get_unused_fd_flags(O_RDWR | O_CLOEXEC);
+	if (fd < 0) {
+		fput(pidfd_file);
+		return fd;
+	}
+
+	unotif->pidfd = fd;
+
+	if (copy_to_user(buf, unotif, sizeof(*unotif))) {
+		put_unused_fd(fd);
+		fput(pidfd_file);
+		return -EFAULT;
+	}
+
+	fd_install(fd, pidfd_file);
+
+	return 0;
+}
+
 static long seccomp_notify_recv(struct seccomp_filter *filter,
 				void __user *buf)
 {
 	struct seccomp_knotif *knotif = NULL, *cur;
 	struct seccomp_notif unotif;
+	struct task_struct *group_leader;
+	bool send_pidfd;
 	ssize_t ret;
 
+	if (copy_from_user(&unotif, buf, sizeof(unotif)))
+		return -EFAULT;
 	/* Verify that we're not given garbage to keep struct extensible. */
-	ret = check_zeroed_user(buf, sizeof(unotif));
-	if (ret < 0)
-		return ret;
-	if (!ret)
+	if (unotif.id ||
+	    unotif.pid ||
+	    memchr_inv(&unotif.data, 0, sizeof(unotif.data)) ||
+	    unotif.pidfd)
+		return -EINVAL;
+	if (unotif.flags & ~(SECCOMP_USER_NOTIF_FLAG_PIDFD))
 		return -EINVAL;
 
-	memset(&unotif, 0, sizeof(unotif));
+	send_pidfd = unotif.flags & SECCOMP_USER_NOTIF_FLAG_PIDFD;
 
 	ret = down_interruptible(&filter->notif->request);
 	if (ret < 0)
@@ -1057,9 +1097,13 @@ static long seccomp_notify_recv(struct seccomp_filter *filter,
 		goto out;
 	}
 
+	memset(&unotif, 0, sizeof(unotif));
+
 	unotif.id = knotif->id;
 	unotif.pid = task_pid_vnr(knotif->task);
 	unotif.data = *(knotif->data);
+	if (send_pidfd)
+		group_leader = get_task_struct(knotif->task->group_leader);
 
 	knotif->state = SECCOMP_NOTIFY_SENT;
 	wake_up_poll(&filter->notif->wqh, EPOLLOUT | EPOLLWRNORM);
@@ -1067,9 +1111,21 @@ static long seccomp_notify_recv(struct seccomp_filter *filter,
 out:
 	mutex_unlock(&filter->notify_lock);
 
-	if (ret == 0 && copy_to_user(buf, &unotif, sizeof(unotif))) {
+	if (ret)
+		return ret;
+
+	/*
+	 * We've successfully received a notification, let's try to copy it to
+	 * userspace.
+	 */
+	if (send_pidfd) {
+		ret = __seccomp_notify_recv_pidfd(buf, &unotif, group_leader);
+		put_task_struct(group_leader);
+	} else if (copy_to_user(buf, &unotif, sizeof(unotif))) {
 		ret = -EFAULT;
+	}
 
+	if (ret) {
 		/*
 		 * Userspace screwed up. To make sure that we keep this
 		 * notification alive, let's reset it back to INIT. It
-- 
2.20.1

Re: [PATCH 3/4] seccomp: Add SECCOMP_USER_NOTIF_FLAG_PIDFD to get pidfd on listener trap

[Tycho Andersen]

On Fri, Jan 24, 2020 at 01:17:42AM -0800, Sargun Dhillon wrote:
> Currently, this just opens the group leader of the thread that triggere
> the event, as pidfds (currently) are limited to group leaders.

I don't love the semantics of this; when they're not limited to thread
group leaders any more, we won't be able to change this. Is that work
far off?

Tycho

Re: [PATCH 3/4] seccomp: Add SECCOMP_USER_NOTIF_FLAG_PIDFD to get pidfd on listener trap

[Sargun Dhillon]

On Fri, Jan 24, 2020 at 10:03 AM Tycho Andersen <tycho@tycho.ws> wrote:
>
> On Fri, Jan 24, 2020 at 01:17:42AM -0800, Sargun Dhillon wrote:
> > Currently, this just opens the group leader of the thread that triggere
> > the event, as pidfds (currently) are limited to group leaders.
>
> I don't love the semantics of this; when they're not limited to thread
> group leaders any more, we won't be able to change this. Is that work
> far off?
>
> Tycho

We would be able to change this in the future if we introduced a flag like
SECCOMP_USER_NOTIF_FLAG_PIDFD_THREAD which would send a
pidfd that's for the thread, and not just the group leader. The flag could
either be XOR with SECCOMP_USER_NOTIF_FLAG_PIDFD, or
could require both. Alternatively, we can rename
SECCOMP_USER_NOTIF_FLAG_PIDFD to
SECCOMP_USER_NOTIF_FLAG_GROUP_LEADER_PIDFD.

Re: [PATCH 3/4] seccomp: Add SECCOMP_USER_NOTIF_FLAG_PIDFD to get pidfd on listener trap

[Aleksa Sarai]

[-- Attachment #1: Type: text/plain, Size: 1736 bytes --]

On 2020-01-24, Sargun Dhillon <sargun@sargun.me> wrote:
> On Fri, Jan 24, 2020 at 10:03 AM Tycho Andersen <tycho@tycho.ws> wrote:
> >
> > On Fri, Jan 24, 2020 at 01:17:42AM -0800, Sargun Dhillon wrote:
> > > Currently, this just opens the group leader of the thread that triggere
> > > the event, as pidfds (currently) are limited to group leaders.
> >
> > I don't love the semantics of this; when they're not limited to thread
> > group leaders any more, we won't be able to change this. Is that work
> > far off?
> >
> > Tycho
> 
> We would be able to change this in the future if we introduced a flag like
> SECCOMP_USER_NOTIF_FLAG_PIDFD_THREAD which would send a
> pidfd that's for the thread, and not just the group leader. The flag could
> either be XOR with SECCOMP_USER_NOTIF_FLAG_PIDFD, or
> could require both. Alternatively, we can rename
> SECCOMP_USER_NOTIF_FLAG_PIDFD to
> SECCOMP_USER_NOTIF_FLAG_GROUP_LEADER_PIDFD.

Possibly unpopular proposal -- would it make sense to just store the
pidfd_open(2) flags rather than coming up with our own set for
SECCOMP_USER_NOTIF? If/when pidfds are expanded to include non-leaders
there will be a corresponding flag for pidfd_open(2). Something like:

	struct seccomp_notif {
		__u64 id;
		__u32 pid;
		__u32 flags;
		struct seccomp_data data;
		__u64 pidfd_flags; // or __u32 -- not sure what Christian plans
		__u32 pidfd;
		__u32 __padding;
	};

This does mean there'll be an additional flags field, but I think it's a
slightly more consistent way to indicate "SECCOMP_USER_NOTIF_FLAG_PIDFD
implies a pidfd_open(2) on the traced task".

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 3/4] seccomp: Add SECCOMP_USER_NOTIF_FLAG_PIDFD to get pidfd on listener trap

[Tycho Andersen]

On Fri, Jan 24, 2020 at 12:09:37PM -0800, Sargun Dhillon wrote:
> On Fri, Jan 24, 2020 at 10:03 AM Tycho Andersen <tycho@tycho.ws> wrote:
> >
> > On Fri, Jan 24, 2020 at 01:17:42AM -0800, Sargun Dhillon wrote:
> > > Currently, this just opens the group leader of the thread that triggere
> > > the event, as pidfds (currently) are limited to group leaders.
> >
> > I don't love the semantics of this; when they're not limited to thread
> > group leaders any more, we won't be able to change this. Is that work
> > far off?
> >
> > Tycho
> 
> We would be able to change this in the future if we introduced a flag like
> SECCOMP_USER_NOTIF_FLAG_PIDFD_THREAD which would send a
> pidfd that's for the thread, and not just the group leader. The flag could
> either be XOR with SECCOMP_USER_NOTIF_FLAG_PIDFD, or
> could require both. Alternatively, we can rename
> SECCOMP_USER_NOTIF_FLAG_PIDFD to
> SECCOMP_USER_NOTIF_FLAG_GROUP_LEADER_PIDFD.

Ok, but then isn't this just another temporary API? Seems like it's
worth waiting until the Right Way exists.

Tycho

Re: [PATCH 3/4] seccomp: Add SECCOMP_USER_NOTIF_FLAG_PIDFD to get pidfd on listener trap

[Christian Brauner]

On Fri, May 15, 2020 at 04:49:14AM -0700, Sargun Dhillon wrote:
> On Sat, Jan 25, 2020 at 9:42 PM Tycho Andersen <tycho@tycho.ws> wrote:
> 
> > On Fri, Jan 24, 2020 at 12:09:37PM -0800, Sargun Dhillon wrote:
> > > On Fri, Jan 24, 2020 at 10:03 AM Tycho Andersen <tycho@tycho.ws> wrote:
> > > >
> > > > On Fri, Jan 24, 2020 at 01:17:42AM -0800, Sargun Dhillon wrote:
> > > > > Currently, this just opens the group leader of the thread that
> > triggere
> > > > > the event, as pidfds (currently) are limited to group leaders.
> > > >
> > > > I don't love the semantics of this; when they're not limited to thread
> > > > group leaders any more, we won't be able to change this. Is that work
> > > > far off?
> > > >
> > > > Tycho
> > >
> > > We would be able to change this in the future if we introduced a flag
> > like
> > > SECCOMP_USER_NOTIF_FLAG_PIDFD_THREAD which would send a
> > > pidfd that's for the thread, and not just the group leader. The flag
> > could
> > > either be XOR with SECCOMP_USER_NOTIF_FLAG_PIDFD, or
> > > could require both. Alternatively, we can rename
> > > SECCOMP_USER_NOTIF_FLAG_PIDFD to
> > > SECCOMP_USER_NOTIF_FLAG_GROUP_LEADER_PIDFD.
> >
> > Ok, but then isn't this just another temporary API? Seems like it's
> > worth waiting until the Right Way exists.
> >
> > Tycho
> >
> 
> It's been a few months. It does not appear like much progress has been made
> moving away from
> pidfd being only useful for leaders.
> 
> I would either like to respin this patch, or at a minimum, include the
> process group leader pid number
> in the seccomp notification, to simplify things for tracers.

I'd prefer if you went with the second option where you include the
process group leader pid number.
I'm against adding countless ways of producing pidfds through various
unrelated apis. The api is still quite fresh so I'd like to not overdo
it.

Christian

Re: [PATCH 3/4] seccomp: Add SECCOMP_USER_NOTIF_FLAG_PIDFD to get pidfd on listener trap

[Aleksa Sarai]

[-- Attachment #1: Type: text/plain, Size: 5718 bytes --]

On 2020-01-24, Sargun Dhillon <sargun@sargun.me> wrote:
> This introduces the capability for users of seccomp's listener behaviour
> to be able to receive the pidfd of the process that triggered the event.
> Currently, this just opens the group leader of the thread that triggere
> the event, as pidfds (currently) are limited to group leaders.
> 
> For actions which do not act on the process outside of the pidfd, there
> is then no need to check the cookie to ensure validity of the request
> throughout the listener's handling of it.
> 
> This can be extended later on as well when pidfd capabilities are added
> to be able to have the listener imbue the pidfd with certain capabilities
> when it is delivered to userspace.
> 
> It is the responsibility of the user to close the pidfd.
> 
> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> ---
>  include/uapi/linux/seccomp.h |  4 +++
>  kernel/seccomp.c             | 68 ++++++++++++++++++++++++++++++++----
>  2 files changed, 66 insertions(+), 6 deletions(-)
> 
> diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
> index be84d87f1f46..64f6fc5c95f1 100644
> --- a/include/uapi/linux/seccomp.h
> +++ b/include/uapi/linux/seccomp.h
> @@ -69,11 +69,15 @@ struct seccomp_notif_sizes {
>  	__u16 seccomp_data;
>  };
>  
> +/* Valid flags for struct seccomp_notif */
> +#define SECCOMP_USER_NOTIF_FLAG_PIDFD	(1UL << 0) /* populate pidfd */
> +
>  struct seccomp_notif {
>  	__u64 id;
>  	__u32 pid;
>  	__u32 flags;
>  	struct seccomp_data data;
> +	__u32 pidfd;
>  };
>  
>  /*
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index b6ea3dcb57bf..93f9cf45ce07 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -1019,21 +1019,61 @@ static int seccomp_notify_release(struct inode *inode, struct file *file)
>  	return 0;
>  }
>  
> +
> +static long __seccomp_notify_recv_pidfd(void __user *buf,
> +					struct seccomp_notif *unotif,
> +					struct task_struct *group_leader)
> +{
> +	struct file *pidfd_file;
> +	struct pid *pid;
> +	int fd;
> +
> +	pid = get_task_pid(group_leader, PIDTYPE_PID);
> +	pidfd_file = pidfd_create_file(pid);
> +	put_pid(pid);
> +	if (IS_ERR(pidfd_file))
> +		return PTR_ERR(pidfd_file);
> +
> +	fd = get_unused_fd_flags(O_RDWR | O_CLOEXEC);

You don't need to pass O_RDWR -- only O_CLOEXEC is checked by
get_unused_fd_flags().

> +	if (fd < 0) {
> +		fput(pidfd_file);
> +		return fd;
> +	}
> +
> +	unotif->pidfd = fd;
> +
> +	if (copy_to_user(buf, unotif, sizeof(*unotif))) {
> +		put_unused_fd(fd);
> +		fput(pidfd_file);
> +		return -EFAULT;
> +	}
> +
> +	fd_install(fd, pidfd_file);
> +
> +	return 0;
> +}
> +
>  static long seccomp_notify_recv(struct seccomp_filter *filter,
>  				void __user *buf)
>  {
>  	struct seccomp_knotif *knotif = NULL, *cur;
>  	struct seccomp_notif unotif;
> +	struct task_struct *group_leader;
> +	bool send_pidfd;
>  	ssize_t ret;
>  
> +	if (copy_from_user(&unotif, buf, sizeof(unotif)))
> +		return -EFAULT;
>  	/* Verify that we're not given garbage to keep struct extensible. */
> -	ret = check_zeroed_user(buf, sizeof(unotif));
> -	if (ret < 0)
> -		return ret;
> -	if (!ret)
> +	if (unotif.id ||
> +	    unotif.pid ||
> +	    memchr_inv(&unotif.data, 0, sizeof(unotif.data)) ||
> +	    unotif.pidfd)
> +		return -EINVAL;

IMHO this check is more confusing than the original check_zeroed_user().
Something like the following is simpler and less prone to forgetting to
add a new field in the future:

	if (memchr_inv(&unotif, 0, sizeof(unotif)))
		return -EINVAL;

> +	if (unotif.flags & ~(SECCOMP_USER_NOTIF_FLAG_PIDFD))
>  		return -EINVAL;
>  
> -	memset(&unotif, 0, sizeof(unotif));
> +	send_pidfd = unotif.flags & SECCOMP_USER_NOTIF_FLAG_PIDFD;
>  
>  	ret = down_interruptible(&filter->notif->request);
>  	if (ret < 0)
> @@ -1057,9 +1097,13 @@ static long seccomp_notify_recv(struct seccomp_filter *filter,
>  		goto out;
>  	}
>  
> +	memset(&unotif, 0, sizeof(unotif));
> +
>  	unotif.id = knotif->id;
>  	unotif.pid = task_pid_vnr(knotif->task);
>  	unotif.data = *(knotif->data);
> +	if (send_pidfd)
> +		group_leader = get_task_struct(knotif->task->group_leader);
>  
>  	knotif->state = SECCOMP_NOTIFY_SENT;
>  	wake_up_poll(&filter->notif->wqh, EPOLLOUT | EPOLLWRNORM);
> @@ -1067,9 +1111,21 @@ static long seccomp_notify_recv(struct seccomp_filter *filter,
>  out:
>  	mutex_unlock(&filter->notify_lock);
>  
> -	if (ret == 0 && copy_to_user(buf, &unotif, sizeof(unotif))) {
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * We've successfully received a notification, let's try to copy it to
> +	 * userspace.
> +	 */
> +	if (send_pidfd) {
> +		ret = __seccomp_notify_recv_pidfd(buf, &unotif, group_leader);
> +		put_task_struct(group_leader);
> +	} else if (copy_to_user(buf, &unotif, sizeof(unotif))) {
>  		ret = -EFAULT;
> +	}

To my eye, the way this helper is used is a bit ugly -- my first
question when reading this was "why aren't we doing a copy_to_user() for
pidfds?".

Something like the following might be a bit cleaner I think:

	struct file *pidfd_file = NULL;

	if (send_pidfd) {
		// helper allocates the pidfd_file and sets unotify->fd
		ret = __seccomp_notify_recv_pidfd(&unotify, &pidfd_file)
		if (ret)
			goto err; // or whatever
	}

	if (copy_to_user(buf, &unotif, sizeof(unotif))) {
		ret = -EFAULT;
		goto err; // or whatever
	}

	if (send_pidfd)
		fd_install(unotif.fd, pidfd_file)

But to be fair, this is also somewhat ugly too.

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 3/4] seccomp: Add SECCOMP_USER_NOTIF_FLAG_PIDFD to get pidfd on listener trap

[Aleksa Sarai]

[-- Attachment #1: Type: text/plain, Size: 1323 bytes --]

On 2020-01-26, Aleksa Sarai <cyphar@cyphar.com> wrote:
> On 2020-01-24, Sargun Dhillon <sargun@sargun.me> wrote:
> >  static long seccomp_notify_recv(struct seccomp_filter *filter,
> >  				void __user *buf)
> >  {
> >  	struct seccomp_knotif *knotif = NULL, *cur;
> >  	struct seccomp_notif unotif;
> > +	struct task_struct *group_leader;
> > +	bool send_pidfd;
> >  	ssize_t ret;
> >  
> > +	if (copy_from_user(&unotif, buf, sizeof(unotif)))
> > +		return -EFAULT;
> >  	/* Verify that we're not given garbage to keep struct extensible. */
> > -	ret = check_zeroed_user(buf, sizeof(unotif));
> > -	if (ret < 0)
> > -		return ret;
> > -	if (!ret)
> > +	if (unotif.id ||
> > +	    unotif.pid ||
> > +	    memchr_inv(&unotif.data, 0, sizeof(unotif.data)) ||
> > +	    unotif.pidfd)
> > +		return -EINVAL;
> 
> IMHO this check is more confusing than the original check_zeroed_user().
> Something like the following is simpler and less prone to forgetting to
> add a new field in the future:
> 
> 	if (memchr_inv(&unotif, 0, sizeof(unotif)))
> 		return -EINVAL;

Also the check in the patch doesn't ensure that any unnamed padding is
zeroed -- memchr_inv(&unotif, 0, sizeof(unotif)) does.

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/4] seccomp: Add SECCOMP_USER_NOTIF_FLAG_PIDFD to get pidfd on listener trap

[Sargun Dhillon]

On Sun, Jan 26, 2020 at 03:14:39PM +1100, Aleksa Sarai wrote:
> On 2020-01-26, Aleksa Sarai <cyphar@cyphar.com> wrote:
> > On 2020-01-24, Sargun Dhillon <sargun@sargun.me> wrote:
> > >  static long seccomp_notify_recv(struct seccomp_filter *filter,
> > >  				void __user *buf)
> > >  {
> > >  	struct seccomp_knotif *knotif = NULL, *cur;
> > >  	struct seccomp_notif unotif;
> > > +	struct task_struct *group_leader;
> > > +	bool send_pidfd;
> > >  	ssize_t ret;
> > >  
> > > +	if (copy_from_user(&unotif, buf, sizeof(unotif)))
> > > +		return -EFAULT;
> > >  	/* Verify that we're not given garbage to keep struct extensible. */
> > > -	ret = check_zeroed_user(buf, sizeof(unotif));
> > > -	if (ret < 0)
> > > -		return ret;
> > > -	if (!ret)
> > > +	if (unotif.id ||
> > > +	    unotif.pid ||
> > > +	    memchr_inv(&unotif.data, 0, sizeof(unotif.data)) ||
> > > +	    unotif.pidfd)
> > > +		return -EINVAL;
> > 
> > IMHO this check is more confusing than the original check_zeroed_user().
> > Something like the following is simpler and less prone to forgetting to
> > add a new field in the future:
> > 
I'm all for this, originally my patch read:

__u32 flags = 0;
swap(unotif.flags, flags);
if (memchr(&unotif, 0, sizeof(unotif))
	return -EINVAL;

--- And then check flags appropriately. I'm not sure if this is "better",
as I didn't see any other implementations that look like this in the
kernel. What do you think? It could even look "simpler", as in:

__u32 flags;

if (copy_from_user(....))
	return -EFAULT;
flags = unotif.flags;
unotif.flags = 0;
if (memchr_inv(&unotif, 0, sizeof(unotif)))
	return -EINVAL;


Are either of those preferential, reasonable, or at a minimum inoffensive?
> > 	if (memchr_inv(&unotif, 0, sizeof(unotif)))
> > 		return -EINVAL;
> 
Wouldn't this fail if flags was set to any value? We either need to zero
out flags prior to checking, or split it into range checks that exclude
flags.

> Also the check in the patch doesn't ensure that any unnamed padding is
> zeroed -- memchr_inv(&unotif, 0, sizeof(unotif)) does.
> 
> -- 
> Aleksa Sarai
> Senior Software Engineer (Containers)
> SUSE Linux GmbH
> <https://www.cyphar.com/>

[PATCH 4/4] selftests/seccomp: test SECCOMP_USER_NOTIF_FLAG_PIDFD

[Sargun Dhillon]

This adds a test which uses the SECCOMP_USER_NOTIF_FLAG_PIDFD flag. It
does this by using sys_pidfd_send_signal to signal the process, and
then relies on traditional waitpid to ensure that the specific
signal was delivered.

Additionally, it verifies the case where the copy of the notification
to userspace fails, and the pidfd file is required to be freed.

Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
 tools/testing/selftests/seccomp/seccomp_bpf.c | 110 ++++++++++++++++++
 1 file changed, 110 insertions(+)

diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index ee1b727ede04..ae9167ffbda9 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -187,6 +187,7 @@ struct seccomp_notif {
 	__u32 pid;
 	__u32 flags;
 	struct seccomp_data data;
+	__u32 pidfd;
 };
 
 struct seccomp_notif_resp {
@@ -212,6 +213,10 @@ struct seccomp_notif_sizes {
 #define SECCOMP_USER_NOTIF_FLAG_CONTINUE 0x00000001
 #endif
 
+#ifndef SECCOMP_USER_NOTIF_FLAG_PIDFD
+#define SECCOMP_USER_NOTIF_FLAG_PIDFD	(1UL << 0)
+#endif
+
 #ifndef seccomp
 int seccomp(unsigned int op, unsigned int flags, void *args)
 {
@@ -1871,6 +1876,7 @@ FIXTURE_TEARDOWN(TRACE_syscall)
 		free(self->prog.filter);
 }
 
+
 TEST_F(TRACE_syscall, ptrace_syscall_redirected)
 {
 	/* Swap SECCOMP_RET_TRACE tracer for PTRACE_SYSCALL tracer. */
@@ -3612,6 +3618,110 @@ TEST(user_notification_continue)
 	}
 }
 
+static int sys_pidfd_send_signal(int pidfd, int sig, siginfo_t *info,
+				 unsigned int flags)
+{
+#ifdef __NR_pidfd_send_signal
+	return syscall(__NR_pidfd_send_signal, pidfd, sig, info, flags);
+#else
+	errno = ENOSYS;
+	return -1;
+#endif
+}
+
+TEST(user_notification_pidfd)
+{
+	struct seccomp_notif req = {
+		.flags	= SECCOMP_USER_NOTIF_FLAG_PIDFD,
+	};
+	struct seccomp_notif_resp resp = {};
+	int ret, listener, status;
+	pid_t pid;
+
+
+	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!");
+	}
+
+	listener = user_trap_syscall(__NR_getppid,
+				     SECCOMP_FILTER_FLAG_NEW_LISTENER);
+	ASSERT_GE(listener, 0);
+
+	pid = fork();
+	ASSERT_GE(pid, 0);
+
+	if (pid == 0) {
+		/* the process should be killed during this syscall */
+		syscall(__NR_getppid);
+		exit(0);
+	}
+
+	EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
+	ASSERT_GE(req.pidfd, 0);
+
+	ASSERT_EQ(sys_pidfd_send_signal(req.pidfd, SIGKILL, NULL, 0), 0) {
+		XFAIL(goto out,
+		      "Kernel does not support pidfd_send_signal() syscall");
+		goto out;
+	}
+	EXPECT_EQ(req.pid, pid);
+
+	EXPECT_EQ(waitpid(pid, &status, 0), pid);
+	EXPECT_EQ(true, WIFSIGNALED(status));
+	EXPECT_EQ(SIGKILL, WTERMSIG(status));
+
+out:
+	close(req.pidfd);
+	close(listener);
+}
+
+TEST(user_notification_pidfd_fault)
+{
+	struct seccomp_notif req = {
+		.flags	= SECCOMP_USER_NOTIF_FLAG_PIDFD,
+	};
+	struct seccomp_notif_resp resp = {};
+	int ret, listener, status;
+	pid_t pid;
+
+	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!");
+	}
+
+	listener = user_trap_syscall(__NR_getppid,
+				     SECCOMP_FILTER_FLAG_NEW_LISTENER);
+	ASSERT_GE(listener, 0);
+
+	pid = fork();
+	ASSERT_GE(pid, 0);
+
+	if (pid == 0)
+		exit(syscall(__NR_getppid) != USER_NOTIF_MAGIC);
+
+	/* trigger an EFAULT */
+	EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, NULL), -1);
+	EXPECT_EQ(errno, EFAULT);
+
+	/* Check that we can still fetch it. */
+	EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
+	EXPECT_EQ(req.pid, pid);
+
+	resp.id = req.id;
+	resp.error = 0;
+	resp.val = USER_NOTIF_MAGIC;
+
+	EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), 0);
+
+	EXPECT_EQ(waitpid(pid, &status, 0), pid);
+	EXPECT_EQ(true, WIFEXITED(status));
+	EXPECT_EQ(0, WEXITSTATUS(status));
+
+	close(req.pidfd);
+	close(listener);
+}
+
 /*
  * TODO:
  * - add microbenchmarks
-- 
2.20.1