[PATCH net] ipv4: ping: fix bind address validity check

[PATCH net] ipv4: ping: fix bind address validity check

[Carlos Llamas]

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

Re: [PATCH net] ipv4: ping: fix bind address validity check

[Maciej Żenczykowski]

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>

Re: [PATCH net] ipv4: ping: fix bind address validity check

[Riccardo Paolo Bestetti]

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