Re: [PATCH net] netlink: render SCM_CREDENTIALS pid in the receiver's pid namespace

[Alexander Mikhalitsyn]

Am Fr., 4. Sept. 2026 um 17:48 Uhr schrieb Eric Dumazet <edumazet@xxxxxxxxxx>:
>
> On Fri, Sep 4, 2026 at 4:46 PM Maoyi Xie <maoyixie.tju@xxxxxxxxx> wrote:
> >
> > __scm_recv_common() translates uid and gid into the reader's user
> > namespace but copies the pid as is. AF_UNIX gets away with that because
> > unix_skb_to_scm() re-renders the pid with pid_vnr() at recvmsg time.
> > netlink_sendmsg() renders it in the sender's namespace and stores a bare
> > u32, so a reader in another pid namespace sees a number from a namespace
> > it is not in. The sender chooses that number. An unprivileged sender in a
> > child namespace made the receiver see pid 300.
> >
> > Carry the sender's struct pid in NETLINK_CB and hand it to scm_set_cred()
> > in netlink_recvmsg(), the way af_unix does. netlink_skb_set_owner_r()
> > takes the reference and netlink_skb_destructor() drops it. A reader in a
> > namespace the sender has no pid in now gets 0, like AF_UNIX.
> >
> > I found this with a CodeQL checker. I used Claude to help write the
> > reproducers. The ones that reproduce the bug run unprivileged and need no
> > kernel changes. Tested on net with KASAN and lockdep, no reports. The tree
> > has no netlink SCM selftest.
> >
> > Fixes: b488893a390e ("pid namespaces: changes to show virtual ids to user")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Assisted-by: Claude:claude-opus-5 codeql
> > Signed-off-by: Maoyi Xie <maoyixie.tju@xxxxxxxxx>
> > ---
> >  include/linux/netlink.h  |  9 +++++++++
> >  net/netlink/af_netlink.c | 30 ++++++++++++++++++++++++++++--
> >  2 files changed, 37 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/netlink.h b/include/linux/netlink.h
> > index 882e9c1b6c1dcc..26266754e27c14 100644
> > --- a/include/linux/netlink.h
> > +++ b/include/linux/netlink.h
> > @@ -30,6 +30,15 @@ struct netlink_skb_parms {
> >         struct sock             *sk;
> >         bool                    nsid_is_set;
> >         int                     nsid;
> > +       /*
> > +        * Sender's struct pid.  netlink_sendmsg() stores a borrowed pointer
> > +        * taken from its own scm_cookie.  netlink_skb_set_owner_r() takes a
> > +        * reference when it takes ownership of the skb for a receiver, and
> > +        * netlink_skb_destructor() drops that reference.  A clone starts out
> > +        * borrowing again, because __skb_clone() clears both skb->sk and
> > +        * skb->destructor.  NULL for a kernel generated skb.
> > +        */
> > +       struct pid              *pid;
> >  };
> >
> >  #define NETLINK_CB(skb)                (*(struct netlink_skb_parms*)&((skb)->cb))
> > diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> > index e6b1d9758c9c92..170d90d472a0db 100644
> > --- a/net/netlink/af_netlink.c
> > +++ b/net/netlink/af_netlink.c
> > @@ -379,13 +379,24 @@ static void netlink_skb_destructor(struct sk_buff *skb)
> >
> >                 skb->head = NULL;
> >         }
> > -       if (skb->sk != NULL)
> > +       if (skb->sk) {
> > +               /*
> > +                * The reference is held for as long as skb->sk is set, taken
> > +                * in netlink_skb_set_owner_r() and dropped here.  The pointer
> > +                * is left in place: do_one_broadcast() orphans an skb one
> > +                * listener owned and hands it to the next, which takes its
> > +                * own reference, and the sender's scm_cookie keeps the pid
> > +                * alive across the whole broadcast.
> > +                */
> > +               put_pid(NETLINK_CB(skb).pid);
>

Dear friends,

> I really have a bad feeling about this dance and these assumptions,
> maybe this is just me.

completely agree with Eric.

1. We need to start from use-case here, because netlink sockets are
quite special in many ways.
     If you just found this using LLM and now we are going to change a
behavior that was there for more than 22 years
     ( just look
https://github.com/torvalds/linux/blame/d679c5324d9a87c6295f56c2dea52d5f68834f41/include/linux/netlink.h#L165
)
2. If you start tracking struct pid (which is a good idea
theoretically, if we ignore 1.), then we should do it a bit smarter
and
    make use pidfs API [pidfs_register_pid() function] like we do it
in unix_maybe_add_creds() to make sure that we properly support
    dead pidfds too.

Kind regards,
Alex

>
> Please include a dedicated selftest in tools/testing/selftests/net/
> exercising SO_PASSCRED / SCM_CREDENTIALS
> over netlink across PID namespaces to demonstrate the issue and
> validate the proposed changes.
>
> Thanks.
>
> >                 sock_rfree(skb);
> > +       }
> >  }
> >
> >  static void netlink_skb_set_owner_r(struct sk_buff *skb, struct sock *sk)
> >  {
> >         WARN_ON(skb->sk != NULL);
> > +       NETLINK_CB(skb).pid = get_pid(NETLINK_CB(skb).pid);
> >         skb->sk = sk;
> >         skb->destructor = netlink_skb_destructor;
> >         sk_mem_charge(sk, skb->truesize);
> > @@ -1880,6 +1891,13 @@ static int netlink_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
> >         NETLINK_CB(skb).dst_group = dst_group;
> >         NETLINK_CB(skb).creds   = scm.creds;
> >         NETLINK_CB(skb).flags   = netlink_skb_flags;
> > +       /*
> > +        * Borrowed here.  scm_destroy() below drops the scm_cookie's own
> > +        * reference, and every delivery in between is synchronous, so the
> > +        * pointer stays valid until netlink_skb_set_owner_r() takes a
> > +        * reference of its own.
> > +        */
> > +       NETLINK_CB(skb).pid     = scm.pid;
> >
> >         err = -EFAULT;
> >         if (memcpy_from_msg(skb_put(skb, len), msg, len)) {
> > @@ -1971,7 +1989,15 @@ static int netlink_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> >                 netlink_cmsg_listen_all_nsid(sk, msg, skb);
> >
> >         memset(&scm, 0, sizeof(scm));
> > -       scm.creds = *NETLINK_CREDS(skb);
> > +       /*
> > +        * Render the sender's pid in the reader's pid namespace, the way
> > +        * unix_skb_to_scm() does through scm_set_cred().  A NULL pid gives 0,
> > +        * so a control block that lost its reference reports "unknown" rather
> > +        * than the sender's own untranslated number.  scm_recv() below drops
> > +        * the reference taken here on both of its paths.
> > +        */
> > +       scm_set_cred(&scm, NETLINK_CB(skb).pid, NETLINK_CREDS(skb)->uid,
> > +                    NETLINK_CREDS(skb)->gid);
> >         if (flags & MSG_TRUNC)
> >                 copied = data_skb->len;
> >
> > --
> > 2.34.1
> >