From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: Mon, 12 May 2025 22:48:09 -0400
Kuniyuki Iwashima wrote:
quoted
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: Mon, 12 May 2025 15:38:13 -0400
quoted
Kuniyuki Iwashima wrote:
quoted
For SOCK_STREAM embryo sockets, the SO_PASS{CRED,PIDFD,SEC} options
are inherited from the parent listen()ing socket.
Currently, this inheritance happens at accept(), because these
attributes were stored in sk->sk_socket->flags and the struct socket
is not allocated until accept().
This leads to unintentional behaviour.
When a peer sends data to an embryo socket in the accept() queue,
unix_maybe_add_creds() embeds credentials into the skb, even if
neither the peer nor the listener has enabled these options.
If the option is enabled, the embryo socket receives the ancillary
data after accept(). If not, the data is silently discarded.
This conservative approach works for SO_PASS{CRED,PIDFD,SEC}, but not
for SO_PASSRIGHTS; once an SCM_RIGHTS with a hung file descriptor is
sent, it’s game over.
Should this be a fix to net then?
Regarding SO_PASS{CRED,PIDFD,SEC}, this patch is a small optimisation
to save unnecessary get_pid() etc, like 16e572626961
And, SO_PASSRIGHTS is not yet added here, so this is not a fix.
Ack, thanks.
quoted
Maybe I should have clarified like "this works but would not for SO_PASSRIGHTS".
quoted
It depends on the move of this one bit from socket to sock. So is not
a stand-alone patch. But does not need all of the previous cleanup
patches if needs to be backportable.
quoted
To avoid this, we will need to preserve SOCK_PASSRIGHTS even on embryo
sockets.
A recent change made it possible to access the parent's flags in
sendmsg() via unix_sk(other)->listener->sk->sk_socket->flags, but
this introduces an unnecessary condition that is irrelevant for
most sockets, accept()ed sockets and clients.
What is this condition and how is it irrelevant? A constraint on the
kernel having the recent change? I.e., not backportable?
Commit aed6ecef55d7 ("af_unix: Save listener for embryo socket.") is
added for a new GC but is a standalone patch.
If we want to use the listener's flags, the condition will be like...
if (UNIXCB(skb).fp &&
((other->sk_socket && other->sk_socket->sk_flags & SOCK_PASSRIGHTS) ||
(!other->sk_socket && unix_sk(other)->listener->sk->sk_socket->sk_flags && SOCK_PASSRIGHTS)))
No need to change as this stays as net-next.
Might we helpful to replace "a recent commit" with the full commit reference.
Will do.
Thanks!