netdev - Re: [PATCH v1 net 2/5] tcp: Fix bind() regression for v4-mapped-v6 non-wildcard address.

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]

Date: Mon, 11 Sep 2023 11:05:03 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <edumazet@...gle.com>
CC: <davem@...emloft.net>, <dsahern@...nel.org>, <joannelkoong@...il.com>,
	<kuba@...nel.org>, <kuni1840@...il.com>, <kuniyu@...zon.com>,
	<netdev@...r.kernel.org>, <pabeni@...hat.com>
Subject: Re: [PATCH v1 net 2/5] tcp: Fix bind() regression for v4-mapped-v6 non-wildcard address.

From: Eric Dumazet <edumazet@...gle.com>
Date: Mon, 11 Sep 2023 19:51:44 +0200
> On Mon, Sep 11, 2023 at 6:52 PM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
> >
> > Since bhash2 was introduced, the example below does now work as expected.
> > These two bind() should conflict, but the 2nd bind() now succeeds.
> >
> >   from socket import *
> >
> >   s1 = socket(AF_INET6, SOCK_STREAM)
> >   s1.bind(('::ffff:127.0.0.1', 0))
> >
> >   s2 = socket(AF_INET, SOCK_STREAM)
> >   s2.bind(('127.0.0.1', s1.getsockname()[1]))
> >
> > During the 2nd bind() in inet_csk_get_port(), inet_bind2_bucket_find()
> > fails to find the 1st socket's tb2, so inet_bind2_bucket_create() allocates
> > a new tb2 for the 2nd socket.  Then, we call inet_csk_bind_conflict() that
> > checks conflicts in the new tb2 by inet_bhash2_conflict().  However, the
> > new tb2 does not include the 1st socket, thus the bind() finally succeeds.
> >
> > In this case, inet_bind2_bucket_match() must check if AF_INET6 tb2 has
> > the conflicting v4-mapped-v6 address so that inet_bind2_bucket_find()
> > returns the 1st socket's tb2.
> >
> > Note that if we bind two sockets to 127.0.0.1 and then ::FFFF:127.0.0.1,
> > the 2nd bind() fails properly for the same reason mentinoed in the previous
> > commit.
> >
> > Fixes: 28044fc1d495 ("net: Add a bhash2 table hashed by port and address")
> > Signed-off-by: Kuniyuki Iwashima <kuniyu@...zon.com>
> > ---
> >  net/ipv4/inet_hashtables.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
> > index 0a9b20eb81c4..54505100c914 100644
> > --- a/net/ipv4/inet_hashtables.c
> > +++ b/net/ipv4/inet_hashtables.c
> > @@ -816,8 +816,15 @@ static bool inet_bind2_bucket_match(const struct inet_bind2_bucket *tb,
> >                                     int l3mdev, const struct sock *sk)
> >  {
> >  #if IS_ENABLED(CONFIG_IPV6)
> > -       if (sk->sk_family != tb->family)
> > +       if (sk->sk_family != tb->family) {
> > +               if (sk->sk_family == AF_INET)
> > +                       return net_eq(ib2_net(tb), net) && tb->port == port &&
> > +                               tb->l3mdev == l3mdev &&
> > +                               ipv6_addr_v4mapped(&tb->v6_rcv_saddr) &&
> > +                               tb->v6_rcv_saddr.s6_addr32[3] == sk->sk_rcv_saddr;
> > +
> >                 return false;
> > +       }
> >
> >         if (sk->sk_family == AF_INET6)
> >                 return net_eq(ib2_net(tb), net) && tb->port == port &&
> > --
> 
> Could we first factorize all these "net_eq(ib2_net(tb), net) &&
> tb->port == port" checks ?

That's much cleaner :)
I'll add a prep patch first in v2.

Thanks!

> 
> diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
> index 7876b7d703cb5647086c45ca547c4caadc00c091..6240c802ed772272028e6e65bf90f345dd2d1619
> 100644
> --- a/net/ipv4/inet_hashtables.c
> +++ b/net/ipv4/inet_hashtables.c
> @@ -832,24 +832,24 @@ static bool inet_bind2_bucket_match(const struct
> inet_bind2_bucket *tb,
>  bool inet_bind2_bucket_match_addr_any(const struct inet_bind2_bucket
> *tb, const struct net *net,
>                                       unsigned short port, int l3mdev,
> const struct sock *sk)
>  {
> +       if (!net_eq(ib2_net(tb), net) || tb->port != port)
> +               return false;
> +
>  #if IS_ENABLED(CONFIG_IPV6)
>         if (sk->sk_family != tb->family) {
>                 if (sk->sk_family == AF_INET)
> -                       return net_eq(ib2_net(tb), net) && tb->port == port &&
> -                               tb->l3mdev == l3mdev &&
> +                       return  tb->l3mdev == l3mdev &&
>                                 ipv6_addr_any(&tb->v6_rcv_saddr);
> 
>                 return false;
>         }
> 
>         if (sk->sk_family == AF_INET6)
> -               return net_eq(ib2_net(tb), net) && tb->port == port &&
> -                       tb->l3mdev == l3mdev &&
> +               return  tb->l3mdev == l3mdev &&
>                         ipv6_addr_any(&tb->v6_rcv_saddr);
>         else
>  #endif
> -               return net_eq(ib2_net(tb), net) && tb->port == port &&
> -                       tb->l3mdev == l3mdev && tb->rcv_saddr == 0;
> +               return tb->l3mdev == l3mdev && tb->rcv_saddr == 0;
>  }
> 
>  /* The socket's bhash2 hashbucket spinlock must be held when this is called */