| Message ID | 20230316131526.283569-2-aleksandr.mikhalitsyn@canonical.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add SCM_PIDFD and SO_PEERPIDFD | expand |
On Thu, Mar 16, 2023 at 6:16 AM Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> wrote: > > 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. Hi Alexander This would add yet another conditional in af_unix fast path. It seems that we already can use pidfd_open() (since linux-5.3), and pass the resulting fd in af_unix SCM_RIGHTS message ? If you think this is not suitable, it should at least be mentioned in the changelog. Thanks.
On Thu, Mar 16, 2023 at 3:34 PM Eric Dumazet <edumazet@google.com> wrote: > > On Thu, Mar 16, 2023 at 6:16 AM Alexander Mikhalitsyn > <aleksandr.mikhalitsyn@canonical.com> wrote: > > > > 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. > > Hi Alexander Hi Eric Thanks for the fast reply! ;-) > > This would add yet another conditional in af_unix fast path. > > It seems that we already can use pidfd_open() (since linux-5.3), and > pass the resulting fd in af_unix SCM_RIGHTS message ? Yes, it's possible, but it means that from the receiver side we need to trust the sent pidfd (in SCM_RIGHTS), or always use combination of SCM_RIGHTS+SCM_CREDENTIALS, then we can extract pidfd from SCM_RIGHTS, then acquire plain pid from pidfd and after compare it with the pid from SCM_CREDENTIALS. > > If you think this is not suitable, it should at least be mentioned in > the changelog. Kind regards, Alex > > Thanks.
Hi Alexander,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net-next/main]
url: https://github.com/intel-lab-lkp/linux/commits/Alexander-Mikhalitsyn/scm-add-SO_PASSPIDFD-and-SCM_PIDFD/20230316-214315
patch link: https://lore.kernel.org/r/20230316131526.283569-2-aleksandr.mikhalitsyn%40canonical.com
patch subject: [PATCH net-next 1/3] scm: add SO_PASSPIDFD and SCM_PIDFD
config: s390-buildonly-randconfig-r002-20230312 (https://download.01.org/0day-ci/archive/20230317/202303170733.ZQbJFE5x-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/41687b4ae0dcef1fdffd656e533f9f35214043d0
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Alexander-Mikhalitsyn/scm-add-SO_PASSPIDFD-and-SCM_PIDFD/20230316-214315
git checkout 41687b4ae0dcef1fdffd656e533f9f35214043d0
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=s390 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303170733.ZQbJFE5x-lkp@intel.com/
All errors (new ones prefixed by >>, old ones prefixed by <<):
ERROR: modpost: "devm_platform_ioremap_resource" [drivers/dma/idma64.ko] undefined!
>> ERROR: modpost: "pidfd_create" [net/unix/unix.ko] undefined!
From: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> Date: Thu, 16 Mar 2023 14:15:24 +0100 > 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. > > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Eric Dumazet <edumazet@google.com> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: Paolo Abeni <pabeni@redhat.com> > Cc: Leon Romanovsky <leon@kernel.org> > Cc: David Ahern <dsahern@kernel.org> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Kees Cook <keescook@chromium.org> > Cc: Christian Brauner <brauner@kernel.org> > Cc: linux-kernel@vger.kernel.org > Cc: netdev@vger.kernel.org > Cc: linux-arch@vger.kernel.org > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> > --- > 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 | 16 +++++++++++++++- > 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, 54 insertions(+), 6 deletions(-) > > diff --git a/arch/alpha/include/uapi/asm/socket.h b/arch/alpha/include/uapi/asm/socket.h > index 739891b94136..ff310613ae64 100644 > --- a/arch/alpha/include/uapi/asm/socket.h > +++ b/arch/alpha/include/uapi/asm/socket.h > @@ -137,6 +137,8 @@ > > #define SO_RCVMARK 75 > > +#define SO_PASSPIDFD 76 > + > #if !defined(__KERNEL__) > > #if __BITS_PER_LONG == 64 > diff --git a/arch/mips/include/uapi/asm/socket.h b/arch/mips/include/uapi/asm/socket.h > index 18f3d95ecfec..762dcb80e4ec 100644 > --- a/arch/mips/include/uapi/asm/socket.h > +++ b/arch/mips/include/uapi/asm/socket.h > @@ -148,6 +148,8 @@ > > #define SO_RCVMARK 75 > > +#define SO_PASSPIDFD 76 > + > #if !defined(__KERNEL__) > > #if __BITS_PER_LONG == 64 > diff --git a/arch/parisc/include/uapi/asm/socket.h b/arch/parisc/include/uapi/asm/socket.h > index f486d3dfb6bb..df16a3e16d64 100644 > --- a/arch/parisc/include/uapi/asm/socket.h > +++ b/arch/parisc/include/uapi/asm/socket.h > @@ -129,6 +129,8 @@ > > #define SO_RCVMARK 0x4049 > > +#define SO_PASSPIDFD 0x404A > + > #if !defined(__KERNEL__) > > #if __BITS_PER_LONG == 64 > diff --git a/arch/sparc/include/uapi/asm/socket.h b/arch/sparc/include/uapi/asm/socket.h > index 2fda57a3ea86..6e2847804fea 100644 > --- a/arch/sparc/include/uapi/asm/socket.h > +++ b/arch/sparc/include/uapi/asm/socket.h > @@ -130,6 +130,8 @@ > > #define SO_RCVMARK 0x0054 > > +#define SO_PASSPIDFD 0x0055 > + > #if !defined(__KERNEL__) > > > diff --git a/include/linux/net.h b/include/linux/net.h > index b73ad8e3c212..c234dfbe7a30 100644 > --- a/include/linux/net.h > +++ b/include/linux/net.h > @@ -43,6 +43,7 @@ struct net; > #define SOCK_PASSSEC 4 > #define SOCK_SUPPORT_ZC 5 > #define SOCK_CUSTOM_SOCKOPT 6 > +#define SOCK_PASSPIDFD 7 > > #ifndef ARCH_HAS_SOCKET_TYPES > /** > diff --git a/include/linux/socket.h b/include/linux/socket.h > index 13c3a237b9c9..6bf90f251910 100644 > --- a/include/linux/socket.h > +++ b/include/linux/socket.h > @@ -177,6 +177,7 @@ static inline size_t msg_data_left(struct msghdr *msg) > #define SCM_RIGHTS 0x01 /* rw: access rights (array of int) */ > #define SCM_CREDENTIALS 0x02 /* rw: struct ucred */ > #define SCM_SECURITY 0x03 /* rw: security label */ > +#define SCM_PIDFD 0x04 /* ro: pidfd (int) */ > > struct ucred { > __u32 pid; > diff --git a/include/net/scm.h b/include/net/scm.h > index 585adc1346bd..4617fbc65294 100644 > --- a/include/net/scm.h > +++ b/include/net/scm.h > @@ -124,7 +124,9 @@ static __inline__ void scm_recv(struct socket *sock, struct msghdr *msg, > struct scm_cookie *scm, int flags) > { > if (!msg->msg_control) { > - if (test_bit(SOCK_PASSCRED, &sock->flags) || scm->fp || > + if (test_bit(SOCK_PASSCRED, &sock->flags) || > + test_bit(SOCK_PASSPIDFD, &sock->flags) || > + scm->fp || nit: I'd remove newline here. > scm_has_secdata(sock)) > msg->msg_flags |= MSG_CTRUNC; > scm_destroy(scm); > @@ -141,6 +143,18 @@ static __inline__ void scm_recv(struct socket *sock, struct msghdr *msg, > put_cmsg(msg, SOL_SOCKET, SCM_CREDENTIALS, sizeof(ucreds), &ucreds); > } > > + if (test_bit(SOCK_PASSPIDFD, &sock->flags)) { > + int pidfd; > + > + if (WARN_ON_ONCE(!scm->pid) || > + !pid_has_task(scm->pid, PIDTYPE_TGID)) Can we change pidfd_create() to return -ESRCH as it has the same test ? > + pidfd = -ESRCH; > + else > + pidfd = pidfd_create(scm->pid, 0); Then we can change this part like: int pidfd = pidfd_create(scm->pid, 0); WARN_ON_ONCE(!scm->pid); Thanks, Kuniyuki > + > + put_cmsg(msg, SOL_SOCKET, SCM_PIDFD, sizeof(int), &pidfd); > + } > + > scm_destroy_cred(scm); > > scm_passec(sock, msg, scm); > diff --git a/include/uapi/asm-generic/socket.h b/include/uapi/asm-generic/socket.h > index 638230899e98..b76169fdb80b 100644 > --- a/include/uapi/asm-generic/socket.h > +++ b/include/uapi/asm-generic/socket.h > @@ -132,6 +132,8 @@ > > #define SO_RCVMARK 75 > > +#define SO_PASSPIDFD 76 > + > #if !defined(__KERNEL__) > > #if __BITS_PER_LONG == 64 || (defined(__x86_64__) && defined(__ILP32__)) > diff --git a/net/core/sock.c b/net/core/sock.c > index c25888795390..3f974246ba3e 100644 > --- a/net/core/sock.c > +++ b/net/core/sock.c > @@ -1246,6 +1246,13 @@ int sk_setsockopt(struct sock *sk, int level, int optname, > clear_bit(SOCK_PASSCRED, &sock->flags); > break; > > + case SO_PASSPIDFD: > + if (valbool) > + set_bit(SOCK_PASSPIDFD, &sock->flags); > + else > + clear_bit(SOCK_PASSPIDFD, &sock->flags); > + break; > + > case SO_TIMESTAMP_OLD: > case SO_TIMESTAMP_NEW: > case SO_TIMESTAMPNS_OLD: > @@ -1737,6 +1744,10 @@ int sk_getsockopt(struct sock *sk, int level, int optname, > v.val = !!test_bit(SOCK_PASSCRED, &sock->flags); > break; > > + case SO_PASSPIDFD: > + v.val = !!test_bit(SOCK_PASSPIDFD, &sock->flags); > + break; > + > case SO_PEERCRED: > { > struct ucred peercred; > diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c > index 8a9656248b0f..bd80e707d0b3 100644 > --- a/net/mptcp/sockopt.c > +++ b/net/mptcp/sockopt.c > @@ -355,6 +355,7 @@ static int mptcp_setsockopt_sol_socket(struct mptcp_sock *msk, int optname, > case SO_BROADCAST: > case SO_BSDCOMPAT: > case SO_PASSCRED: > + case SO_PASSPIDFD: > case SO_PASSSEC: > case SO_RXQ_OVFL: > case SO_WIFI_STATUS: > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c > index 0b0f18ecce44..b0ac768752fa 100644 > --- a/net/unix/af_unix.c > +++ b/net/unix/af_unix.c > @@ -1361,7 +1361,8 @@ static int unix_dgram_connect(struct socket *sock, struct sockaddr *addr, > if (err) > goto out; > > - if (test_bit(SOCK_PASSCRED, &sock->flags) && > + if ((test_bit(SOCK_PASSCRED, &sock->flags) || > + test_bit(SOCK_PASSPIDFD, &sock->flags)) && > !unix_sk(sk)->addr) { > err = unix_autobind(sk); > if (err) > @@ -1469,7 +1470,8 @@ static int unix_stream_connect(struct socket *sock, struct sockaddr *uaddr, > if (err) > goto out; > > - if (test_bit(SOCK_PASSCRED, &sock->flags) && !u->addr) { > + if ((test_bit(SOCK_PASSCRED, &sock->flags) || > + test_bit(SOCK_PASSPIDFD, &sock->flags)) && !u->addr) { > err = unix_autobind(sk); > if (err) > goto out; > @@ -1670,6 +1672,8 @@ static void unix_sock_inherit_flags(const struct socket *old, > { > if (test_bit(SOCK_PASSCRED, &old->flags)) > set_bit(SOCK_PASSCRED, &new->flags); > + if (test_bit(SOCK_PASSPIDFD, &old->flags)) > + set_bit(SOCK_PASSPIDFD, &new->flags); > if (test_bit(SOCK_PASSSEC, &old->flags)) > set_bit(SOCK_PASSSEC, &new->flags); > } > @@ -1819,8 +1823,10 @@ static bool unix_passcred_enabled(const struct socket *sock, > const struct sock *other) > { > return test_bit(SOCK_PASSCRED, &sock->flags) || > + test_bit(SOCK_PASSPIDFD, &sock->flags) || > !other->sk_socket || > - test_bit(SOCK_PASSCRED, &other->sk_socket->flags); > + test_bit(SOCK_PASSCRED, &other->sk_socket->flags) || > + test_bit(SOCK_PASSPIDFD, &other->sk_socket->flags); > } > > /* > @@ -1922,7 +1928,8 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg, > goto out; > } > > - if (test_bit(SOCK_PASSCRED, &sock->flags) && !u->addr) { > + if ((test_bit(SOCK_PASSCRED, &sock->flags) || > + test_bit(SOCK_PASSPIDFD, &sock->flags)) && !u->addr) { > err = unix_autobind(sk); > if (err) > goto out; > @@ -2824,7 +2831,8 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state, > /* Never glue messages from different writers */ > if (!unix_skb_scm_eq(skb, &scm)) > break; > - } else if (test_bit(SOCK_PASSCRED, &sock->flags)) { > + } else if (test_bit(SOCK_PASSCRED, &sock->flags) || > + test_bit(SOCK_PASSPIDFD, &sock->flags)) { > /* Copy credentials */ > scm_set_cred(&scm, UNIXCB(skb).pid, UNIXCB(skb).uid, UNIXCB(skb).gid); > unix_set_secdata(&scm, skb); > diff --git a/tools/include/uapi/asm-generic/socket.h b/tools/include/uapi/asm-generic/socket.h > index 8756df13be50..fbbc4bf53ee3 100644 > --- a/tools/include/uapi/asm-generic/socket.h > +++ b/tools/include/uapi/asm-generic/socket.h > @@ -121,6 +121,8 @@ > > #define SO_RCVMARK 75 > > +#define SO_PASSPIDFD 76 > + > #if !defined(__KERNEL__) > > #if __BITS_PER_LONG == 64 || (defined(__x86_64__) && defined(__ILP32__)) > -- > 2.34.1
On Thu, Mar 16, 2023 at 10:53:20PM -0700, Kuniyuki Iwashima wrote: > From: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> > Date: Thu, 16 Mar 2023 14:15:24 +0100 > > 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. > > > > Cc: "David S. Miller" <davem@davemloft.net> > > Cc: Eric Dumazet <edumazet@google.com> > > Cc: Jakub Kicinski <kuba@kernel.org> > > Cc: Paolo Abeni <pabeni@redhat.com> > > Cc: Leon Romanovsky <leon@kernel.org> > > Cc: David Ahern <dsahern@kernel.org> > > Cc: Arnd Bergmann <arnd@arndb.de> > > Cc: Kees Cook <keescook@chromium.org> > > Cc: Christian Brauner <brauner@kernel.org> > > Cc: linux-kernel@vger.kernel.org > > Cc: netdev@vger.kernel.org > > Cc: linux-arch@vger.kernel.org > > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> > > --- > > 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 | 16 +++++++++++++++- > > 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, 54 insertions(+), 6 deletions(-) > > > > diff --git a/arch/alpha/include/uapi/asm/socket.h b/arch/alpha/include/uapi/asm/socket.h > > index 739891b94136..ff310613ae64 100644 > > --- a/arch/alpha/include/uapi/asm/socket.h > > +++ b/arch/alpha/include/uapi/asm/socket.h > > @@ -137,6 +137,8 @@ > > > > #define SO_RCVMARK 75 > > > > +#define SO_PASSPIDFD 76 > > + > > #if !defined(__KERNEL__) > > > > #if __BITS_PER_LONG == 64 > > diff --git a/arch/mips/include/uapi/asm/socket.h b/arch/mips/include/uapi/asm/socket.h > > index 18f3d95ecfec..762dcb80e4ec 100644 > > --- a/arch/mips/include/uapi/asm/socket.h > > +++ b/arch/mips/include/uapi/asm/socket.h > > @@ -148,6 +148,8 @@ > > > > #define SO_RCVMARK 75 > > > > +#define SO_PASSPIDFD 76 > > + > > #if !defined(__KERNEL__) > > > > #if __BITS_PER_LONG == 64 > > diff --git a/arch/parisc/include/uapi/asm/socket.h b/arch/parisc/include/uapi/asm/socket.h > > index f486d3dfb6bb..df16a3e16d64 100644 > > --- a/arch/parisc/include/uapi/asm/socket.h > > +++ b/arch/parisc/include/uapi/asm/socket.h > > @@ -129,6 +129,8 @@ > > > > #define SO_RCVMARK 0x4049 > > > > +#define SO_PASSPIDFD 0x404A > > + > > #if !defined(__KERNEL__) > > > > #if __BITS_PER_LONG == 64 > > diff --git a/arch/sparc/include/uapi/asm/socket.h b/arch/sparc/include/uapi/asm/socket.h > > index 2fda57a3ea86..6e2847804fea 100644 > > --- a/arch/sparc/include/uapi/asm/socket.h > > +++ b/arch/sparc/include/uapi/asm/socket.h > > @@ -130,6 +130,8 @@ > > > > #define SO_RCVMARK 0x0054 > > > > +#define SO_PASSPIDFD 0x0055 > > + > > #if !defined(__KERNEL__) > > > > > > diff --git a/include/linux/net.h b/include/linux/net.h > > index b73ad8e3c212..c234dfbe7a30 100644 > > --- a/include/linux/net.h > > +++ b/include/linux/net.h > > @@ -43,6 +43,7 @@ struct net; > > #define SOCK_PASSSEC 4 > > #define SOCK_SUPPORT_ZC 5 > > #define SOCK_CUSTOM_SOCKOPT 6 > > +#define SOCK_PASSPIDFD 7 > > > > #ifndef ARCH_HAS_SOCKET_TYPES > > /** > > diff --git a/include/linux/socket.h b/include/linux/socket.h > > index 13c3a237b9c9..6bf90f251910 100644 > > --- a/include/linux/socket.h > > +++ b/include/linux/socket.h > > @@ -177,6 +177,7 @@ static inline size_t msg_data_left(struct msghdr *msg) > > #define SCM_RIGHTS 0x01 /* rw: access rights (array of int) */ > > #define SCM_CREDENTIALS 0x02 /* rw: struct ucred */ > > #define SCM_SECURITY 0x03 /* rw: security label */ > > +#define SCM_PIDFD 0x04 /* ro: pidfd (int) */ > > > > struct ucred { > > __u32 pid; > > diff --git a/include/net/scm.h b/include/net/scm.h > > index 585adc1346bd..4617fbc65294 100644 > > --- a/include/net/scm.h > > +++ b/include/net/scm.h > > @@ -124,7 +124,9 @@ static __inline__ void scm_recv(struct socket *sock, struct msghdr *msg, > > struct scm_cookie *scm, int flags) > > { > > if (!msg->msg_control) { > > - if (test_bit(SOCK_PASSCRED, &sock->flags) || scm->fp || > > + if (test_bit(SOCK_PASSCRED, &sock->flags) || > > + test_bit(SOCK_PASSPIDFD, &sock->flags) || > > + scm->fp || > > nit: I'd remove newline here. > > > > scm_has_secdata(sock)) > > msg->msg_flags |= MSG_CTRUNC; > > scm_destroy(scm); > > @@ -141,6 +143,18 @@ static __inline__ void scm_recv(struct socket *sock, struct msghdr *msg, > > put_cmsg(msg, SOL_SOCKET, SCM_CREDENTIALS, sizeof(ucreds), &ucreds); > > } > > > > + if (test_bit(SOCK_PASSPIDFD, &sock->flags)) { > > + int pidfd; > > + > > + if (WARN_ON_ONCE(!scm->pid) || > > + !pid_has_task(scm->pid, PIDTYPE_TGID)) > > Can we change pidfd_create() to return -ESRCH as it has the same test ? I don't think we can do this. pidfd_create() can't distinguish between "this task has been reaped" and "this is a non-thread group leader". So EINVAL is a better hint especially since pidfd_open() is preceeded by a check that would return -ESRCH. It would also be a uapi change for the pidfd_open() syscall to change pidfd_create() like that and for fanotify as well. Now they get EINVAL for a non-thread group leader pid they pass into the system call. Afterwards they'd get ESRCH which is at least misleading if not wrong.
On Thu, Mar 16, 2023 at 04:32:03PM +0100, Aleksandr Mikhalitsyn wrote: > On Thu, Mar 16, 2023 at 3:34 PM Eric Dumazet <edumazet@google.com> wrote: > > > > On Thu, Mar 16, 2023 at 6:16 AM Alexander Mikhalitsyn > > <aleksandr.mikhalitsyn@canonical.com> wrote: > > > > > > 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. > > > > Hi Alexander > > Hi Eric > > Thanks for the fast reply! ;-) > > > > > This would add yet another conditional in af_unix fast path. > > > > It seems that we already can use pidfd_open() (since linux-5.3), and > > pass the resulting fd in af_unix SCM_RIGHTS message ? > > Yes, it's possible, but it means that from the receiver side we need > to trust the sent pidfd (in SCM_RIGHTS), > or always use combination of SCM_RIGHTS+SCM_CREDENTIALS, then we can > extract pidfd from SCM_RIGHTS, > then acquire plain pid from pidfd and after compare it with the pid > from SCM_CREDENTIALS. > > > > > If you think this is not suitable, it should at least be mentioned in > > the changelog. 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
On Fr, 17.03.23 11:20, Christian Brauner (brauner@kernel.org) wrote: > > > It seems that we already can use pidfd_open() (since linux-5.3), and > > > pass the resulting fd in af_unix SCM_RIGHTS message ? 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. Lennart -- Lennart Poettering, Berlin
diff --git a/arch/alpha/include/uapi/asm/socket.h b/arch/alpha/include/uapi/asm/socket.h index 739891b94136..ff310613ae64 100644 --- a/arch/alpha/include/uapi/asm/socket.h +++ b/arch/alpha/include/uapi/asm/socket.h @@ -137,6 +137,8 @@ #define SO_RCVMARK 75 +#define SO_PASSPIDFD 76 + #if !defined(__KERNEL__) #if __BITS_PER_LONG == 64 diff --git a/arch/mips/include/uapi/asm/socket.h b/arch/mips/include/uapi/asm/socket.h index 18f3d95ecfec..762dcb80e4ec 100644 --- a/arch/mips/include/uapi/asm/socket.h +++ b/arch/mips/include/uapi/asm/socket.h @@ -148,6 +148,8 @@ #define SO_RCVMARK 75 +#define SO_PASSPIDFD 76 + #if !defined(__KERNEL__) #if __BITS_PER_LONG == 64 diff --git a/arch/parisc/include/uapi/asm/socket.h b/arch/parisc/include/uapi/asm/socket.h index f486d3dfb6bb..df16a3e16d64 100644 --- a/arch/parisc/include/uapi/asm/socket.h +++ b/arch/parisc/include/uapi/asm/socket.h @@ -129,6 +129,8 @@ #define SO_RCVMARK 0x4049 +#define SO_PASSPIDFD 0x404A + #if !defined(__KERNEL__) #if __BITS_PER_LONG == 64 diff --git a/arch/sparc/include/uapi/asm/socket.h b/arch/sparc/include/uapi/asm/socket.h index 2fda57a3ea86..6e2847804fea 100644 --- a/arch/sparc/include/uapi/asm/socket.h +++ b/arch/sparc/include/uapi/asm/socket.h @@ -130,6 +130,8 @@ #define SO_RCVMARK 0x0054 +#define SO_PASSPIDFD 0x0055 + #if !defined(__KERNEL__) diff --git a/include/linux/net.h b/include/linux/net.h index b73ad8e3c212..c234dfbe7a30 100644 --- a/include/linux/net.h +++ b/include/linux/net.h @@ -43,6 +43,7 @@ struct net; #define SOCK_PASSSEC 4 #define SOCK_SUPPORT_ZC 5 #define SOCK_CUSTOM_SOCKOPT 6 +#define SOCK_PASSPIDFD 7 #ifndef ARCH_HAS_SOCKET_TYPES /** diff --git a/include/linux/socket.h b/include/linux/socket.h index 13c3a237b9c9..6bf90f251910 100644 --- a/include/linux/socket.h +++ b/include/linux/socket.h @@ -177,6 +177,7 @@ static inline size_t msg_data_left(struct msghdr *msg) #define SCM_RIGHTS 0x01 /* rw: access rights (array of int) */ #define SCM_CREDENTIALS 0x02 /* rw: struct ucred */ #define SCM_SECURITY 0x03 /* rw: security label */ +#define SCM_PIDFD 0x04 /* ro: pidfd (int) */ struct ucred { __u32 pid; diff --git a/include/net/scm.h b/include/net/scm.h index 585adc1346bd..4617fbc65294 100644 --- a/include/net/scm.h +++ b/include/net/scm.h @@ -124,7 +124,9 @@ static __inline__ void scm_recv(struct socket *sock, struct msghdr *msg, struct scm_cookie *scm, int flags) { if (!msg->msg_control) { - if (test_bit(SOCK_PASSCRED, &sock->flags) || scm->fp || + if (test_bit(SOCK_PASSCRED, &sock->flags) || + test_bit(SOCK_PASSPIDFD, &sock->flags) || + scm->fp || scm_has_secdata(sock)) msg->msg_flags |= MSG_CTRUNC; scm_destroy(scm); @@ -141,6 +143,18 @@ static __inline__ void scm_recv(struct socket *sock, struct msghdr *msg, put_cmsg(msg, SOL_SOCKET, SCM_CREDENTIALS, sizeof(ucreds), &ucreds); } + if (test_bit(SOCK_PASSPIDFD, &sock->flags)) { + int pidfd; + + if (WARN_ON_ONCE(!scm->pid) || + !pid_has_task(scm->pid, PIDTYPE_TGID)) + pidfd = -ESRCH; + else + pidfd = pidfd_create(scm->pid, 0); + + put_cmsg(msg, SOL_SOCKET, SCM_PIDFD, sizeof(int), &pidfd); + } + scm_destroy_cred(scm); scm_passec(sock, msg, scm); diff --git a/include/uapi/asm-generic/socket.h b/include/uapi/asm-generic/socket.h index 638230899e98..b76169fdb80b 100644 --- a/include/uapi/asm-generic/socket.h +++ b/include/uapi/asm-generic/socket.h @@ -132,6 +132,8 @@ #define SO_RCVMARK 75 +#define SO_PASSPIDFD 76 + #if !defined(__KERNEL__) #if __BITS_PER_LONG == 64 || (defined(__x86_64__) && defined(__ILP32__)) diff --git a/net/core/sock.c b/net/core/sock.c index c25888795390..3f974246ba3e 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -1246,6 +1246,13 @@ int sk_setsockopt(struct sock *sk, int level, int optname, clear_bit(SOCK_PASSCRED, &sock->flags); break; + case SO_PASSPIDFD: + if (valbool) + set_bit(SOCK_PASSPIDFD, &sock->flags); + else + clear_bit(SOCK_PASSPIDFD, &sock->flags); + break; + case SO_TIMESTAMP_OLD: case SO_TIMESTAMP_NEW: case SO_TIMESTAMPNS_OLD: @@ -1737,6 +1744,10 @@ int sk_getsockopt(struct sock *sk, int level, int optname, v.val = !!test_bit(SOCK_PASSCRED, &sock->flags); break; + case SO_PASSPIDFD: + v.val = !!test_bit(SOCK_PASSPIDFD, &sock->flags); + break; + case SO_PEERCRED: { struct ucred peercred; diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c index 8a9656248b0f..bd80e707d0b3 100644 --- a/net/mptcp/sockopt.c +++ b/net/mptcp/sockopt.c @@ -355,6 +355,7 @@ static int mptcp_setsockopt_sol_socket(struct mptcp_sock *msk, int optname, case SO_BROADCAST: case SO_BSDCOMPAT: case SO_PASSCRED: + case SO_PASSPIDFD: case SO_PASSSEC: case SO_RXQ_OVFL: case SO_WIFI_STATUS: diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index 0b0f18ecce44..b0ac768752fa 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -1361,7 +1361,8 @@ static int unix_dgram_connect(struct socket *sock, struct sockaddr *addr, if (err) goto out; - if (test_bit(SOCK_PASSCRED, &sock->flags) && + if ((test_bit(SOCK_PASSCRED, &sock->flags) || + test_bit(SOCK_PASSPIDFD, &sock->flags)) && !unix_sk(sk)->addr) { err = unix_autobind(sk); if (err) @@ -1469,7 +1470,8 @@ static int unix_stream_connect(struct socket *sock, struct sockaddr *uaddr, if (err) goto out; - if (test_bit(SOCK_PASSCRED, &sock->flags) && !u->addr) { + if ((test_bit(SOCK_PASSCRED, &sock->flags) || + test_bit(SOCK_PASSPIDFD, &sock->flags)) && !u->addr) { err = unix_autobind(sk); if (err) goto out; @@ -1670,6 +1672,8 @@ static void unix_sock_inherit_flags(const struct socket *old, { if (test_bit(SOCK_PASSCRED, &old->flags)) set_bit(SOCK_PASSCRED, &new->flags); + if (test_bit(SOCK_PASSPIDFD, &old->flags)) + set_bit(SOCK_PASSPIDFD, &new->flags); if (test_bit(SOCK_PASSSEC, &old->flags)) set_bit(SOCK_PASSSEC, &new->flags); } @@ -1819,8 +1823,10 @@ static bool unix_passcred_enabled(const struct socket *sock, const struct sock *other) { return test_bit(SOCK_PASSCRED, &sock->flags) || + test_bit(SOCK_PASSPIDFD, &sock->flags) || !other->sk_socket || - test_bit(SOCK_PASSCRED, &other->sk_socket->flags); + test_bit(SOCK_PASSCRED, &other->sk_socket->flags) || + test_bit(SOCK_PASSPIDFD, &other->sk_socket->flags); } /* @@ -1922,7 +1928,8 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg, goto out; } - if (test_bit(SOCK_PASSCRED, &sock->flags) && !u->addr) { + if ((test_bit(SOCK_PASSCRED, &sock->flags) || + test_bit(SOCK_PASSPIDFD, &sock->flags)) && !u->addr) { err = unix_autobind(sk); if (err) goto out; @@ -2824,7 +2831,8 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state, /* Never glue messages from different writers */ if (!unix_skb_scm_eq(skb, &scm)) break; - } else if (test_bit(SOCK_PASSCRED, &sock->flags)) { + } else if (test_bit(SOCK_PASSCRED, &sock->flags) || + test_bit(SOCK_PASSPIDFD, &sock->flags)) { /* Copy credentials */ scm_set_cred(&scm, UNIXCB(skb).pid, UNIXCB(skb).uid, UNIXCB(skb).gid); unix_set_secdata(&scm, skb); diff --git a/tools/include/uapi/asm-generic/socket.h b/tools/include/uapi/asm-generic/socket.h index 8756df13be50..fbbc4bf53ee3 100644 --- a/tools/include/uapi/asm-generic/socket.h +++ b/tools/include/uapi/asm-generic/socket.h @@ -121,6 +121,8 @@ #define SO_RCVMARK 75 +#define SO_PASSPIDFD 76 + #if !defined(__KERNEL__) #if __BITS_PER_LONG == 64 || (defined(__x86_64__) && defined(__ILP32__))
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. Cc: "David S. Miller" <davem@davemloft.net> Cc: Eric Dumazet <edumazet@google.com> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Paolo Abeni <pabeni@redhat.com> Cc: Leon Romanovsky <leon@kernel.org> Cc: David Ahern <dsahern@kernel.org> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Kees Cook <keescook@chromium.org> Cc: Christian Brauner <brauner@kernel.org> Cc: linux-kernel@vger.kernel.org Cc: netdev@vger.kernel.org Cc: linux-arch@vger.kernel.org Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> --- 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 | 16 +++++++++++++++- 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, 54 insertions(+), 6 deletions(-)