netdev - Re: [PATCH v2 net-next 7/9] af_unix: Inherit sk

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250512223729.58686-1-kuniyu@amazon.com>
Date: Mon, 12 May 2025 15:34:55 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <willemdebruijn.kernel@...il.com>
CC: <brauner@...nel.org>, <davem@...emloft.net>, <edumazet@...gle.com>,
	<horms@...nel.org>, <kuba@...nel.org>, <kuni1840@...il.com>,
	<kuniyu@...zon.com>, <netdev@...r.kernel.org>, <pabeni@...hat.com>,
	<willemb@...gle.com>
Subject: Re: [PATCH v2 net-next 7/9] af_unix: Inherit sk_flags at connect().

From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
Date: Mon, 12 May 2025 15:38:13 -0400
> Kuniyuki Iwashima wrote:
> > 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.

Maybe I should have clarified like "this works but would not for SO_PASSRIGHTS".


> 
> 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.
> 
> > 
> > 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)))