netdev - Re: [PATCH net v3] sunrpc: fix one UAF issue caused by sunrpc kernel tcp socket

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: <20241112001308.58355-1-kuniyu@amazon.com>
Date: Mon, 11 Nov 2024 16:13:08 -0800
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <neilb@...e.de>
CC: <Dai.Ngo@...cle.com>, <anna@...nel.org>, <chuck.lever@...cle.com>,
	<davem@...emloft.net>, <ebiederm@...ssion.com>, <edumazet@...gle.com>,
	<horms@...nel.org>, <jlayton@...nel.org>, <kuba@...nel.org>,
	<kuniyu@...zon.com>, <linux-nfs@...r.kernel.org>, <liujian56@...wei.com>,
	<netdev@...r.kernel.org>, <okorniev@...hat.com>, <pabeni@...hat.com>,
	<tom@...pey.com>, <trondmy@...nel.org>
Subject: Re: [PATCH net v3] sunrpc: fix one UAF issue caused by sunrpc kernel tcp socket

From: "NeilBrown" <neilb@...e.de>
Date: Tue, 12 Nov 2024 10:52:34 +1100
> > diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> > index 6f272013fd9b..d4330aaadc23 100644
> > --- a/net/sunrpc/svcsock.c
> > +++ b/net/sunrpc/svcsock.c
> > @@ -1551,6 +1551,10 @@ static struct svc_xprt *svc_create_socket(struct svc_serv *serv,
> >  	newlen = error;
> >  
> >  	if (protocol == IPPROTO_TCP) {
> > +		__netns_tracker_free(net, &sock->sk->ns_tracker, false);
> > +		sock->sk->sk_net_refcnt = 1;
> > +		get_net_track(net, &sock->sk->ns_tracker, GFP_KERNEL);
> > +		sock_inuse_add(net, 1);
> 
> This is really ugly.  These internal details of the network layer have
> no place in sunrpc code. There must be a better way.

I asked to do this way.  I agree this way is really ugly.  Similar
code exists in MPTCP, SMC, CIFS, etc, so I plan to add a new API for
this case, but this requires huge change adding a new parameter for
->create() prototype and the changes are not backportable.

https://github.com/q2ven/linux/commit/bb8b8814a73b3f50c3fef5eaf8d30d8c1df43e7b
https://github.com/q2ven/linux/commits/427_2

After my series, we can use the following but cannot backport it to
stable.

  sock_create_net(net, family, type, protocol);

  e.g. commit for MPTCP
  https://github.com/q2ven/linux/commit/24a4647561272c1e67a685d8403e27eb863398cf

That's why I suggested to go with the ugly way and I will clean them
up in the next cycle.

So, finally the sunrpc code will be much cleaner and the netns refcnt
will be touched only in the core code.


> 
> Can we pass '0' for the kern arg to __sock_create()?  That should fix
> the refcounting issues, but might mess up security labelling.

This should be avoided as it's confusing for BPF programs, LSMs, and
LOCKDEP.


> 
> Can we wait for something before we call put_net() to release the net.
> 
> Maybe we want to split the "kern" arg t __sock_create() and have
> "kern" which affects labeling and "refnet" with affects refcounting the
> net.

This is exactly what my series does, but again, it's not backport
friendly.
https://github.com/q2ven/linux/commit/413e867b4aee9e9f60f3c33fb38d2004aeb29c40


> 
> I had a quick look and very nearly every caller of __sock_create()
> outside of net/core really does want refcount.  Many callers of
> sock_create_kern() possibly don't.

Actually, since sock_create_kern() is added, we no longer need to
export __sock_create(), so I have a patch to convert them to
sock_create_kern().

And most of TCP socket does need refcnt, but non-TCP won't.
Also, handshake one is exception, which uses TCP but only in init_net,
where we need not take care of netns refcnt.

https://github.com/q2ven/linux/commit/b56888bbbf327d57ea25a6b97275d6b9b8ad043a



> 
> So I really think this needs to be cleaned up in net/core, not in all
> the different network clients in the kernel.

Yes, will be done in the next cycle.