net: Fix incorrect address comparison when searching for a bind2 bucket
Subsystems (3)
PATCH 1/1 net: Fix incorrect address comparison when searching for a bind2 bucket
From: Martin KaFai Lau <[email protected]>
The v6_rcv_saddr and rcv_saddr are inside a union in the 'struct inet_bind2_bucket'. When searching a bucket by following the bhash2 hashtable chain, eg. inet_bind2_bucket_match, it is only using the sk->sk_family and there is no way to check if the inet_bind2_bucket has a v6 or v4 address in the union. This leads to an uninit-value KMSAN report in [0] and also potentially incorrect matches.
This patch fixes it by adding a family member to the inet_bind2_bucket and then tests 'sk->sk_family != tb->family' before matching the sk's address to the tb's address.
--- include/net/inet_hashtables.h | 3 +++ net/ipv4/inet_hashtables.c | 10 ++++++++++ 2 files changed, 13 insertions(+)
include/net/inet_hashtables.h +3 -0
@@ -95,6 +95,9 @@ struct inet_bind2_bucket {possible_net_t ib_net;int l3mdev;unsigned short port; +#if IS_ENABLED (CONFIG_IPV6 ) +unsigned short family; +#endif union {#if IS_ENABLED (CONFIG_IPV6 )struct in6_addr v6_rcv_saddr;
net/ipv4/inet_hashtables.c +10 -0
@@ -109,6 +109,7 @@ static void inet_bind2_bucket_init(struct inet_bind2_bucket *tb, tb->l3mdev = l3mdev; tb->port = port;#if IS_ENABLED (CONFIG_IPV6 ) + tb->family = sk->sk_family;if (sk->sk_family ==AF_INET6 ) tb->v6_rcv_saddr = sk->sk_v6_rcv_saddr;else
@@ -146,6 +147,9 @@ 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);
@@ -791,6 +795,9 @@ 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) + return false; +if (sk->sk_family ==AF_INET6 )return net_eq (ib2_net (tb), net) && tb->port == port && tb->l3mdev == l3mdev &&
@@ -807,6 +814,9 @@ bool inet_bind2_bucket_match_addr_any(const struct inet_bind2_bucket *tb, const#if IS_ENABLED (CONFIG_IPV6 )struct in6_addr addr_any = {}; +if (sk->sk_family != tb->family) +return false; +if (sk->sk_family ==AF_INET6 )return net_eq (ib2_net (tb), net) && tb->port == port && tb->l3mdev == l3mdev &&
Signature · 1 line
2.30.2
Quoted 9 lines · Martin KaFai Lau
From: Martin KaFai Lau <[email protected]>
The
v6_rcv_saddrandrcv_saddrare inside a union in the 'structinet_bind2_bucket'. When searching a bucket by following the bhash2 hashtable chain, eg.inet_bind2_bucket_match, it is only using thesk->sk_familyand there is no way to check if theinet_bind2_buckethas a v6 or v4 address in the union. This leads to an uninit-value KMSAN report in [0] and also potentially incorrect matches.
I do not see the KMSAN report, is it missing from this changelog ?
Thanks.
Quoted 75 lines
This patch fixes it by adding a family member to the
inet_bind2_bucketand then tests 'sk->sk_family!=tb->family' before matching the sk's address to the tb's address.Cc Joanne Koong <[email protected]>Cc Alexander Potapenko <[email protected]>Fixes 28044fc1d495 ("net: Add a bhash2 table hashed by port and address")Signed-off-by Martin KaFai Lau <[email protected]>--- include/net/inet_hashtables.h | 3 +++ net/ipv4/inet_hashtables.c | 10 ++++++++++ 2 files changed, 13 insertions(+)include/net/inet_hashtables.h +3
-0@@ -95,6 +95,9 @@ struct inet_bind2_bucket {possible_net_t ib_net;int l3mdev;unsigned short port; +#if IS_ENABLED (CONFIG_IPV6 ) +unsigned short family; +#endif union {#if IS_ENABLED (CONFIG_IPV6 )struct in6_addr v6_rcv_saddr;net/ipv4/inet_hashtables.c +10
-1@@ -109,6 +109,7 @@ static void inet_bind2_bucket_init(struct inet_bind2_bucket *tb, tb->l3mdev = l3mdev; tb->port = port;#if IS_ENABLED (CONFIG_IPV6 ) + tb->family = sk->sk_family;if (sk->sk_family ==AF_INET6 ) tb->v6_rcv_saddr = sk->sk_v6_rcv_saddr;else @@ -146,6 +147,9 @@ 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);@@ -791,6 +795,9 @@ 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) + return false; +if (sk->sk_family ==AF_INET6 )return net_eq (ib2_net (tb), net) && tb->port == port && tb->l3mdev == l3mdev &&@@ -807,6 +814,9 @@ bool inet_bind2_bucket_match_addr_any(const struct inet_bind2_bucket *tb, const#if IS_ENABLED (CONFIG_IPV6 )struct in6_addr addr_any = {}; +if (sk->sk_family != tb->family) +return false; +if (sk->sk_family ==AF_INET6 )return net_eq (ib2_net (tb), net) && tb->port == port && tb->l3mdev == l3mdev && --2.30.2
Quoted 4 lines · Eric Dumazet
Quoted 3 lines · Martin KaFai Lau
From: Martin KaFai Lau <[email protected]>
Quoted 8 lines
Quoted 6 lines
The
v6_rcv_saddrandrcv_saddrare inside a union in the 'structinet_bind2_bucket'. When searching a bucket by following the bhash2 hashtable chain, eg.inet_bind2_bucket_match, it is only using thesk->sk_familyand there is no way to check if theinet_bind2_buckethas a v6 or v4 address in the union. This leads to an uninit-value KMSAN report in [0] and also potentially incorrect matches.I do not see the KMSAN report, is it missing from this changelog ?
My bad. Forgot to paste the link in the commit message. It is here:
https://lore.kernel.org/netdev/CAG_fn=Ud3zSW7AZWXc+asfMhZVL5ETnvuY44Pmyv4NPv-ijN-A@mail.gmail.com/
Eric Dumazet
Reviewed-byQuoted 13 lines · Martin KaFai Lau
Quoted 11 lines · Eric Dumazet
Quoted 9 lines · Martin KaFai Lau
From: Martin KaFai Lau <[email protected]>
The
v6_rcv_saddrandrcv_saddrare inside a union in the 'structinet_bind2_bucket'. When searching a bucket by following the bhash2 hashtable chain, eg.inet_bind2_bucket_match, it is only using thesk->sk_familyand there is no way to check if theinet_bind2_buckethas a v6 or v4 address in the union. This leads to an uninit-value KMSAN report in [0] and also potentially incorrect matches.
Quoted 5 lines
Quoted 1 line
I do not see the KMSAN report, is it missing from this changelog ?
My bad. Forgot to paste the link in the commit message. It is here:
https://lore.kernel.org/netdev/CAG_fn=Ud3zSW7AZWXc+asfMhZVL5ETnvuY44Pmyv4NPv-ijN-A@mail.gmail.com/
I see, thanks.
Alexander Potapenko
Tested-byQuoted 17 lines · Eric Dumazet
Quoted 15 lines · Martin KaFai Lau
Quoted 12 lines · Eric Dumazet
Quoted 9 lines · Martin KaFai Lau
From: Martin KaFai Lau <[email protected]>
The
v6_rcv_saddrandrcv_saddrare inside a union in the 'structinet_bind2_bucket'. When searching a bucket by following the bhash2 hashtable chain, eg.inet_bind2_bucket_match, it is only using thesk->sk_familyand there is no way to check if theinet_bind2_buckethas a v6 or v4 address in the union. This leads to an uninit-value KMSAN report in [0] and also potentially incorrect matches.I do not see the KMSAN report, is it missing from this changelog ?
Quoted 7 lines
Quoted 3 lines
My bad. Forgot to paste the link in the commit message. It is here:
https://lore.kernel.org/netdev/CAG_fn=Ud3zSW7AZWXc+asfMhZVL5ETnvuY44Pmyv4NPv-ijN-A@mail.gmail.com/
I see, thanks.
Reviewed-by Eric Dumazet <[email protected]>
Thanks!
Signature · 10 lines
Alexander Potapenko Software Engineer Google Germany GmbH Erika-Mann-Straße, 33 80636 München Geschäftsführer: Paul Manicle, Liana Sebastian Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg
This patch was applied to netdev/net-next.git (master) by Jakub Kicinski <[email protected]>:
Hello:
This patch was applied to netdev/net-next.git (master) by Jakub Kicinski <[email protected]>:
Quoted 10 lines · On Mon, 26 Sep 2022 17:25:44 -0700 you wrote:
From: Martin KaFai Lau <[email protected]>
The
v6_rcv_saddrandrcv_saddrare inside a union in the 'structinet_bind2_bucket'. When searching a bucket by following the bhash2 hashtable chain, eg.inet_bind2_bucket_match, it is only using thesk->sk_familyand there is no way to check if theinet_bind2_buckethas a v6 or v4 address in the union. This leads to an uninit-value KMSAN report in [0] and also potentially incorrect matches.[...]
Here is the summary with links:
- [net-next] net: Fix incorrect address comparison when searching for a bind2 bucket
https://git.kernel.org/netdev/net-next/c/5456262d2baa
You are awesome, thank you!
Signature · 2 lines
Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
Quoted 9 lines · Martin KaFai Lau
From: Martin KaFai Lau <[email protected]>
The
v6_rcv_saddrandrcv_saddrare inside a union in the 'structinet_bind2_bucket'. When searching a bucket by following the bhash2 hashtable chain, eg.inet_bind2_bucket_match, it is only using thesk->sk_familyand there is no way to check if theinet_bind2_buckethas a v6 or v4 address in the union. This leads to an uninit-value KMSAN report in [0] and also potentially incorrect matches.
Quoted 3 lines
This patch fixes it by adding a family member to the
inet_bind2_bucketand then tests 'sk->sk_family!=tb->family' before matching the sk's address to the tb's address.
It seems this patch doesn't handle v4mapped addresses properly. One of gVisor test started failing with this change:
socket(AF_INET6, SOCK_STREAM, IPPROTO_IP) = 3
bind(3, {sa_family=AF_INET6, sin6_port=htons(0), sin6_flowinfo=htonl(0), inet_pton(AF_INET6, "::ffff:0.0.0.0", &sin6_addr), sin6_scope_id=0}, 28) = 0 getsockname(3, {sa_family=AF_INET6, sin6_port=htons(33789), sin6_flowinfo=htonl(0), inet_pton(AF_INET6, "::ffff:0.0.0.0", &sin6_addr), sin6_scope_id=0}, [28]) = 0 socket(AF_INET6, SOCK_STREAM, IPPROTO_IP) = 4
bind(4, {sa_family=AF_INET6, sin6_port=htons(33789), sin6_flowinfo=htonl(0), inet_pton(AF_INET6, "::1", &sin6_addr), sin6_scope_id=0}, 28) = 0 socket(AF_INET, SOCK_STREAM, IPPROTO_IP) = 5bind(5, {sa_family=AF_INET, sin_port=htons(33789), sin_addr=inet_addr("127.0.0.1")}, 16) = 0
The test expects that the second bind returns EADDRINUSE.
Thanks,
Andrei
Quoted 69 lines
Cc Joanne Koong <[email protected]>Cc Alexander Potapenko <[email protected]>Fixes 28044fc1d495 ("net: Add a bhash2 table hashed by port and address")Signed-off-by Martin KaFai Lau <[email protected]>--- include/net/inet_hashtables.h | 3 +++ net/ipv4/inet_hashtables.c | 10 ++++++++++ 2 files changed, 13 insertions(+)include/net/inet_hashtables.h +3
-0@@ -95,6 +95,9 @@ struct inet_bind2_bucket {possible_net_t ib_net;int l3mdev;unsigned short port; +#if IS_ENABLED (CONFIG_IPV6 ) +unsigned short family; +#endif union {#if IS_ENABLED (CONFIG_IPV6 )struct in6_addr v6_rcv_saddr;net/ipv4/inet_hashtables.c +10
-0@@ -109,6 +109,7 @@ static void inet_bind2_bucket_init(struct inet_bind2_bucket *tb, tb->l3mdev = l3mdev; tb->port = port;#if IS_ENABLED (CONFIG_IPV6 ) + tb->family = sk->sk_family;if (sk->sk_family ==AF_INET6 ) tb->v6_rcv_saddr = sk->sk_v6_rcv_saddr;else @@ -146,6 +147,9 @@ 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);@@ -791,6 +795,9 @@ 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) + return false; +if (sk->sk_family ==AF_INET6 )return net_eq (ib2_net (tb), net) && tb->port == port && tb->l3mdev == l3mdev &&@@ -807,6 +814,9 @@ bool inet_bind2_bucket_match_addr_any(const struct inet_bind2_bucket *tb, const#if IS_ENABLED (CONFIG_IPV6 )struct in6_addr addr_any = {}; +if (sk->sk_family != tb->family) +return false; +if (sk->sk_family ==AF_INET6 )return net_eq (ib2_net (tb), net) && tb->port == port && tb->l3mdev == l3mdev &&Signature · 1 line
2.30.2
From: Andrei Vagin <[email protected]>
Date: Fri, 8 Sep 2023 14:54:12 -0700
Quoted 25 lines · Martin KaFai Lau
Quoted 9 lines
From: Martin KaFai Lau <[email protected]>
The
v6_rcv_saddrandrcv_saddrare inside a union in the 'structinet_bind2_bucket'. When searching a bucket by following the bhash2 hashtable chain, eg.inet_bind2_bucket_match, it is only using thesk->sk_familyand there is no way to check if theinet_bind2_buckethas a v6 or v4 address in the union. This leads to an uninit-value KMSAN report in [0] and also potentially incorrect matches.Quoted 3 lines
This patch fixes it by adding a family member to the
inet_bind2_bucketand then tests 'sk->sk_family!=tb->family' before matching the sk's address to the tb's address.It seems this patch doesn't handle v4mapped addresses properly. One of gVisor test started failing with this change:
socket(AF_INET6, SOCK_STREAM, IPPROTO_IP)= 3
bind(3, {sa_family=AF_INET6, sin6_port=htons(0), sin6_flowinfo=htonl(0),inet_pton(AF_INET6, "::ffff:0.0.0.0", &sin6_addr), sin6_scope_id=0}, 28) = 0 getsockname(3, {sa_family=AF_INET6, sin6_port=htons(33789), sin6_flowinfo=htonl(0),inet_pton(AF_INET6, "::ffff:0.0.0.0", &sin6_addr), sin6_scope_id=0}, [28]) = 0socket(AF_INET6, SOCK_STREAM, IPPROTO_IP)= 4
bind(4, {sa_family=AF_INET6, sin6_port=htons(33789), sin6_flowinfo=htonl(0),inet_pton(AF_INET6, "::1", &sin6_addr), sin6_scope_id=0}, 28) = 0socket(AF_INET, SOCK_STREAM, IPPROTO_IP)= 5bind(5, {sa_family=AF_INET, sin_port=htons(33789), sin_addr=inet_addr("127.0.0.1")}, 16)= 0
Quoted 1 line
The test expects that the second bind returns EADDRINUSE.
Thanks for the report.
inet_bind2_bucket_match_addr_any() forgot to take care of IPV6_ADDR_MAPPED inaddr_any case.
This change fixes the regression. I'll post a patch after checking other two functions that the commit touched.
---8<---
net/ipv4/inet_hashtables.c +3 -1
@@ -837,7 +837,9 @@ bool inet_bind2_bucket_match_addr_any(const struct inet_bind2_bucket *tb, constif (sk->sk_family ==AF_INET )return net_eq (ib2_net (tb), net) && tb->port == port && tb->l3mdev == l3mdev && -ipv6_addr_any (&tb->v6_rcv_saddr); + (ipv6_addr_any (&tb->v6_rcv_saddr) || + (ipv6_addr_type (&tb->v6_rcv_saddr) ==IPV6_ADDR_MAPPED && + tb->v6_rcv_saddr.s6_addr32[3 ] ==0 ));return false; }
---8<---
---8<---
[root@localhost ~]# python3
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
from socket import *
s1 =
socket(AF_INET6, SOCK_STREAM)s1.bind(('::ffff:0.0.0.0', 0))
port = s1.getsockname()[1]s2 =
socket(AF_INET, SOCK_STREAM)s2.bind(('127.0.0.1', port))
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
OSError: [Errno 98] Address already in use
---8<---