Add SCM_PIDFD and SO_PEERPIDFD

It seems that we already can use pidfd_open() (since linux-5.3), and pass the resulting fd in af_unix SCM_RIGHTS message ?

Let me try and provide some of the missing background.

There are a range of use-cases where we would like to authenticate a client through sockets without being susceptible to PID recycling attacks. Currently, we can't do this as the race isn't fully fixable. We can only apply mitigations.

What this patchset will allows us to do is to get a pidfd without the client having to send us an fd explicitly via SCM_RIGHTS. As that's already possibly as you correctly point out.

But for protocols like polkit this is quite important. Every message is standalone and we would need to force a complete protocol change where we would need to require that every client allocate and send a pidfd via SCM_RIGHTS. That would also mean patching through all polkit users.

For something like systemd-journald where we provide logging facilities and want to add metadata to the log we would also immensely benefit from being able to get a receiver-side controlled pidfd.

With the message type we envisioned we don't need to change the sender at all and can be safe against pid recycling.

Link https://gitlab.freedesktop.org/polkit/polkit/-/merge_requests/154

Link https://uapi-group.org/kernel-features

So yes, this is of course possible, but it would mean the pidfd would have to be transported as part of the user protocol, explicitly sent by the sender. (Moreover, the receiver after receiving the pidfd would then still have to somehow be able to prove that the pidfd it just received actually refers to the peer's process and not some random process. – this part is actually solvable in userspace, but ugly)

The big thing is simply that we want that the pidfd is associated *implicity* with each AF_UNIX connection, not explicitly. A lot of userspace already relies on this, both in the authentication area (polkit) as well as in the logging area (systemd-journald). Right now using the PID field from SO_PEERCREDS/SCM_CREDENTIALS is racy though and very hard to get right. Making this available as pidfd too, would solve this raciness, without otherwise changing semantics of it all:
receivers can still enable the creds stuff as they wish, and the data is then implicitly appended to the connections/datagrams the sender initiates.

Or to turn this around: things like polkit are typically used to authenticate arbitrary dbus methods calls: some service implements a dbus method call, and when an unprivileged client then issues that call, it will take the client's info, go to polkit and ask it if this is ok. If we wanted to send the pidfd as part of the protocol we basically would have to extend every single method call to contain the client's pidfd along with it as an additional argument, which would be a massive undertaking: it would change the prototypes of basically *all* methods a service defines… And that's just ugly.

Note that Alex' patch set doesn't expose anything that wasn't exposed before, or attach, propagate what wasn't before. All it does, is make the field already available anyway (the struct ucred .pid field) available also in a better way (as a pidfd), to solve a variety of races, with no effect on the protocol actually spoken within the AF_UNIX transport. It's a seamless improvement of the status quo.

Implement SCM_PIDFD, a new type of CMSG type analogical to SCM_CREDENTIALS, but it contains pidfd instead of plain pid, which allows programmers not to care about PID reuse problem.

We mask SO_PASSPIDFD feature if CONFIG_UNIX is not builtin because it depends on a pidfd_prepare() API which is not exported to the kernel modules.

Idea comes from UAPI kernel group:
https://uapi-group.org/kernel-features/

Big thanks to Christian Brauner and Lennart Poettering for productive discussions about this.

Cc "David S. Miller" <[email protected]>

Cc Eric Dumazet <[email protected]>

Cc Jakub Kicinski <[email protected]>

Cc Paolo Abeni <[email protected]>

Cc Leon Romanovsky <[email protected]>

Cc David Ahern <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Kees Cook <[email protected]>

Cc Christian Brauner <[email protected]>

Cc Kuniyuki Iwashima <[email protected]>

Cc Lennart Poettering <[email protected]>

Cc Luca Boccassi <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Tested-by Luca Boccassi <[email protected]>

Reviewed-by Kuniyuki Iwashima <[email protected]>

Reviewed-by Christian Brauner <[email protected]>

Signed-off-by Alexander Mikhalitsyn <[email protected]>

---

Quoted 34 lines · Alexander Mikhalitsyn

Implement SCM_PIDFD, a new type of CMSG type analogical to SCM_CREDENTIALS, but it contains pidfd instead of plain pid, which allows programmers not to care about PID reuse problem.

We mask SO_PASSPIDFD feature if CONFIG_UNIX is not builtin because it depends on a pidfd_prepare() API which is not exported to the kernel modules.

Idea comes from UAPI kernel group:
https://uapi-group.org/kernel-features/

Big thanks to Christian Brauner and Lennart Poettering for productive discussions about this.

Cc "David S. Miller" <[email protected]>

Cc Eric Dumazet <[email protected]>

Cc Jakub Kicinski <[email protected]>

Cc Paolo Abeni <[email protected]>

Cc Leon Romanovsky <[email protected]>

Cc David Ahern <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Kees Cook <[email protected]>

Cc Christian Brauner <[email protected]>

Cc Kuniyuki Iwashima <[email protected]>

Cc Lennart Poettering <[email protected]>

Cc Luca Boccassi <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Tested-by Luca Boccassi <[email protected]>

Reviewed-by Kuniyuki Iwashima <[email protected]>

Reviewed-by Christian Brauner <[email protected]>

Signed-off-by Alexander Mikhalitsyn <[email protected]>

Quoted 1 line

---

Reviewed-by Eric Dumazet <[email protected]>

Implement SCM_PIDFD, a new type of CMSG type analogical to SCM_CREDENTIALS, but it contains pidfd instead of plain pid, which allows programmers not to care about PID reuse problem.

We mask SO_PASSPIDFD feature if CONFIG_UNIX is not builtin because it depends on a pidfd_prepare() API which is not exported to the kernel modules.

Idea comes from UAPI kernel group:
https://uapi-group.org/kernel-features/

Big thanks to Christian Brauner and Lennart Poettering for productive discussions about this.

Cc "David S. Miller" <[email protected]>

Cc Eric Dumazet <[email protected]>

Cc Jakub Kicinski <[email protected]>

Cc Paolo Abeni <[email protected]>

Cc Leon Romanovsky <[email protected]>

Cc David Ahern <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Kees Cook <[email protected]>

Cc Christian Brauner <[email protected]>

Cc Kuniyuki Iwashima <[email protected]>

Cc Lennart Poettering <[email protected]>

Cc Luca Boccassi <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Tested-by Luca Boccassi <[email protected]>

Reviewed-by Kuniyuki Iwashima <[email protected]>

Reviewed-by Christian Brauner <[email protected]>

Signed-off-by Alexander Mikhalitsyn <[email protected]>

---
 arch/alpha/include/uapi/asm/socket.h    |  2 ++
 arch/mips/include/uapi/asm/socket.h     |  2 ++
 arch/parisc/include/uapi/asm/socket.h   |  2 ++
 arch/sparc/include/uapi/asm/socket.h    |  2 ++
 include/linux/net.h                     |  1 +
 include/linux/socket.h                  |  1 +
 include/net/scm.h                       | 39 +++++++++++++++++++++++--
 include/uapi/asm-generic/socket.h       |  2 ++
 net/core/sock.c                         | 11 +++++++
 net/mptcp/sockopt.c                     |  1 +
 net/unix/af_unix.c                      | 18 ++++++++----
 tools/include/uapi/asm-generic/socket.h |  2 ++
 12 files changed, 76 insertions(+), 7 deletions(-)

include/net/scm.h +4 -0

+static __inline__ void scm_pidfd_recv(struct msghdr *msg, struct scm_cookie *scm)
+{
+	struct file *pidfd_file = NULL;
+	int pidfd;

Quoted 47 lines

Implement SCM_PIDFD, a new type of CMSG type analogical to SCM_CREDENTIALS, but it contains pidfd instead of plain pid, which allows programmers not to care about PID reuse problem.

We mask SO_PASSPIDFD feature if CONFIG_UNIX is not builtin because it depends on a pidfd_prepare() API which is not exported to the kernel modules.

Idea comes from UAPI kernel group:
https://uapi-group.org/kernel-features/

Big thanks to Christian Brauner and Lennart Poettering for productive discussions about this.

Cc "David S. Miller" <[email protected]>

Cc Eric Dumazet <[email protected]>

Cc Jakub Kicinski <[email protected]>

Cc Paolo Abeni <[email protected]>

Cc Leon Romanovsky <[email protected]>

Cc David Ahern <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Kees Cook <[email protected]>

Cc Christian Brauner <[email protected]>

Cc Kuniyuki Iwashima <[email protected]>

Cc Lennart Poettering <[email protected]>

Cc Luca Boccassi <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Tested-by Luca Boccassi <[email protected]>

Reviewed-by Kuniyuki Iwashima <[email protected]>

Reviewed-by Christian Brauner <[email protected]>

Signed-off-by Alexander Mikhalitsyn <[email protected]>

---
 arch/alpha/include/uapi/asm/socket.h    |  2 ++
 arch/mips/include/uapi/asm/socket.h     |  2 ++
 arch/parisc/include/uapi/asm/socket.h   |  2 ++
 arch/sparc/include/uapi/asm/socket.h    |  2 ++
 include/linux/net.h                     |  1 +
 include/linux/socket.h                  |  1 +
 include/net/scm.h                       | 39 +++++++++++++++++++++++--
 include/uapi/asm-generic/socket.h       |  2 ++
 net/core/sock.c                         | 11 +++++++
 net/mptcp/sockopt.c                     |  1 +
 net/unix/af_unix.c                      | 18 ++++++++----
 tools/include/uapi/asm-generic/socket.h |  2 ++
 12 files changed, 76 insertions(+), 7 deletions(-)

...

Quoted 4 lines

(quoted diff) +4 -0

+static __inline__ void scm_pidfd_recv(struct msghdr *msg, struct scm_cookie *scm)
+{
+	struct file *pidfd_file = NULL;
+	int pidfd;

Review

Quoted 10 lines

(quoted diff) +10 -0

+
+	/*
+	 * put_cmsg() doesn't return an error if CMSG is truncated,
+	 * that's why we need to opencode these checks here.
+	 */
+	if ((msg->msg_controllen <= sizeof(struct cmsghdr)) ||
+	    (msg->msg_controllen - sizeof(struct cmsghdr)) < sizeof(int)) {
+		msg->msg_flags |= MSG_CTRUNC;
+		return;
+	}

This does not work for compat tasks since the size of struct cmsghdr (aka struct compat_cmsghdr) is differently. If the check from put_cmsg() is open-coded here, then also a different check for compat tasks needs to be added.

Discovered this because I was wondering why strace compat tests fail; it seems because of this.

See https://github.com/strace/strace/blob/master/tests/scm_pidfd.c

For compat tasks recvmsg() returns with msg_flags=MSG_CTRUNC since the above code expects a larger buffer than is necessary.

From: Heiko Carstens <[email protected]>
Date: Fri, 1 Sep 2023 22:05:17 +0200

Quoted 15 lines · Alexander Mikhalitsyn

include/net/scm.h static __inline__ void scm_pidfd_recv(struct msghdr *msg, struct scm_cookie *scm · line 136

Quoted 5 lines

include/net/scm.h +5 -0

+	if ((msg->msg_controllen <= sizeof(struct cmsghdr)) ||
+	    (msg->msg_controllen - sizeof(struct cmsghdr)) < sizeof(int)) {
+		msg->msg_flags |= MSG_CTRUNC;
+		return;
+	}

This does not work for compat tasks since the size of struct cmsghdr (aka struct compat_cmsghdr) is differently. If the check from put_cmsg() is open-coded here, then also a different check for compat tasks needs to be added.

Discovered this because I was wondering why strace compat tests fail; it seems because of this.

Quoted 4 lines

See https://github.com/strace/strace/blob/master/tests/scm_pidfd.c

For compat tasks recvmsg() returns with msg_flags=MSG_CTRUNC since the above code expects a larger buffer than is necessary.

Can you test this ?

Quoted 19 lines

From: Heiko Carstens <[email protected]>
Date: Fri, 1 Sep 2023 22:05:17 +0200

Quoted 17 lines · Alexander Mikhalitsyn

include/net/scm.h static __inline__ void scm_pidfd_recv(struct msghdr *msg, struct scm_cookie *scm · line 136

Quoted 5 lines

include/net/scm.h +5 -0

+	if ((msg->msg_controllen <= sizeof(struct cmsghdr)) ||
+	    (msg->msg_controllen - sizeof(struct cmsghdr)) < sizeof(int)) {
+		msg->msg_flags |= MSG_CTRUNC;
+		return;
+	}

This does not work for compat tasks since the size of struct cmsghdr (aka struct compat_cmsghdr) is differently. If the check from put_cmsg() is open-coded here, then also a different check for compat tasks needs to be added.

Discovered this because I was wondering why strace compat tests fail; it seems because of this.

See https://github.com/strace/strace/blob/master/tests/scm_pidfd.c

Quoted 4 lines

Quoted 2 lines

For compat tasks recvmsg() returns with msg_flags=MSG_CTRUNC since the above code expects a larger buffer than is necessary.

Can you test this ?

Works for me.

Tested-by Heiko Carstens <[email protected]>

From: Heiko Carstens <[email protected]>
Date: Fri, 1 Sep 2023 22:51:45 +0200

Quoted 28 lines · Kuniyuki Iwashima

Quoted 17 lines

From: Heiko Carstens <[email protected]>
Date: Fri, 1 Sep 2023 22:05:17 +0200

Quoted 15 lines · Alexander Mikhalitsyn

include/net/scm.h static __inline__ void scm_pidfd_recv(struct msghdr *msg, struct scm_cookie *scm · line 136

Quoted 5 lines

include/net/scm.h +5 -0

+       if ((msg->msg_controllen <= sizeof(struct cmsghdr)) ||
+           (msg->msg_controllen - sizeof(struct cmsghdr)) < sizeof(int)) {
+               msg->msg_flags |= MSG_CTRUNC;
+               return;
+       }

This does not work for compat tasks since the size of struct cmsghdr (aka struct compat_cmsghdr) is differently. If the check from put_cmsg() is open-coded here, then also a different check for compat tasks needs to be added.

Discovered this because I was wondering why strace compat tests fail; it seems because of this.

Quoted 6 lines

Quoted 4 lines

See https://github.com/strace/strace/blob/master/tests/scm_pidfd.c

For compat tasks recvmsg() returns with msg_flags=MSG_CTRUNC since the above code expects a larger buffer than is necessary.

Can you test this ?

Works for me.

Tested-by Heiko Carstens <[email protected]>

Thanks!
I'll post a formal patch.

Add SO_PEERPIDFD which allows to get pidfd of peer socket holder pidfd. This thing is direct analog of SO_PEERCRED which allows to get plain PID.

Cc "David S. Miller" <[email protected]>

Cc Eric Dumazet <[email protected]>

Cc Jakub Kicinski <[email protected]>

Cc Paolo Abeni <[email protected]>

Cc Leon Romanovsky <[email protected]>

Cc David Ahern <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Kees Cook <[email protected]>

Cc Christian Brauner <[email protected]>

Cc Kuniyuki Iwashima <[email protected]>

Cc Lennart Poettering <[email protected]>

Cc Luca Boccassi <[email protected]>

Cc Daniel Borkmann <[email protected]>

Cc Stanislav Fomichev <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Reviewed-by Christian Brauner <[email protected]>

Acked-by Stanislav Fomichev <[email protected]>

Tested-by Luca Boccassi <[email protected]>

Signed-off-by Alexander Mikhalitsyn <[email protected]>

Let's make CONFIG_UNIX a bool instead of a tristate.
We've decided to do that during discussion about SCM_PIDFD patchset [1].

[1] https://lore.kernel.org/lkml/[email protected]/

Cc "David S. Miller" <[email protected]>

Cc Eric Dumazet <[email protected]>

Cc Jakub Kicinski <[email protected]>

Cc Paolo Abeni <[email protected]>

Cc Leon Romanovsky <[email protected]>

Cc David Ahern <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Kees Cook <[email protected]>

Cc Christian Brauner <[email protected]>

Cc Kuniyuki Iwashima <[email protected]>

Cc Lennart Poettering <[email protected]>

Cc Luca Boccassi <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Suggested-by Jakub Kicinski <[email protected]>

Signed-off-by Alexander Mikhalitsyn <[email protected]>

---

Let's make CONFIG_UNIX a bool instead of a tristate.
We've decided to do that during discussion about SCM_PIDFD patchset [1].

[1] https://lore.kernel.org/lkml/[email protected]/

Cc "David S. Miller" <[email protected]>

Cc Eric Dumazet <[email protected]>

Cc Jakub Kicinski <[email protected]>

Cc Paolo Abeni <[email protected]>

Cc Leon Romanovsky <[email protected]>

Cc David Ahern <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Kees Cook <[email protected]>

Cc Christian Brauner <[email protected]>

Cc Kuniyuki Iwashima <[email protected]>

Cc Lennart Poettering <[email protected]>

Cc Luca Boccassi <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Suggested-by Jakub Kicinski <[email protected]>

Signed-off-by Alexander Mikhalitsyn <[email protected]>