* [PATCH net] ping: fix the dif and sdif check in ping_lookup
@ 2022-02-16 5:20 Xin Long
2022-02-17 15:10 ` patchwork-bot+netdevbpf
2022-02-17 15:57 ` Jakub Kicinski
0 siblings, 2 replies; 5+ messages in thread
From: Xin Long @ 2022-02-16 5:20 UTC (permalink / raw)
To: network dev; +Cc: davem, kuba, Vasiliy Kulikov
When 'ping' changes to use PING socket instead of RAW socket by:
# sysctl -w net.ipv4.ping_group_range="0 100"
There is another regression caused when matching sk_bound_dev_if
and dif, RAW socket is using inet_iif() while PING socket lookup
is using skb->dev->ifindex, the cmd below fails due to this:
# ip link add dummy0 type dummy
# ip link set dummy0 up
# ip addr add 192.168.111.1/24 dev dummy0
# ping -I dummy0 192.168.111.1 -c1
The issue was also reported on:
https://github.com/iputils/iputils/issues/104
But fixed in iputils in a wrong way by not binding to device when
destination IP is on device, and it will cause some of kselftests
to fail, as Jianlin noticed.
This patch is to use inet(6)_iif and inet(6)_sdif to get dif and
sdif for PING socket, and keep consistent with RAW socket.
Fixes: c319b4d76b9e ("net: ipv4: add IPPROTO_ICMP socket kind")
Reported-by: Jianlin Shi <jishi@redhat.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
net/ipv4/ping.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index bcf7bc71cb56..3a5994b50571 100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -172,16 +172,23 @@ static struct sock *ping_lookup(struct net *net, struct sk_buff *skb, u16 ident)
struct sock *sk = NULL;
struct inet_sock *isk;
struct hlist_nulls_node *hnode;
- int dif = skb->dev->ifindex;
+ int dif, sdif;
if (skb->protocol == htons(ETH_P_IP)) {
+ dif = inet_iif(skb);
+ sdif = inet_sdif(skb);
pr_debug("try to find: num = %d, daddr = %pI4, dif = %d\n",
(int)ident, &ip_hdr(skb)->daddr, dif);
#if IS_ENABLED(CONFIG_IPV6)
} else if (skb->protocol == htons(ETH_P_IPV6)) {
+ dif = inet6_iif(skb);
+ sdif = inet6_sdif(skb);
pr_debug("try to find: num = %d, daddr = %pI6c, dif = %d\n",
(int)ident, &ipv6_hdr(skb)->daddr, dif);
#endif
+ } else {
+ pr_err("ping: protocol(%x) is not supported\n", ntohs(skb->protocol));
+ return NULL;
}
read_lock_bh(&ping_table.lock);
@@ -221,7 +228,7 @@ static struct sock *ping_lookup(struct net *net, struct sk_buff *skb, u16 ident)
}
if (sk->sk_bound_dev_if && sk->sk_bound_dev_if != dif &&
- sk->sk_bound_dev_if != inet_sdif(skb))
+ sk->sk_bound_dev_if != sdif)
continue;
sock_hold(sk);
--
2.31.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH net] ping: fix the dif and sdif check in ping_lookup
2022-02-16 5:20 [PATCH net] ping: fix the dif and sdif check in ping_lookup Xin Long
@ 2022-02-17 15:10 ` patchwork-bot+netdevbpf
2022-02-17 15:57 ` Jakub Kicinski
1 sibling, 0 replies; 5+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-02-17 15:10 UTC (permalink / raw)
To: Xin Long; +Cc: netdev, davem, kuba, segoon
Hello:
This patch was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:
On Wed, 16 Feb 2022 00:20:52 -0500 you wrote:
> When 'ping' changes to use PING socket instead of RAW socket by:
>
> # sysctl -w net.ipv4.ping_group_range="0 100"
>
> There is another regression caused when matching sk_bound_dev_if
> and dif, RAW socket is using inet_iif() while PING socket lookup
> is using skb->dev->ifindex, the cmd below fails due to this:
>
> [...]
Here is the summary with links:
- [net] ping: fix the dif and sdif check in ping_lookup
https://git.kernel.org/netdev/net/c/35a79e64de29
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net] ping: fix the dif and sdif check in ping_lookup
2022-02-16 5:20 [PATCH net] ping: fix the dif and sdif check in ping_lookup Xin Long
2022-02-17 15:10 ` patchwork-bot+netdevbpf
@ 2022-02-17 15:57 ` Jakub Kicinski
2022-02-23 6:08 ` Xin Long
1 sibling, 1 reply; 5+ messages in thread
From: Jakub Kicinski @ 2022-02-17 15:57 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, davem, Vasiliy Kulikov
On Wed, 16 Feb 2022 00:20:52 -0500 Xin Long wrote:
> if (skb->protocol == htons(ETH_P_IP)) {
> + dif = inet_iif(skb);
> + sdif = inet_sdif(skb);
> pr_debug("try to find: num = %d, daddr = %pI4, dif = %d\n",
> (int)ident, &ip_hdr(skb)->daddr, dif);
> #if IS_ENABLED(CONFIG_IPV6)
> } else if (skb->protocol == htons(ETH_P_IPV6)) {
> + dif = inet6_iif(skb);
> + sdif = inet6_sdif(skb);
> pr_debug("try to find: num = %d, daddr = %pI6c, dif = %d\n",
> (int)ident, &ipv6_hdr(skb)->daddr, dif);
> #endif
> + } else {
> + pr_err("ping: protocol(%x) is not supported\n", ntohs(skb->protocol));
> + return NULL;
> }
Are you sure this is not reachable from some networking path allowing
attacker (or local user) to DoS the kernel logs?
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net] ping: fix the dif and sdif check in ping_lookup
2022-02-17 15:57 ` Jakub Kicinski
@ 2022-02-23 6:08 ` Xin Long
2022-02-23 15:52 ` Jakub Kicinski
0 siblings, 1 reply; 5+ messages in thread
From: Xin Long @ 2022-02-23 6:08 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: network dev, davem, Vasiliy Kulikov
On Thu, Feb 17, 2022 at 11:57 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 16 Feb 2022 00:20:52 -0500 Xin Long wrote:
> > if (skb->protocol == htons(ETH_P_IP)) {
> > + dif = inet_iif(skb);
> > + sdif = inet_sdif(skb);
> > pr_debug("try to find: num = %d, daddr = %pI4, dif = %d\n",
> > (int)ident, &ip_hdr(skb)->daddr, dif);
> > #if IS_ENABLED(CONFIG_IPV6)
> > } else if (skb->protocol == htons(ETH_P_IPV6)) {
> > + dif = inet6_iif(skb);
> > + sdif = inet6_sdif(skb);
> > pr_debug("try to find: num = %d, daddr = %pI6c, dif = %d\n",
> > (int)ident, &ipv6_hdr(skb)->daddr, dif);
> > #endif
> > + } else {
> > + pr_err("ping: protocol(%x) is not supported\n", ntohs(skb->protocol));
> > + return NULL;
> > }
>
> Are you sure this is not reachable from some networking path allowing
> attacker (or local user) to DoS the kernel logs?
Hi, Jakub, sorry for late.
I actually didn't see how a skb with protocol != IP/IPV6 could reach here.
ping_err() is using "BUG()" for this kind of case.
I added this 'else' branch mostly to avoid the possible compiling
warning for "Use of uninitialized value dif/sdif".
Thanks.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net] ping: fix the dif and sdif check in ping_lookup
2022-02-23 6:08 ` Xin Long
@ 2022-02-23 15:52 ` Jakub Kicinski
0 siblings, 0 replies; 5+ messages in thread
From: Jakub Kicinski @ 2022-02-23 15:52 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, davem, Vasiliy Kulikov
On Wed, 23 Feb 2022 14:08:06 +0800 Xin Long wrote:
> On Thu, Feb 17, 2022 at 11:57 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > Are you sure this is not reachable from some networking path allowing
> > attacker (or local user) to DoS the kernel logs?
> Hi, Jakub, sorry for late.
>
> I actually didn't see how a skb with protocol != IP/IPV6 could reach here.
> ping_err() is using "BUG()" for this kind of case.
> I added this 'else' branch mostly to avoid the possible compiling
> warning for "Use of uninitialized value dif/sdif".
Understood but it's best practice to avoid prints on the datapath.
We should either drop the print completely (given it's impossible)
or make it _once.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-02-23 15:52 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-16 5:20 [PATCH net] ping: fix the dif and sdif check in ping_lookup Xin Long
2022-02-17 15:10 ` patchwork-bot+netdevbpf
2022-02-17 15:57 ` Jakub Kicinski
2022-02-23 6:08 ` Xin Long
2022-02-23 15:52 ` Jakub Kicinski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).