| Message ID | 20220617020213.1881452-1-cmllamas@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] ipv4: ping: fix bind address validity check | expand |
On Thu, Jun 16, 2022 at 7:02 PM Carlos Llamas <cmllamas@google.com> wrote: > > Commit 8ff978b8b222 ("ipv4/raw: support binding to nonlocal addresses") > introduced a helper function to fold duplicated validity checks of bind > addresses into inet_addr_valid_or_nonlocal(). However, this caused an > unintended regression in ping_check_bind_addr(), which previously would > reject binding to multicast and broadcast addresses, but now these are > both incorrectly allowed as reported in [1]. > > This patch restores the original check. A simple reordering is done to > improve readability and make it evident that multicast and broadcast > addresses should not be allowed. Also, add an early exit for INADDR_ANY > which replaces lost behavior added by commit 0ce779a9f501 ("net: Avoid > unnecessary inet_addr_type() call when addr is INADDR_ANY"). > > [1] https://lore.kernel.org/netdev/CANP3RGdkAcDyAZoT1h8Gtuu0saq+eOrrTiWbxnOs+5zn+cpyKg@mail.gmail.com/ > > Fixes: 8ff978b8b222 ("ipv4/raw: support binding to nonlocal addresses") > Cc: Miaohe Lin <linmiaohe@huawei.com> > Cc: Riccardo Paolo Bestetti <pbl@bestov.io> > Reported-by: Maciej Żenczykowski <maze@google.com> > Signed-off-by: Carlos Llamas <cmllamas@google.com> > --- > net/ipv4/ping.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c > index 1a43ca73f94d..3c6101def7d6 100644 > --- a/net/ipv4/ping.c > +++ b/net/ipv4/ping.c > @@ -319,12 +319,16 @@ static int ping_check_bind_addr(struct sock *sk, struct inet_sock *isk, > pr_debug("ping_check_bind_addr(sk=%p,addr=%pI4,port=%d)\n", > sk, &addr->sin_addr.s_addr, ntohs(addr->sin_port)); > > + if (addr->sin_addr.s_addr == htonl(INADDR_ANY)) > + return 0; > + > tb_id = l3mdev_fib_table_by_index(net, sk->sk_bound_dev_if) ? : tb_id; > chk_addr_ret = inet_addr_type_table(net, addr->sin_addr.s_addr, tb_id); > > - if (!inet_addr_valid_or_nonlocal(net, inet_sk(sk), > - addr->sin_addr.s_addr, > - chk_addr_ret)) > + if (chk_addr_ret == RTN_MULTICAST || > + chk_addr_ret == RTN_BROADCAST || > + (chk_addr_ret != RTN_LOCAL && > + !inet_can_nonlocal_bind(net, isk))) > return -EADDRNOTAVAIL; > > #if IS_ENABLED(CONFIG_IPV6) > -- > 2.36.1.476.g0c4daa206d-goog Reviewed-by: Maciej Żenczykowski <maze@google.com>
On Fri Jun 17, 2022 at 4:02 AM CEST, Carlos Llamas wrote: > Commit 8ff978b8b222 ("ipv4/raw: support binding to nonlocal addresses") > introduced a helper function to fold duplicated validity checks of bind > addresses into inet_addr_valid_or_nonlocal(). However, this caused an > unintended regression in ping_check_bind_addr(), which previously would > reject binding to multicast and broadcast addresses, but now these are > both incorrectly allowed as reported in [1]. > > This patch restores the original check. A simple reordering is done to > improve readability and make it evident that multicast and broadcast > addresses should not be allowed. Also, add an early exit for INADDR_ANY > which replaces lost behavior added by commit 0ce779a9f501 ("net: Avoid > unnecessary inet_addr_type() call when addr is INADDR_ANY"). Like I mentioned in one of my follow-ups to [1], I think a proper patch for this not only restores the original behaviour, but also includes regression tests (if we had it we wouldn't be needing this patch in the first place; as one would expect, the regression escaped multiple human reviews :) I'm following up this email with a v2 to have that. Riccardo P. Bestetti > > [1] https://lore.kernel.org/netdev/CANP3RGdkAcDyAZoT1h8Gtuu0saq+eOrrTiWbxnOs+5zn+cpyKg@mail.gmail.com/ > > Fixes: 8ff978b8b222 ("ipv4/raw: support binding to nonlocal addresses") > Cc: Miaohe Lin <linmiaohe@huawei.com> > Cc: Riccardo Paolo Bestetti <pbl@bestov.io> > Reported-by: Maciej Żenczykowski <maze@google.com> > Signed-off-by: Carlos Llamas <cmllamas@google.com> > --- > net/ipv4/ping.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c > index 1a43ca73f94d..3c6101def7d6 100644 > --- a/net/ipv4/ping.c > +++ b/net/ipv4/ping.c > @@ -319,12 +319,16 @@ static int ping_check_bind_addr(struct sock *sk, struct inet_sock *isk, > pr_debug("ping_check_bind_addr(sk=%p,addr=%pI4,port=%d)\n", > sk, &addr->sin_addr.s_addr, ntohs(addr->sin_port)); > > + if (addr->sin_addr.s_addr == htonl(INADDR_ANY)) > + return 0; > + > tb_id = l3mdev_fib_table_by_index(net, sk->sk_bound_dev_if) ? : tb_id; > chk_addr_ret = inet_addr_type_table(net, addr->sin_addr.s_addr, tb_id); > > - if (!inet_addr_valid_or_nonlocal(net, inet_sk(sk), > - addr->sin_addr.s_addr, > - chk_addr_ret)) > + if (chk_addr_ret == RTN_MULTICAST || > + chk_addr_ret == RTN_BROADCAST || > + (chk_addr_ret != RTN_LOCAL && > + !inet_can_nonlocal_bind(net, isk))) > return -EADDRNOTAVAIL; > > #if IS_ENABLED(CONFIG_IPV6) > -- > 2.36.1.476.g0c4daa206d-goog
diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c index 1a43ca73f94d..3c6101def7d6 100644 --- a/net/ipv4/ping.c +++ b/net/ipv4/ping.c @@ -319,12 +319,16 @@ static int ping_check_bind_addr(struct sock *sk, struct inet_sock *isk, pr_debug("ping_check_bind_addr(sk=%p,addr=%pI4,port=%d)\n", sk, &addr->sin_addr.s_addr, ntohs(addr->sin_port)); + if (addr->sin_addr.s_addr == htonl(INADDR_ANY)) + return 0; + tb_id = l3mdev_fib_table_by_index(net, sk->sk_bound_dev_if) ? : tb_id; chk_addr_ret = inet_addr_type_table(net, addr->sin_addr.s_addr, tb_id); - if (!inet_addr_valid_or_nonlocal(net, inet_sk(sk), - addr->sin_addr.s_addr, - chk_addr_ret)) + if (chk_addr_ret == RTN_MULTICAST || + chk_addr_ret == RTN_BROADCAST || + (chk_addr_ret != RTN_LOCAL && + !inet_can_nonlocal_bind(net, isk))) return -EADDRNOTAVAIL; #if IS_ENABLED(CONFIG_IPV6)
Commit 8ff978b8b222 ("ipv4/raw: support binding to nonlocal addresses") introduced a helper function to fold duplicated validity checks of bind addresses into inet_addr_valid_or_nonlocal(). However, this caused an unintended regression in ping_check_bind_addr(), which previously would reject binding to multicast and broadcast addresses, but now these are both incorrectly allowed as reported in [1]. This patch restores the original check. A simple reordering is done to improve readability and make it evident that multicast and broadcast addresses should not be allowed. Also, add an early exit for INADDR_ANY which replaces lost behavior added by commit 0ce779a9f501 ("net: Avoid unnecessary inet_addr_type() call when addr is INADDR_ANY"). [1] https://lore.kernel.org/netdev/CANP3RGdkAcDyAZoT1h8Gtuu0saq+eOrrTiWbxnOs+5zn+cpyKg@mail.gmail.com/ Fixes: 8ff978b8b222 ("ipv4/raw: support binding to nonlocal addresses") Cc: Miaohe Lin <linmiaohe@huawei.com> Cc: Riccardo Paolo Bestetti <pbl@bestov.io> Reported-by: Maciej Żenczykowski <maze@google.com> Signed-off-by: Carlos Llamas <cmllamas@google.com> --- net/ipv4/ping.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)