| 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
| ||
|
Message-ID: <20230912075907.91325-1-kuniyu@amazon.com>
Date: Tue, 12 Sep 2023 00:59:07 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <avagin@...il.com>
CC: <davem@...emloft.net>, <dsahern@...nel.org>, <edumazet@...gle.com>,
<joannelkoong@...il.com>, <kuba@...nel.org>, <kuni1840@...il.com>,
<kuniyu@...zon.com>, <netdev@...r.kernel.org>, <pabeni@...hat.com>
Subject: Re: [PATCH v2 net 3/6] tcp: Fix bind() regression for v4-mapped-v6 non-wildcard address.
From: Andrei Vagin <avagin@...il.com>
Date: Tue, 12 Sep 2023 00:25:58 -0700
> On Mon, Sep 11, 2023 at 11:36:57AM -0700, Kuniyuki Iwashima wrote:
> > Since bhash2 was introduced, the example below does not 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 | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
> > index a58b04052ca6..c32f5e28758b 100644
> > --- a/net/ipv4/inet_hashtables.c
> > +++ b/net/ipv4/inet_hashtables.c
> > @@ -820,8 +820,13 @@ static bool inet_bind2_bucket_match(const struct inet_bind2_bucket *tb,
>
> Should we fix inet_bind2_bucket_addr_match too?
No, there's no real bug.
I have this patch in my local branch and will post it against
net-next after this series is merged.
---8<---
commit 06333d4b0d053e4c0d40090b29e5a8546b42bb50
Author: Kuniyuki Iwashima <kuniyu@...zon.com>
Date: Sun Sep 10 19:01:23 2023 +0000
tcp: Remove redundant sk_family check in inet_bind2_bucket_addr_match().
Commit 5456262d2baa ("net: Fix incorrect address comparison when
searching for a bind2 bucket") added the test for the KMSAN report.
However, the condition never be true as tb2 is listener's
inet_csk(sk)->icsk_bind2_hash and its sk_family always matches with
child->sk_family.
Link: https://lore.kernel.org/netdev/CAG_fn=Ud3zSW7AZWXc+asfMhZVL5ETnvuY44Pmyv4NPv-ijN-A@mail.gmail.com/
Signed-off-by: Kuniyuki Iwashima <kuniyu@...zon.com>
diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index c32f5e28758b..dfb1c61c0c2b 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -149,9 +149,6 @@ static bool inet_bind2_bucket_addr_match(const struct inet_bind2_bucket *tb2,
const struct sock *sk)
{
#if IS_ENABLED(CONFIG_IPV6)
- if (sk->sk_family != tb2->family)
- return false;
-
if (sk->sk_family == AF_INET6)
return ipv6_addr_equal(&tb2->v6_rcv_saddr,
&sk->sk_v6_rcv_saddr);
---8<---
>
> > return false;
> >
> > #if IS_ENABLED(CONFIG_IPV6)
> > - if (sk->sk_family != tb->family)
> > + if (sk->sk_family != tb->family) {
> > + if (sk->sk_family == AF_INET)
> > + return ipv6_addr_v4mapped(&tb->v6_rcv_saddr) &&
> > + tb->v6_rcv_saddr.s6_addr32[3] == sk->sk_rcv_saddr;
>
> I was wondering why we don't check a case when sk is AF_INET6 and tb is
> AF_INET. I tried to run the next test:
>
> import socket
> sk4 = socket.socket(socket.AF_INET, socket.SOCK_STREAM, 0)
> sk6 = socket.socket(socket.AF_INET6, socket.SOCK_STREAM, 0)
> sk4.bind(("127.0.0.1", 32773))
> sk6.bind(("::ffff:127.0.0.1", 32773))
>
> The second bind returned EADDRINUSE. It works as expected only because
> inet_use_bhash2_on_bind returns false for all v4mapped addresses. This
> doesn't look right, and I am not sure it was intentional. I think it can
> to be changed this way:
>
> @@ -158,7 +158,7 @@ static bool inet_use_bhash2_on_bind(const struct sock *sk)
> int addr_type = ipv6_addr_type(&sk->sk_v6_rcv_saddr);
>
> return addr_type != IPV6_ADDR_ANY &&
> - addr_type != IPV6_ADDR_MAPPED;
> + !ipv6_addr_v4mapped_any(&sk->sk_v6_rcv_saddr);
> }
> #endif
> return sk->sk_rcv_saddr != htonl(INADDR_ANY);
>
> As for this patch, I think it may be a good idea if bind2 buckets for
> v4-mapped addresses are created with the AF_INET family and matching
> ipv4 addresses.
Let's say we create tb2 with AF_INET for v4-mapped address. If we bind
::ffff:127.0.0.1 and 127.0.0.1, in the second bind(), both tb->family and
sk->sk_family is AF_INET. So, we can remove this AF_INET test.
if (sk->sk_family != tb->family) {
if (sk->sk_family == AF_INET)
But what about 127.0.0.1 and then ::ffff:127.0.0.1 ? There tb->family is
AF_INET and sk->sk_family is AF_INET6. We need to add another AF_INET6
test in the same place.
So, finally we need to check the special case in either way.
Also, as you may notice, we need to change inet_bind2_bucket_addr_match()
as well. As mentioned in my patch above, sk->sk_family always match
tb2->family there, but v4-mapped AF_INET tb2 breaks the rule.
Using bhash2 for v4-mapped-v6 address could be done but churns code a lot.
So, I think we should not include such changes as fix at least.
>
> > +
> > return false;
> > + }
> >
> > if (sk->sk_family == AF_INET6)
> > return ipv6_addr_equal(&tb->v6_rcv_saddr, &sk->sk_v6_rcv_saddr);
Powered by blists - more mailing lists