| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAAVpQUDFzPBJmCeawhaHL5Twjxk8obLZW9UPH0HfD_5BYpjh_w@mail.gmail.com>
Date: Wed, 2 Jul 2025 18:53:21 -0700
From: Kuniyuki Iwashima <kuniyu@...gle.com>
To: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@...onical.com>
Cc: linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>,
Leon Romanovsky <leon@...nel.org>, Arnd Bergmann <arnd@...db.de>, Christian Brauner <brauner@...nel.org>,
Lennart Poettering <mzxreary@...inter.de>, Luca Boccassi <bluca@...ian.org>,
David Rheinsberg <david@...dahead.eu>, Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH net-next v2 4/6] af_unix: stash pidfs dentry when needed
On Tue, Jul 1, 2025 at 1:41 AM Alexander Mikhalitsyn
<aleksandr.mikhalitsyn@...onical.com> wrote:
>
> We need to ensure that pidfs dentry is allocated when we meet any
> struct pid for the first time. This will allows us to open pidfd
> even after the task it corresponds to is reaped.
>
> Basically, we need to identify all places where we fill skb/scm_cookie
> with struct pid reference for the first time and call pidfs_register_pid().
>
> Tricky thing here is that we have a few places where this happends
> depending on what userspace is doing:
> - [__scm_replace_pid()] explicitly sending an SCM_CREDENTIALS message
> and specified pid in a numeric format
> - [unix_maybe_add_creds()] enabled SO_PASSCRED/SO_PASSPIDFD but
> didn't send SCM_CREDENTIALS explicitly
> - [scm_send()] force_creds is true. Netlink case.
>
> Cc: linux-kernel@...r.kernel.org
> Cc: netdev@...r.kernel.org
> Cc: "David S. Miller" <davem@...emloft.net>
> Cc: Eric Dumazet <edumazet@...gle.com>
> Cc: Jakub Kicinski <kuba@...nel.org>
> Cc: Paolo Abeni <pabeni@...hat.com>
> Cc: Simon Horman <horms@...nel.org>
> Cc: Leon Romanovsky <leon@...nel.org>
> Cc: Arnd Bergmann <arnd@...db.de>
> Cc: Christian Brauner <brauner@...nel.org>
> Cc: Kuniyuki Iwashima <kuniyu@...gle.com>
> Cc: Lennart Poettering <mzxreary@...inter.de>
> Cc: Luca Boccassi <bluca@...ian.org>
> Cc: David Rheinsberg <david@...dahead.eu>
> Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@...onical.com>
> ---
> v2:
> - renamed __skb_set_pid() -> unix_set_pid_to_skb() [ as Kuniyuki suggested ]
> - get rid of extra helper (__scm_set_cred()) I've introduced before [ as Kuniyuki suggested ]
> - s/__inline__/inline/ for functions I touched [ as Kuniyuki suggested ]
> - get rid of chunk in unix_destruct_scm() with NULLifying UNIXCB(skb).pid [ as Kuniyuki suggested ]
> - added proper error handling in scm_send() for scm_set_cred() return value [ found by me during rework ]
> ---
> include/net/scm.h | 32 ++++++++++++++++++++++++--------
> net/core/scm.c | 6 ++++++
> net/unix/af_unix.c | 33 +++++++++++++++++++++++++++++----
> 3 files changed, 59 insertions(+), 12 deletions(-)
>
> diff --git a/include/net/scm.h b/include/net/scm.h
> index 84c4707e78a5..597a40779269 100644
> --- a/include/net/scm.h
> +++ b/include/net/scm.h
> @@ -8,6 +8,7 @@
> #include <linux/file.h>
> #include <linux/security.h>
> #include <linux/pid.h>
> +#include <linux/pidfs.h>
> #include <linux/nsproxy.h>
> #include <linux/sched/signal.h>
> #include <net/compat.h>
> @@ -66,19 +67,28 @@ static __inline__ void unix_get_peersec_dgram(struct socket *sock, struct scm_co
> { }
> #endif /* CONFIG_SECURITY_NETWORK */
>
> -static __inline__ void scm_set_cred(struct scm_cookie *scm,
> - struct pid *pid, kuid_t uid, kgid_t gid)
> +static inline int scm_set_cred(struct scm_cookie *scm,
> + struct pid *pid, bool pidfs_register,
> + kuid_t uid, kgid_t gid)
> {
> - scm->pid = get_pid(pid);
> + if (pidfs_register) {
> + int err = pidfs_register_pid(pid);
> + if (err)
> + return err;
> + }
> +
> + scm->pid = get_pid(pid);
> +
> scm->creds.pid = pid_vnr(pid);
> scm->creds.uid = uid;
> scm->creds.gid = gid;
> + return 0;
> }
>
> static __inline__ void scm_destroy_cred(struct scm_cookie *scm)
> {
> put_pid(scm->pid);
> - scm->pid = NULL;
> + scm->pid = NULL;
Could you split these double-space changes to another
patch to make review easier ?
> }
>
> static __inline__ void scm_destroy(struct scm_cookie *scm)
> @@ -88,14 +98,20 @@ static __inline__ void scm_destroy(struct scm_cookie *scm)
> __scm_destroy(scm);
> }
>
> -static __inline__ int scm_send(struct socket *sock, struct msghdr *msg,
> - struct scm_cookie *scm, bool forcecreds)
> +static inline int scm_send(struct socket *sock, struct msghdr *msg,
> + struct scm_cookie *scm, bool forcecreds)
> {
> memset(scm, 0, sizeof(*scm));
> scm->creds.uid = INVALID_UID;
> scm->creds.gid = INVALID_GID;
> - if (forcecreds)
> - scm_set_cred(scm, task_tgid(current), current_uid(), current_gid());
> +
> + if (forcecreds) {
> + int err = scm_set_cred(scm, task_tgid(current), true,
> + current_uid(), current_gid());
Do we need to pass true here ?
Given this series affects scm_pidfd_recv(), we don't need to
touch netlink path that is not allowed to call scm_recv_unix() ?
Then, all callers pass false to scm_set_cred() and
pidfs_register_pid() there will be unnecessary.
> + if (err)
> + return err;
> + }
> +
> unix_get_peersec_dgram(sock, scm);
> if (msg->msg_controllen <= 0)
> return 0;
> diff --git a/net/core/scm.c b/net/core/scm.c
> index 68441c024dd8..50dfec6f8a2b 100644
> --- a/net/core/scm.c
> +++ b/net/core/scm.c
> @@ -147,9 +147,15 @@ EXPORT_SYMBOL(__scm_destroy);
>
> static inline int __scm_replace_pid(struct scm_cookie *scm, struct pid *pid)
> {
> + int err;
> +
> /* drop all previous references */
> scm_destroy_cred(scm);
>
> + err = pidfs_register_pid(pid);
> + if (err)
> + return err;
> +
> scm->pid = pid;
> scm->creds.pid = pid_vnr(pid);
> return 0;
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index df2174d9904d..18c677683ddc 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -1924,12 +1924,27 @@ static void unix_peek_fds(struct scm_cookie *scm, struct sk_buff *skb)
> scm->fp = scm_fp_dup(UNIXCB(skb).fp);
> }
>
> +static int unix_set_pid_to_skb(struct sk_buff *skb, struct pid *pid, bool pidfs_register)
> +{
> + if (pidfs_register) {
> + int err;
> +
> + err = pidfs_register_pid(pid);
> + if (err)
> + return err;
> + }
> +
> + UNIXCB(skb).pid = get_pid(pid);
> + return 0;
> +}
> +
> static void unix_destruct_scm(struct sk_buff *skb)
> {
> struct scm_cookie scm;
>
> memset(&scm, 0, sizeof(scm));
> - scm.pid = UNIXCB(skb).pid;
> + scm.pid = UNIXCB(skb).pid;
> +
> if (UNIXCB(skb).fp)
> unix_detach_fds(&scm, skb);
>
> @@ -1943,7 +1958,10 @@ static int unix_scm_to_skb(struct scm_cookie *scm, struct sk_buff *skb, bool sen
> {
> int err = 0;
>
> - UNIXCB(skb).pid = get_pid(scm->pid);
> + err = unix_set_pid_to_skb(skb, scm->pid, false);
> + if (unlikely(err))
This does not fail too.
Perhaps keep get_pid() here and move pidfs_register_pid()
to unix_maybe_add_creds(), that will look simpler.
> + return err;
> +
> UNIXCB(skb).uid = scm->creds.uid;
> UNIXCB(skb).gid = scm->creds.gid;
> UNIXCB(skb).fp = NULL;
> @@ -1957,7 +1975,8 @@ static int unix_scm_to_skb(struct scm_cookie *scm, struct sk_buff *skb, bool sen
>
> static void unix_skb_to_scm(struct sk_buff *skb, struct scm_cookie *scm)
> {
> - scm_set_cred(scm, UNIXCB(skb).pid, UNIXCB(skb).uid, UNIXCB(skb).gid);
> + /* scm_set_cred() can't fail when pidfs_register == false */
> + scm_set_cred(scm, UNIXCB(skb).pid, false, UNIXCB(skb).uid, UNIXCB(skb).gid);
> unix_set_secdata(scm, skb);
> }
>
> @@ -1971,6 +1990,7 @@ static void unix_skb_to_scm(struct sk_buff *skb, struct scm_cookie *scm)
> * We include credentials if source or destination socket
> * asserted SOCK_PASSCRED.
> *
> + * Context: May sleep.
> * Return: On success zero, on error a negative error code is returned.
> */
> static int unix_maybe_add_creds(struct sk_buff *skb, const struct sock *sk,
> @@ -1980,7 +2000,12 @@ static int unix_maybe_add_creds(struct sk_buff *skb, const struct sock *sk,
> return 0;
>
> if (unix_may_passcred(sk) || unix_may_passcred(other)) {
I forgot to mention that this part will conflict with net-next.
I guess Christian will take this series via vfs tree ?
> - UNIXCB(skb).pid = get_pid(task_tgid(current));
> + int err;
> +
> + err = unix_set_pid_to_skb(skb, task_tgid(current), true);
> + if (unlikely(err))
> + return err;
> +
> current_uid_gid(&UNIXCB(skb).uid, &UNIXCB(skb).gid);
> }
>
> --
> 2.43.0
>
Powered by blists - more mailing lists