Skip to contentSkip to conversation

ipv4: Fix accidental RTO_ONLINK flags passed to ip_route_output_key_hash()

PATCH Applied Guillaume Nault 2 participants 6 messages Jan 10, 2022 – Jan 12, 2022

Subsystems (5)
lore
Share & tools
lore
#0 lore raw

v2 cover ipv4: Fix accidental RTO_ONLINK flags passed to ip_route_output_key_hash()

The IPv4 stack generally uses the last bit of ->flowi4_tos as a flag indicating link scope for route lookups (RTO_ONLINK). Therefore, we have to be careful when copying a TOS value to ->flowi4_tos. In particular, the ->tos field of IPv4 packets may have this bit set because of ECN. Also tunnel keys generally accept any user value for the tos.

This series fixes several places where ->flowi4_tos was set from non-sanitised values and the flowi4 structure was later used by ip_route_output_key_hash().

Note that the IPv4 stack usually clears the RTO_ONLINK bit using RT_TOS(). However this macro is based on an obsolete interpretation of the old IPv4 TOS field (RFC 1349) and clears the three high order bits too. Since we don't need to clear these bits and since it doesn't make sense to clear only one of the ECN bits, this patch series uses INET_ECN_MASK instead.

All patches were compile tested only.

v2: Rebase on top of net.

Guillaume Nault (4):
xfrm: Don't accidentally set RTO_ONLINK in decode_session4()
gre: Don't accidentally set RTO_ONLINK in gre_fill_metadata_dst()
libcxgb: Don't accidentally set RTO_ONLINK in cxgb_find_route()
mlx5: Don't accidentally set RTO_ONLINK before

    mlx5e_route_lookup_ipv4_get()
 drivers/net/ethernet/chelsio/libcxgb/libcxgb_cm.c   | 3 ++-
 drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c | 5 +++--
 net/ipv4/ip_gre.c                                   | 5 +++--
 net/xfrm/xfrm_policy.c                              | 3 ++-
 4 files changed, 10 insertions(+), 6 deletions(-)
Signature · 1 line
2.21.3
#1 lore raw

PATCH v2 1/4 xfrm: Don't accidentally set RTO_ONLINK in decode_session4()

Similar to commit 94e2238969e8 ("xfrm4: strip ECN bits from tos field"), clear the ECN bits from iph->tos when setting ->flowi4_tos. This ensures that the last bit of ->flowi4_tos is cleared, so ip_route_output_key_hash() isn't going to restrict the scope of the route lookup.

Use ~INET_ECN_MASK instead of IPTOS_RT_MASK, because we have no reason to clear the high order bits.

Found by code inspection, compile tested only.

Fixes 4da3089f2b58 ("[IPSEC]: Use TOS when doing tunnel lookups")
---
 net/xfrm/xfrm_policy.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
net/xfrm/xfrm_policy.c +2 -1
index 84d2361da015..4924b9135c6e 100644
@@ -31,6 +31,7 @@
 #include <linux/if_tunnel.h>
 #include <net/dst.h>
 #include <net/flow.h>
+#include <net/inet_ecn.h>
 #include <net/xfrm.h>
 #include <net/ip.h>
 #include <net/gre.h>
@@ -3295,7 +3296,7 @@ decode_session4(struct sk_buff *skb, struct flowi *fl, bool reverse)
 	fl4->flowi4_proto = iph->protocol;
 	fl4->daddr = reverse ? iph->saddr : iph->daddr;
 	fl4->saddr = reverse ? iph->daddr : iph->saddr;
