netdev - Re: [PATCH v1 net 1/3] udp: Update reuse->has_conns under reuseport

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 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]

Date:   Tue, 11 Oct 2022 08:16:18 -0700
From:   Kuniyuki Iwashima <kuniyu@...zon.com>
To:     <pabeni@...hat.com>
CC:     <davem@...emloft.net>, <dsahern@...nel.org>, <edumazet@...gle.com>,
        <kraig@...gle.com>, <kuba@...nel.org>, <kuni1840@...il.com>,
        <kuniyu@...zon.com>, <linux-kernel@...r.kernel.org>,
        <martin.lau@...nel.org>, <netdev@...r.kernel.org>,
        <willemb@...gle.com>, <yoshfuji@...ux-ipv6.org>
Subject: Re: [PATCH v1 net 1/3] udp: Update reuse->has_conns under reuseport_lock.

From:   Paolo Abeni <pabeni@...hat.com>
Date:   Tue, 11 Oct 2022 12:50:52 +0200
> On Mon, 2022-10-10 at 10:43 -0700, Kuniyuki Iwashima wrote:
> > When we call connect() for a UDP socket in a reuseport group, we have
> > to update sk->sk_reuseport_cb->has_conns to 1.  Otherwise, the kernel
> > could select a unconnected socket wrongly for packets sent to the
> > connected socket.
> > 
> > However, the current way to set has_conns is illegal and possible to
> > trigger that problem.  reuseport_has_conns() changes has_conns under
> > rcu_read_lock(), which upgrades the RCU reader to the updater.  Then,
> > it must do the update under the updater's lock, reuseport_lock, but
> > it doesn't for now.
> > 
> > For this reason, there is a race below where we fail to set has_conns
> > resulting in the wrong socket selection.  To avoid the race, let's split
> > the reader and updater with proper locking.
> > 
> >  cpu1                               cpu2
> > +----+                             +----+
> > 
> > __ip[46]_datagram_connect()        reuseport_grow()
> > .                                  .
> > > - reuseport_has_conns(sk, true)   |- more_reuse = __reuseport_alloc(more_socks_size)
> > >  .                               |
> > >  |- rcu_read_lock()
> > >  |- reuse = rcu_dereference(sk->sk_reuseport_cb)
> > >  |
> > >  |                               |  /* reuse->has_conns == 0 here */
> > >  |                               |- more_reuse->has_conns = reuse->has_conns
> > >  |- reuse->has_conns = 1         |  /* more_reuse->has_conns SHOULD BE 1 HERE */
> > >  |                               |
> > >  |                               |- rcu_assign_pointer(reuse->socks[i]->sk_reuseport_cb,
> > >  |                               |                     more_reuse)
> > >  `- rcu_read_unlock()            `- kfree_rcu(reuse, rcu)
> > > 
> > > - sk->sk_state = TCP_ESTABLISHED
> > 
> > Fixes: acdcecc61285 ("udp: correct reuseport selection with connected sockets")
> > Signed-off-by: Kuniyuki Iwashima <kuniyu@...zon.com>
> > ---
> >  include/net/sock_reuseport.h | 23 +++++++++++++++++------
> >  net/ipv4/datagram.c          |  2 +-
> >  net/ipv4/udp.c               |  2 +-
> >  net/ipv6/datagram.c          |  2 +-
> >  net/ipv6/udp.c               |  2 +-
> >  5 files changed, 21 insertions(+), 10 deletions(-)
> > 
> > diff --git a/include/net/sock_reuseport.h b/include/net/sock_reuseport.h
> > index 473b0b0fa4ab..fe9779e6d90f 100644
> > --- a/include/net/sock_reuseport.h
> > +++ b/include/net/sock_reuseport.h
> > @@ -43,21 +43,32 @@ struct sock *reuseport_migrate_sock(struct sock *sk,
> >  extern int reuseport_attach_prog(struct sock *sk, struct bpf_prog *prog);
> >  extern int reuseport_detach_prog(struct sock *sk);
> >  
> > -static inline bool reuseport_has_conns(struct sock *sk, bool set)
> > +static inline bool reuseport_has_conns(struct sock *sk)
> >  {
> >  	struct sock_reuseport *reuse;
> >  	bool ret = false;
> >  
> >  	rcu_read_lock();
> >  	reuse = rcu_dereference(sk->sk_reuseport_cb);
> > -	if (reuse) {
> > -		if (set)
> > -			reuse->has_conns = 1;
> > -		ret = reuse->has_conns;
> > -	}
> > +	if (reuse && reuse->has_conns)
> > +		ret = true;
> >  	rcu_read_unlock();
> >  
> >  	return ret;
> >  }
> >  
> > +static inline void reuseport_has_conns_set(struct sock *sk)
> > +{
> > +	struct sock_reuseport *reuse;
> > +
> > +	if (!rcu_access_pointer(sk->sk_reuseport_cb))
> > +		return;
> > +
> > +	spin_lock(&reuseport_lock);
> > +	reuse = rcu_dereference_protected(sk->sk_reuseport_cb,
> > +					  lockdep_is_held(&reuseport_lock));
> > +	reuse->has_conns = 1;
> > +	spin_unlock(&reuseport_lock);
> > +}
> 
> Since the above is not super critical, it's probably better move it
> into  sock_reuseport.c file and export it (to fix the build issue)

I'll fix the CONFIG_IPV6=m build failure.
Thank you.