-	fl4->flowi4_tos = iph->tos;
+	fl4->flowi4_tos = iph->tos & ~INET_ECN_MASK;
 
 	if (!ip_is_fragment(iph)) {
 		switch (iph->protocol) {
Signature · 1 line
2.21.3
#2 lore raw

PATCH v2 2/4 gre: Don't accidentally set RTO_ONLINK in gre_fill_metadata_dst()

In reply to Guillaume Nault

Mask the ECN bits before initialising ->flowi4_tos. The tunnel key may have the last ECN bit set, which will interfere with the route lookup process as ip_route_output_key_hash() interpretes this bit specially (to restrict the route scope).

Found by code inspection, compile tested only.

Fixes 962924fa2b7a ("ip_gre: Refactor collect metatdata mode tunnel xmit to ip_md_tunnel_xmit")
---
 net/ipv4/ip_gre.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
net/ipv4/ip_gre.c +3 -2
index 2ac2b95c5694..99db2e41ed10 100644
@@ -604,8 +604,9 @@ static int gre_fill_metadata_dst(struct net_device *dev, struct sk_buff *skb)
 
 	key = &info->key;
 	ip_tunnel_init_flow(&fl4, IPPROTO_GRE, key->u.ipv4.dst, key->u.ipv4.src,
-			    tunnel_id_to_key32(key->tun_id), key->tos, 0,
-			    skb->mark, skb_get_hash(skb));
+			    tunnel_id_to_key32(key->tun_id),
+			    key->tos & ~INET_ECN_MASK, 0, skb->mark,
+			    skb_get_hash(skb));
 	rt = ip_route_output_key(dev_net(dev), &fl4);
 	if (IS_ERR(rt))
 		return PTR_ERR(rt);
Signature · 1 line
2.21.3
#3 lore raw

PATCH v2 3/4 libcxgb: Don't accidentally set RTO_ONLINK in cxgb_find_route()

In reply to Guillaume Nault

Mask the ECN bits before calling ip_route_output_ports(). The tos variable might be passed directly from an IPv4 header, so it may have the last ECN bit set. This interferes with the route lookup process as ip_route_output_key_hash() interpretes this bit specially (to restrict the route scope).

Found by code inspection, compile tested only.

Fixes 804c2f3e36ef ("libcxgb,iw_cxgb4,cxgbit: add cxgb_find_route()")
---
 drivers/net/ethernet/chelsio/libcxgb/libcxgb_cm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/net/ethernet/chelsio/libcxgb/libcxgb_cm.c +2 -1
index d04a6c163445..da8d10475a08 100644
@@ -32,6 +32,7 @@
 
 #include <linux/tcp.h>
 #include <linux/ipv6.h>
+#include <net/inet_ecn.h>
 #include <net/route.h>
 #include <net/ip6_route.h>
 
@@ -99,7 +100,7 @@ cxgb_find_route(struct cxgb4_lld_info *lldi,
 
 	rt = ip_route_output_ports(&init_net, &fl4, NULL, peer_ip, local_ip,
 				   peer_port, local_port, IPPROTO_TCP,
-				   tos, 0);
+				   tos & ~INET_ECN_MASK, 0);
 	if (IS_ERR(rt))
 		return NULL;
 	n = dst_neigh_lookup(&rt->dst, &peer_ip);
Signature · 1 line
2.21.3
#4 lore raw

PATCH v2 4/4 mlx5: Don't accidentally set RTO_ONLINK before mlx5e_route_lookup_ipv4_get()

In reply to Guillaume Nault

Mask the ECN bits before calling mlx5e_route_lookup_ipv4_get(). The tunnel key might have the last ECN bit set. This interferes with the route lookup process as ip_route_output_key_hash() interpretes this bit specially (to restrict the route scope).

Found by code inspection, compile tested only.

Fixes c7b9038d8af6 ("net/mlx5e: TC preparation refactoring for routing update event")
Fixes 9a941117fb76 ("net/mlx5e: Maximize ip tunnel key usage on the TC offloading path")
---
 drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c +3 -2
index a5e450973225..bc5f1dcb75e1 100644
@@ -1,6 +1,7 @@
 /* SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB */
 /* Copyright (c) 2018 Mellanox Technologies. */
 
+#include <net/inet_ecn.h>
 #include <net/vxlan.h>
 #include <net/gre.h>
 #include <net/geneve.h>
@@ -235,7 +236,7 @@ int mlx5e_tc_tun_create_header_ipv4(struct mlx5e_priv *priv,
 	int err;
 
 	/* add the IP fields */
-	attr.fl.fl4.flowi4_tos = tun_key->tos;
+	attr.fl.fl4.flowi4_tos = tun_key->tos & ~INET_ECN_MASK;
 	attr.fl.fl4.daddr = tun_key->u.ipv4.dst;
 	attr.fl.fl4.saddr = tun_key->u.ipv4.src;
 	attr.ttl = tun_key->ttl;
@@ -350,7 +351,7 @@ int mlx5e_tc_tun_update_header_ipv4(struct mlx5e_priv *priv,
 	int err;
 
 	/* add the IP fields */
-	attr.fl.fl4.flowi4_tos = tun_key->tos;
+	attr.fl.fl4.flowi4_tos = tun_key->tos & ~INET_ECN_MASK;
 	attr.fl.fl4.daddr = tun_key->u.ipv4.dst;
 	attr.fl.fl4.saddr = tun_key->u.ipv4.src;
 	attr.ttl = tun_key->ttl;
Signature · 1 line
2.21.3
#5 lore raw
In reply to Guillaume Nault
This series was applied to netdev/net.git (master) by Jakub Kicinski <[email protected]>:

Hello:

This series was applied to netdev/net.git (master) by Jakub Kicinski <[email protected]>:

Quoted 8 lines · On Mon, 10 Jan 2022 14:43:04 +0100 you wrote:

The IPv4 stack generally uses the last bit of ->flowi4_tos as a flag indicating link scope for route lookups (RTO_ONLINK). Therefore, we have to be careful when copying a TOS value to ->flowi4_tos. In particular, the ->tos field of IPv4 packets may have this bit set because of ECN. Also tunnel keys generally accept any user value for the tos.

[...]

Here is the summary with links:
- [v2,net,1/4] xfrm: Don't accidentally set RTO_ONLINK in decode_session4()

    https://git.kernel.org/netdev/net/c/23e7b1bfed61
  - [v2,net,2/4] gre: Don't accidentally set RTO_ONLINK in gre_fill_metadata_dst()
    https://git.kernel.org/netdev/net/c/f7716b318568
  - [v2,net,3/4] libcxgb: Don't accidentally set RTO_ONLINK in cxgb_find_route()
    https://git.kernel.org/netdev/net/c/a915deaa9abe
  - [v2,net,4/4] mlx5: Don't accidentally set RTO_ONLINK before mlx5e_route_lookup_ipv4_get()
    https://git.kernel.org/netdev/net/c/48d67543e01d

You are awesome, thank you!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html