Skip to contentSkip to conversation

net: make sure skb_dst is valid before using

PATCH In review Roman Kapl 5 participants 9 messages Jan 23, 2018 – Jan 25, 2018

Subsystems (7)
Networking Network drivers InfiniBand s390 +3 more subsystems
lore
Share & tools
lore

Roman Kapl

author
#0 lore raw

PATCH 1/1 net: make sure skb_dst is valid before using

Tunnel devices often use skb_dst(skb)->ops, but ops are not implemented for metadata tunnel destinations. Use skb_valid_dst to check if skb_dst is a real (non-metadata) destination.

Such packets can easily be crafted using tc tunnel_key actions or BPF and will crash the tunnels, as observed at least with sit, geneve and vxlan. Some of these tunnels are also intended to be used with metadata destinations, so this represents a loss of functionality.

There might be more places with similar patterns, but sometimes it is hard to tell if metadata dst packets can reach them, so this patch is not exhaustive.

Fixes 52a589d51f10 ("geneve: update skb dst pmtu on tx path")
Fixes a93bf0ff4490 ("vxlan: update skb dst pmtu on tx path")
Signed-off-by Roman Kapl <[email protected]>
---
 drivers/infiniband/ulp/ipoib/ipoib_cm.c | 3 ++-
 drivers/net/geneve.c                    | 5 +++--
 drivers/net/vxlan.c                     | 5 +++--
 drivers/s390/net/qeth_l3_main.c         | 7 +++++--
 include/net/ip6_tunnel.h                | 1 +
 net/ipv4/ip_tunnel.c                    | 2 +-
 net/ipv4/ip_vti.c                       | 3 ++-
 net/ipv6/sit.c                          | 7 ++++---
 8 files changed, 21 insertions(+), 12 deletions(-)
drivers/infiniband/ulp/ipoib/ipoib_cm.c +2 -1
index 2c13123bfd69..65eacafc898f 100644
@@ -40,6 +40,7 @@
 #include <linux/moduleparam.h>
 #include <linux/sched/signal.h>
 #include <linux/sched/mm.h>
+#include <net/dst_metadata.h>
 
 #include "ipoib.h"
 

@@ -1456,7 +1457,7 @@ void ipoib_cm_skb_too_long(struct net_device *dev, struct sk_buff *skb,
 	struct ipoib_dev_priv *priv = ipoib_priv(dev);
 	int e = skb_queue_empty(&priv->cm.skb_queue);
 
-	if (skb_dst(skb))
+	if (skb_valid_dst(skb))
 		skb_dst(skb)->ops->update_pmtu(skb_dst(skb), NULL, skb, mtu);
 
 	skb_queue_tail(&priv->cm.skb_queue, skb);
drivers/net/geneve.c +3 -2
index 195e0d0add8d..2e5d2bc8d851 100644
@@ -19,6 +19,7 @@
 #include <net/rtnetlink.h>
 #include <net/geneve.h>
 #include <net/protocol.h>
+#include <net/dst_metadata.h>
 
 #define GENEVE_NETDEV_VER	"0.6"
 

@@ -825,7 +826,7 @@ static int geneve_xmit_skb(struct sk_buff *skb, struct net_device *dev,
 	if (IS_ERR(rt))
 		return PTR_ERR(rt);
 
-	if (skb_dst(skb)) {
+	if (skb_valid_dst(skb)) {
 		int mtu = dst_mtu(&rt->dst) - sizeof(struct iphdr) -
 			  GENEVE_BASE_HLEN - info->options_len - 14;
 

@@ -871,7 +872,7 @@ static int geneve6_xmit_skb(struct sk_buff *skb, struct net_device *dev,
 	if (IS_ERR(dst))
 		return PTR_ERR(dst);
 
-	if (skb_dst(skb)) {
+	if (skb_valid_dst(skb)) {
 		int mtu = dst_mtu(dst) - sizeof(struct ipv6hdr) -
 			  GENEVE_BASE_HLEN - info->options_len - 14;
drivers/net/vxlan.c +3 -2
index 82090ae7ced1..b0e96746bc2e 100644
@@ -28,6 +28,7 @@
 #include <net/netns/generic.h>
 #include <net/tun_proto.h>
 #include <net/vxlan.h>
+#include <net/dst_metadata.h>
 
 #if IS_ENABLED(CONFIG_IPV6)
 #include <net/ip6_tunnel.h>

@@ -2155,7 +2156,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 		}
 
 		ndst = &rt->dst;
-		if (skb_dst(skb)) {
+		if (skb_valid_dst(skb)) {
 			int mtu = dst_mtu(ndst) - VXLAN_HEADROOM;
 
 			skb_dst(skb)->ops->update_pmtu(skb_dst(skb), NULL,

@@ -2197,7 +2198,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 				goto out_unlock;
 		}
 
-		if (skb_dst(skb)) {
+		if (skb_valid_dst(skb)) {
 			int mtu = dst_mtu(ndst) - VXLAN6_HEADROOM;
 
 			skb_dst(skb)->ops->update_pmtu(skb_dst(skb), NULL,
drivers/s390/net/qeth_l3_main.c +5 -2
index b0c888e86cd4..694efab9cccd 100644
@@ -35,6 +35,7 @@
 #include <net/ip6_fib.h>
 #include <net/ip6_checksum.h>
 #include <net/iucv/af_iucv.h>
+#include <net/dst_metadata.h>
 #include <linux/hashtable.h>
 
 #include "qeth_l3.h"

@@ -2259,9 +2260,11 @@ static int qeth_l3_get_cast_type(struct sk_buff *skb)
 	struct dst_entry *dst;
 
 	rcu_read_lock();
-	dst = skb_dst(skb);
-	if (dst)
+	if (skb_valid_dst(skb)) {
+		dst = skb_dst(skb);
 		n = dst_neigh_lookup_skb(dst, skb);
+	}
+
 	if (n) {
 		int cast_type = n->type;
include/net/ip6_tunnel.h +1 -0
index 236e40ba06bf..1a704dfe5e1a 100644
@@ -148,6 +148,7 @@ struct net *ip6_tnl_get_link_net(const struct net_device *dev);
 int ip6_tnl_get_iflink(const struct net_device *dev);
 int ip6_tnl_change_mtu(struct net_device *dev, int new_mtu);
 
+/* Must be called with valid skb_dst */
 static inline void ip6tunnel_xmit(struct sock *sk, struct sk_buff *skb,
 				  struct net_device *dev)
 {
net/ipv4/ip_tunnel.c +1 -1
index 5ddb1cb52bd4..9256e8b9c538 100644
@@ -520,7 +520,7 @@ static int tnl_update_pmtu(struct net_device *dev, struct sk_buff *skb,
 	else
 		mtu = skb_dst(skb) ? dst_mtu(skb_dst(skb)) : dev->mtu;
 
-	if (skb_dst(skb))
+	if (skb_valid_dst(skb))
 		skb_dst(skb)->ops->update_pmtu(skb_dst(skb), NULL, skb, mtu);
 
 	if (skb->protocol == htons(ETH_P_IP)) {
net/ipv4/ip_vti.c +2 -1
index 949f432a5f04..fdb20a95b819 100644
@@ -43,6 +43,7 @@
 #include <net/xfrm.h>
 #include <net/net_namespace.h>
 #include <net/netns/generic.h>
+#include <net/dst_metadata.h>
 
 static struct rtnl_link_ops vti_link_ops __read_mostly;
 

@@ -172,7 +173,7 @@ static netdev_tx_t vti_xmit(struct sk_buff *skb, struct net_device *dev,
 	int err;
 	int mtu;
 
-	if (!dst) {
+	if (!skb_valid_dst(skb)) {
 		dev->stats.tx_carrier_errors++;
 		goto tx_error_icmp;
 	}
net/ipv6/sit.c +4 -3
index d7dc23c1b2ca..50a5be98462f 100644
@@ -55,6 +55,7 @@
 #include <net/dsfield.h>
 #include <net/net_namespace.h>
 #include <net/netns/generic.h>
+#include <net/dst_metadata.h>
 
 /*
    This version of net/ipv6/sit.c is cloned of net/ipv4/ip_gre.c

@@ -837,7 +838,7 @@ static netdev_tx_t ipip6_tunnel_xmit(struct sk_buff *skb,
 		struct neighbour *neigh = NULL;
 		bool do_tx_error = false;
 
-		if (skb_dst(skb))
+		if (skb_valid_dst(skb))
 			neigh = dst_neigh_lookup(skb_dst(skb), &iph6->daddr);
 
 		if (!neigh) {

@@ -866,7 +867,7 @@ static netdev_tx_t ipip6_tunnel_xmit(struct sk_buff *skb,
 		struct neighbour *neigh = NULL;
 		bool do_tx_error = false;
 
-		if (skb_dst(skb))
+		if (skb_valid_dst(skb))
 			neigh = dst_neigh_lookup(skb_dst(skb), &iph6->daddr);
 
 		if (!neigh) {

@@ -934,7 +935,7 @@ static netdev_tx_t ipip6_tunnel_xmit(struct sk_buff *skb,
 			df = 0;
 		}
 
-		if (tunnel->parms.iph.daddr && skb_dst(skb))
+		if (tunnel->parms.iph.daddr && skb_valid_dst(skb))
 			skb_dst(skb)->ops->update_pmtu(skb_dst(skb), NULL, skb, mtu);
 
 		if (skb->len > mtu && !skb_is_gso(skb)) {
Signature · 1 line
2.15.1
#1 lore raw
Quoted 4 lines · Roman Kapl

Tunnel devices often use skb_dst(skb)->ops, but ops are not implemented for metadata tunnel destinations. Use skb_valid_dst to check if skb_dst is a real (non-metadata) destination.

Quoted 8 lines

Such packets can easily be crafted using tc tunnel_key actions or BPF and will crash the tunnels, as observed at least with sit, geneve and vxlan. Some of these tunnels are also intended to be used with metadata destinations, so this represents a loss of functionality.

There might be more places with similar patterns, but sometimes it is hard to tell if metadata dst packets can reach them, so this patch is not exhaustive.

This patch is trying to fix a lot of places, and there may be more. But all because of the lack of .update_pmtu or .neigh_lookup. (dst_link_failure is safe, btw)

Not sure if it will be better to add .update_pmtu, .neigh_lookup and even .redirect for md_dst_ops, but just leave them empty ? like fake_dst_ops in br_nf.

Let's see other's opinions.

Quoted 210 lines
Fixes 52a589d51f10 ("geneve: update skb dst pmtu on tx path")
Fixes a93bf0ff4490 ("vxlan: update skb dst pmtu on tx path")
Signed-off-by Roman Kapl <[email protected]>
---
 drivers/infiniband/ulp/ipoib/ipoib_cm.c | 3 ++-
 drivers/net/geneve.c                    | 5 +++--
 drivers/net/vxlan.c                     | 5 +++--
 drivers/s390/net/qeth_l3_main.c         | 7 +++++--
 include/net/ip6_tunnel.h                | 1 +
 net/ipv4/ip_tunnel.c                    | 2 +-
 net/ipv4/ip_vti.c                       | 3 ++-
 net/ipv6/sit.c                          | 7 ++++---
 8 files changed, 21 insertions(+), 12 deletions(-)
drivers/infiniband/ulp/ipoib/ipoib_cm.c +2 -1
index 2c13123bfd69..65eacafc898f 100644
@@ -40,6 +40,7 @@
 #include <linux/moduleparam.h>
 #include <linux/sched/signal.h>
 #include <linux/sched/mm.h>
+#include <net/dst_metadata.h>
 
 #include "ipoib.h"
 

@@ -1456,7 +1457,7 @@ void ipoib_cm_skb_too_long(struct net_device *dev, struct sk_buff *skb,
        struct ipoib_dev_priv *priv = ipoib_priv(dev);
        int e = skb_queue_empty(&priv->cm.skb_queue);
 
-       if (skb_dst(skb))
+       if (skb_valid_dst(skb))
                skb_dst(skb)->ops->update_pmtu(skb_dst(skb), NULL, skb, mtu);
 
        skb_queue_tail(&priv->cm.skb_queue, skb);
drivers/net/geneve.c +3 -2
index 195e0d0add8d..2e5d2bc8d851 100644
@@ -19,6 +19,7 @@
 #include <net/rtnetlink.h>
 #include <net/geneve.h>
 #include <net/protocol.h>
+#include <net/dst_metadata.h>
 
 #define GENEVE_NETDEV_VER      "0.6"
 

@@ -825,7 +826,7 @@ static int geneve_xmit_skb(struct sk_buff *skb, struct net_device *dev,
        if (IS_ERR(rt))
                return PTR_ERR(rt);
 
-       if (skb_dst(skb)) {
+       if (skb_valid_dst(skb)) {
                int mtu = dst_mtu(&rt->dst) - sizeof(struct iphdr) -
                          GENEVE_BASE_HLEN - info->options_len - 14;
 

@@ -871,7 +872,7 @@ static int geneve6_xmit_skb(struct sk_buff *skb, struct net_device *dev,
        if (IS_ERR(dst))
                return PTR_ERR(dst);
 
-       if (skb_dst(skb)) {
+       if (skb_valid_dst(skb)) {
                int mtu = dst_mtu(dst) - sizeof(struct ipv6hdr) -
                          GENEVE_BASE_HLEN - info->options_len - 14;
drivers/net/vxlan.c +3 -2
index 82090ae7ced1..b0e96746bc2e 100644
@@ -28,6 +28,7 @@
 #include <net/netns/generic.h>
 #include <net/tun_proto.h>
 #include <net/vxlan.h>
+#include <net/dst_metadata.h>
 
 #if IS_ENABLED(CONFIG_IPV6)
 #include <net/ip6_tunnel.h>

@@ -2155,7 +2156,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
                }
 
                ndst = &rt->dst;
-               if (skb_dst(skb)) {
+               if (skb_valid_dst(skb)) {
                        int mtu = dst_mtu(ndst) - VXLAN_HEADROOM;
 
                        skb_dst(skb)->ops->update_pmtu(skb_dst(skb), NULL,

@@ -2197,7 +2198,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
                                goto out_unlock;
                }
 
-               if (skb_dst(skb)) {
+               if (skb_valid_dst(skb)) {
                        int mtu = dst_mtu(ndst) - VXLAN6_HEADROOM;
 
                        skb_dst(skb)->ops->update_pmtu(skb_dst(skb), NULL,
drivers/s390/net/qeth_l3_main.c +5 -2
index b0c888e86cd4..694efab9cccd 100644
@@ -35,6 +35,7 @@
 #include <net/ip6_fib.h>
 #include <net/ip6_checksum.h>
 #include <net/iucv/af_iucv.h>
+#include <net/dst_metadata.h>
 #include <linux/hashtable.h>
 
 #include "qeth_l3.h"

@@ -2259,9 +2260,11 @@ static int qeth_l3_get_cast_type(struct sk_buff *skb)
        struct dst_entry *dst;
 
        rcu_read_lock();
-       dst = skb_dst(skb);
-       if (dst)
+       if (skb_valid_dst(skb)) {
+               dst = skb_dst(skb);
                n = dst_neigh_lookup_skb(dst, skb);
+       }
+
        if (n) {
                int cast_type = n->type;
include/net/ip6_tunnel.h +1 -0
index 236e40ba06bf..1a704dfe5e1a 100644
@@ -148,6 +148,7 @@ struct net *ip6_tnl_get_link_net(const struct net_device *dev);
 int ip6_tnl_get_iflink(const struct net_device *dev);
 int ip6_tnl_change_mtu(struct net_device *dev, int new_mtu);
 
+/* Must be called with valid skb_dst */
 static inline void ip6tunnel_xmit(struct sock *sk, struct sk_buff *skb,
                                  struct net_device *dev)
 {
net/ipv4/ip_tunnel.c +1 -1
index 5ddb1cb52bd4..9256e8b9c538 100644
@@ -520,7 +520,7 @@ static int tnl_update_pmtu(struct net_device *dev, struct sk_buff *skb,
        else
                mtu = skb_dst(skb) ? dst_mtu(skb_dst(skb)) : dev->mtu;
 
-       if (skb_dst(skb))
+       if (skb_valid_dst(skb))
                skb_dst(skb)->ops->update_pmtu(skb_dst(skb), NULL, skb, mtu);
 
        if (skb->protocol == htons(ETH_P_IP)) {
net/ipv4/ip_vti.c +2 -1
index 949f432a5f04..fdb20a95b819 100644
@@ -43,6 +43,7 @@
 #include <net/xfrm.h>
 #include <net/net_namespace.h>
 #include <net/netns/generic.h>
+#include <net/dst_metadata.h>
 
 static struct rtnl_link_ops vti_link_ops __read_mostly;
 

@@ -172,7 +173,7 @@ static netdev_tx_t vti_xmit(struct sk_buff *skb, struct net_device *dev,
        int err;
        int mtu;
 
-       if (!dst) {
+       if (!skb_valid_dst(skb)) {
                dev->stats.tx_carrier_errors++;
                goto tx_error_icmp;
        }
net/ipv6/sit.c +4 -4
index d7dc23c1b2ca..50a5be98462f 100644
@@ -55,6 +55,7 @@
 #include <net/dsfield.h>
 #include <net/net_namespace.h>
 #include <net/netns/generic.h>
+#include <net/dst_metadata.h>
 
 /*
    This version of net/ipv6/sit.c is cloned of net/ipv4/ip_gre.c

@@ -837,7 +838,7 @@ static netdev_tx_t ipip6_tunnel_xmit(struct sk_buff *skb,
                struct neighbour *neigh = NULL;
                bool do_tx_error = false;
 
-               if (skb_dst(skb))
+               if (skb_valid_dst(skb))
                        neigh = dst_neigh_lookup(skb_dst(skb), &iph6->daddr);
 
                if (!neigh) {

@@ -866,7 +867,7 @@ static netdev_tx_t ipip6_tunnel_xmit(struct sk_buff *skb,
                struct neighbour *neigh = NULL;
                bool do_tx_error = false;
 
-               if (skb_dst(skb))
+               if (skb_valid_dst(skb))
                        neigh = dst_neigh_lookup(skb_dst(skb), &iph6->daddr);
 
                if (!neigh) {

@@ -934,7 +935,7 @@ static netdev_tx_t ipip6_tunnel_xmit(struct sk_buff *skb,
                        df = 0;
                }
 
-               if (tunnel->parms.iph.daddr && skb_dst(skb))
+               if (tunnel->parms.iph.daddr && skb_valid_dst(skb))
                        skb_dst(skb)->ops->update_pmtu(skb_dst(skb), NULL, skb, mtu);
 
                if (skb->len > mtu && !skb_is_gso(skb)) {
--

2.15.1

author
#2 lore raw
Quoted 13 lines · Xin Long
Quoted 4 lines · Roman Kapl

Tunnel devices often use skb_dst(skb)->ops, but ops are not implemented for metadata tunnel destinations. Use skb_valid_dst to check if skb_dst is a real (non-metadata) destination.

Quoted 8 lines

Such packets can easily be crafted using tc tunnel_key actions or BPF and will crash the tunnels, as observed at least with sit, geneve and vxlan. Some of these tunnels are also intended to be used with metadata destinations, so this represents a loss of functionality.

There might be more places with similar patterns, but sometimes it is hard to tell if metadata dst packets can reach them, so this patch is not exhaustive.

Quoted 7 lines

This patch is trying to fix a lot of places, and there may be more. But all because of the lack of .update_pmtu or .neigh_lookup. (dst_link_failure is safe, btw)

Not sure if it will be better to add .update_pmtu, .neigh_lookup and even .redirect for md_dst_ops, but just leave them empty ? like fake_dst_ops in br_nf.

That's what I was suggesting in the original mail. However, it might have slight impact on performance. Also it was not done when metadata destinations were originally added, was there any good reason?

Quoted 1 line

Let's see other's opinions.

Exactly.

Quoted 210 lines
Quoted 210 lines
Fixes 52a589d51f10 ("geneve: update skb dst pmtu on tx path")
Fixes a93bf0ff4490 ("vxlan: update skb dst pmtu on tx path")
Signed-off-by Roman Kapl <[email protected]>
---
  drivers/infiniband/ulp/ipoib/ipoib_cm.c | 3 ++-
  drivers/net/geneve.c                    | 5 +++--
  drivers/net/vxlan.c                     | 5 +++--
  drivers/s390/net/qeth_l3_main.c         | 7 +++++--
  include/net/ip6_tunnel.h                | 1 +
  net/ipv4/ip_tunnel.c                    | 2 +-
  net/ipv4/ip_vti.c                       | 3 ++-
  net/ipv6/sit.c                          | 7 ++++---
  8 files changed, 21 insertions(+), 12 deletions(-)
drivers/infiniband/ulp/ipoib/ipoib_cm.c +2 -1
index 2c13123bfd69..65eacafc898f 100644
@@ -40,6 +40,7 @@
  #include <linux/moduleparam.h>
  #include <linux/sched/signal.h>
  #include <linux/sched/mm.h>
+#include <net/dst_metadata.h>
 
  #include "ipoib.h"
 

@@ -1456,7 +1457,7 @@ void ipoib_cm_skb_too_long(struct net_device *dev, struct sk_buff *skb,
         struct ipoib_dev_priv *priv = ipoib_priv(dev);
         int e = skb_queue_empty(&priv->cm.skb_queue);
 
-       if (skb_dst(skb))
+       if (skb_valid_dst(skb))
                 skb_dst(skb)->ops->update_pmtu(skb_dst(skb), NULL, skb, mtu);
 
         skb_queue_tail(&priv->cm.skb_queue, skb);
drivers/net/geneve.c +3 -2
index 195e0d0add8d..2e5d2bc8d851 100644
@@ -19,6 +19,7 @@
  #include <net/rtnetlink.h>
  #include <net/geneve.h>
  #include <net/protocol.h>
+#include <net/dst_metadata.h>
 
  #define GENEVE_NETDEV_VER      "0.6"
 

@@ -825,7 +826,7 @@ static int geneve_xmit_skb(struct sk_buff *skb, struct net_device *dev,
         if (IS_ERR(rt))
                 return PTR_ERR(rt);
 
-       if (skb_dst(skb)) {
+       if (skb_valid_dst(skb)) {
                 int mtu = dst_mtu(&rt->dst) - sizeof(struct iphdr) -
                           GENEVE_BASE_HLEN - info->options_len - 14;
 

@@ -871,7 +872,7 @@ static int geneve6_xmit_skb(struct sk_buff *skb, struct net_device *dev,
         if (IS_ERR(dst))
                 return PTR_ERR(dst);
 
-       if (skb_dst(skb)) {
+       if (skb_valid_dst(skb)) {
                 int mtu = dst_mtu(dst) - sizeof(struct ipv6hdr) -
                           GENEVE_BASE_HLEN - info->options_len - 14;
drivers/net/vxlan.c +3 -2
index 82090ae7ced1..b0e96746bc2e 100644
@@ -28,6 +28,7 @@
  #include <net/netns/generic.h>
  #include <net/tun_proto.h>
  #include <net/vxlan.h>
+#include <net/dst_metadata.h>
 
  #if IS_ENABLED(CONFIG_IPV6)
  #include <net/ip6_tunnel.h>

@@ -2155,7 +2156,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
                 }
 
                 ndst = &rt->dst;
-               if (skb_dst(skb)) {
+               if (skb_valid_dst(skb)) {
                         int mtu = dst_mtu(ndst) - VXLAN_HEADROOM;
 
                         skb_dst(skb)->ops->update_pmtu(skb_dst(skb), NULL,

@@ -2197,7 +2198,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
                                 goto out_unlock;
                 }
 
-               if (skb_dst(skb)) {
+               if (skb_valid_dst(skb)) {
                         int mtu = dst_mtu(ndst) - VXLAN6_HEADROOM;
 
                         skb_dst(skb)->ops->update_pmtu(skb_dst(skb), NULL,
drivers/s390/net/qeth_l3_main.c +5 -2
index b0c888e86cd4..694efab9cccd 100644
@@ -35,6 +35,7 @@
  #include <net/ip6_fib.h>
  #include <net/ip6_checksum.h>
  #include <net/iucv/af_iucv.h>
+#include <net/dst_metadata.h>
  #include <linux/hashtable.h>
 
  #include "qeth_l3.h"

@@ -2259,9 +2260,11 @@ static int qeth_l3_get_cast_type(struct sk_buff *skb)
         struct dst_entry *dst;
 
         rcu_read_lock();
-       dst = skb_dst(skb);
-       if (dst)
+       if (skb_valid_dst(skb)) {
+               dst = skb_dst(skb);
                 n = dst_neigh_lookup_skb(dst, skb);
+       }
+
         if (n) {
                 int cast_type = n->type;
include/net/ip6_tunnel.h +1 -0
index 236e40ba06bf..1a704dfe5e1a 100644
@@ -148,6 +148,7 @@ struct net *ip6_tnl_get_link_net(const struct net_device *dev);
  int ip6_tnl_get_iflink(const struct net_device *dev);
  int ip6_tnl_change_mtu(struct net_device *dev, int new_mtu);
 
+/* Must be called with valid skb_dst */
  static inline void ip6tunnel_xmit(struct sock *sk, struct sk_buff *skb,
                                   struct net_device *dev)
  {
net/ipv4/ip_tunnel.c +1 -1
index 5ddb1cb52bd4..9256e8b9c538 100644
@@ -520,7 +520,7 @@ static int tnl_update_pmtu(struct net_device *dev, struct sk_buff *skb,
         else
                 mtu = skb_dst(skb) ? dst_mtu(skb_dst(skb)) : dev->mtu;
 
-       if (skb_dst(skb))
+       if (skb_valid_dst(skb))
                 skb_dst(skb)->ops->update_pmtu(skb_dst(skb), NULL, skb, mtu);
 
         if (skb->protocol == htons(ETH_P_IP)) {
net/ipv4/ip_vti.c +2 -1
index 949f432a5f04..fdb20a95b819 100644
@@ -43,6 +43,7 @@
  #include <net/xfrm.h>
  #include <net/net_namespace.h>
  #include <net/netns/generic.h>
+#include <net/dst_metadata.h>
 
  static struct rtnl_link_ops vti_link_ops __read_mostly;
 

@@ -172,7 +173,7 @@ static netdev_tx_t vti_xmit(struct sk_buff *skb, struct net_device *dev,
         int err;
         int mtu;
 
-       if (!dst) {
+       if (!skb_valid_dst(skb)) {
                 dev->stats.tx_carrier_errors++;
                 goto tx_error_icmp;
         }
net/ipv6/sit.c +4 -4
index d7dc23c1b2ca..50a5be98462f 100644
@@ -55,6 +55,7 @@
  #include <net/dsfield.h>
  #include <net/net_namespace.h>
  #include <net/netns/generic.h>
+#include <net/dst_metadata.h>
 
  /*
     This version of net/ipv6/sit.c is cloned of net/ipv4/ip_gre.c

@@ -837,7 +838,7 @@ static netdev_tx_t ipip6_tunnel_xmit(struct sk_buff *skb,
                 struct neighbour *neigh = NULL;
                 bool do_tx_error = false;
 
-               if (skb_dst(skb))
+               if (skb_valid_dst(skb))
                         neigh = dst_neigh_lookup(skb_dst(skb), &iph6->daddr);
 
                 if (!neigh) {

@@ -866,7 +867,7 @@ static netdev_tx_t ipip6_tunnel_xmit(struct sk_buff *skb,
                 struct neighbour *neigh = NULL;
                 bool do_tx_error = false;
 
-               if (skb_dst(skb))
+               if (skb_valid_dst(skb))
                         neigh = dst_neigh_lookup(skb_dst(skb), &iph6->daddr);
 
                 if (!neigh) {

@@ -934,7 +935,7 @@ static netdev_tx_t ipip6_tunnel_xmit(struct sk_buff *skb,
                         df = 0;
                 }
 
-               if (tunnel->parms.iph.daddr && skb_dst(skb))
+               if (tunnel->parms.iph.daddr && skb_valid_dst(skb))
                         skb_dst(skb)->ops->update_pmtu(skb_dst(skb), NULL, skb, mtu);
 
                 if (skb->len > mtu && !skb_is_gso(skb)) {
--

2.15.1

#3 lore raw

Le 24/01/2018 à 10:49, Roman Kapl a écrit :

Quoted 18 lines · Xin Long
Quoted 17 lines · Roman Kapl
Quoted 4 lines

Tunnel devices often use skb_dst(skb)->ops, but ops are not implemented for metadata tunnel destinations. Use skb_valid_dst to check if skb_dst is a real (non-metadata) destination.

Quoted 8 lines

Such packets can easily be crafted using tc tunnel_key actions or BPF and will crash the tunnels, as observed at least with sit, geneve and vxlan. Some of these tunnels are also intended to be used with metadata destinations, so this represents a loss of functionality.

There might be more places with similar patterns, but sometimes it is hard to tell if metadata dst packets can reach them, so this patch is not exhaustive.

This patch is trying to fix a lot of places, and there may be more. But all because of the lack of .update_pmtu or .neigh_lookup. (dst_link_failure is safe, btw)

Quoted 8 lines
Quoted 3 lines

Not sure if it will be better to add .update_pmtu, .neigh_lookup and even .redirect for md_dst_ops, but just leave them empty ? like fake_dst_ops in br_nf.

That's what I was suggesting in the original mail. However, it might have slight impact on performance. Also it was not done when metadata destinations were originally added, was there any good reason?

Quoted 1 line

Let's see other's opinions.

Exactly.

I would prefer a patch that test if the handler is available. It would prevent to have this bug again in the future. But I don't have a strong opinion about this.

But it would be nice to send a patch quickly, before the 4.15 release, to fix this crash.

Thank you,
Nicolas

maintainer
#4 lore raw

From: Nicolas Dichtel <[email protected]>
Date: Wed, 24 Jan 2018 16:37:56 +0100

Quoted 3 lines

I would prefer a patch that test if the handler is available. It would prevent to have this bug again in the future. But I don't have a strong opinion about this.

Yes that's probably better, making all calls to these dst_ops that are troublesome for metadata-dsts go through a helper function.

Can someone code that up quickly?

Thanks.

#5 lore raw

PATCH 1/1 net: don't call update_pmtu unconditionally

Some dst_ops (e.g. md_dst_ops)) doesn't set this handler. It may result to:
"BUG: unable to handle kernel NULL pointer dereference at (null)"

Let's add a helper to check if update_pmtu is available before calling it.

Fixes 52a589d51f10 ("geneve: update skb dst pmtu on tx path")
Fixes a93bf0ff4490 ("vxlan: update skb dst pmtu on tx path")
CC Roman Kapl <[email protected]>
CC Xin Long <[email protected]>
Signed-off-by Nicolas Dichtel <[email protected]>
---
 drivers/infiniband/ulp/ipoib/ipoib_cm.c | 3 +--
 drivers/net/geneve.c                    | 4 ++--
 drivers/net/vxlan.c                     | 6 ++----
 include/net/dst.h                       | 8 ++++++++
 net/ipv4/ip_tunnel.c                    | 3 +--
 net/ipv4/ip_vti.c                       | 2 +-
 net/ipv6/ip6_tunnel.c                   | 6 ++----
 net/ipv6/ip6_vti.c                      | 2 +-
 net/ipv6/sit.c                          | 4 ++--
 9 files changed, 20 insertions(+), 18 deletions(-)
drivers/infiniband/ulp/ipoib/ipoib_cm.c +1 -2
index 2c13123bfd69..71ea9e26666c 100644
@@ -1456,8 +1456,7 @@ void ipoib_cm_skb_too_long(struct net_device *dev, struct sk_buff *skb,
 	struct ipoib_dev_priv *priv = ipoib_priv(dev);
 	int e = skb_queue_empty(&priv->cm.skb_queue);
 
-	if (skb_dst(skb))
-		skb_dst(skb)->ops->update_pmtu(skb_dst(skb), NULL, skb, mtu);
+	skb_dst_update_pmtu(skb, mtu);
 
 	skb_queue_tail(&priv->cm.skb_queue, skb);
 	if (e)
drivers/net/geneve.c +2 -2
index 0a48b3073d3d..64fda2e1040e 100644
@@ -829,7 +829,7 @@ static int geneve_xmit_skb(struct sk_buff *skb, struct net_device *dev,
 		int mtu = dst_mtu(&rt->dst) - sizeof(struct iphdr) -
 			  GENEVE_BASE_HLEN - info->options_len - 14;
 
-		skb_dst(skb)->ops->update_pmtu(skb_dst(skb), NULL, skb, mtu);
+		skb_dst_update_pmtu(skb, mtu);
 	}
 
 	sport = udp_flow_src_port(geneve->net, skb, 1, USHRT_MAX, true);

@@ -875,7 +875,7 @@ static int geneve6_xmit_skb(struct sk_buff *skb, struct net_device *dev,
 		int mtu = dst_mtu(dst) - sizeof(struct ipv6hdr) -
 			  GENEVE_BASE_HLEN - info->options_len - 14;
 
-		skb_dst(skb)->ops->update_pmtu(skb_dst(skb), NULL, skb, mtu);
+		skb_dst_update_pmtu(skb, mtu);
 	}
 
 	sport = udp_flow_src_port(geneve->net, skb, 1, USHRT_MAX, true);
drivers/net/vxlan.c +2 -4
index 31f4b7911ef8..c3e34e3c82a7 100644
@@ -2158,8 +2158,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 		if (skb_dst(skb)) {
 			int mtu = dst_mtu(ndst) - VXLAN_HEADROOM;
 
-			skb_dst(skb)->ops->update_pmtu(skb_dst(skb), NULL,
-						       skb, mtu);
+			skb_dst_update_pmtu(skb, mtu);
 		}
 
 		tos = ip_tunnel_ecn_encap(tos, old_iph, skb);

@@ -2200,8 +2199,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 		if (skb_dst(skb)) {
 			int mtu = dst_mtu(ndst) - VXLAN6_HEADROOM;
 
-			skb_dst(skb)->ops->update_pmtu(skb_dst(skb), NULL,
-						       skb, mtu);
+			skb_dst_update_pmtu(skb, mtu);
 		}
 
 		tos = ip_tunnel_ecn_encap(tos, old_iph, skb);
include/net/dst.h +8 -0
index b091fd536098..6216edb58393 100644
@@ -521,4 +521,12 @@ static inline struct xfrm_state *dst_xfrm(const struct dst_entry *dst)
 }
 #endif
 
+static inline void skb_dst_update_pmtu(struct sk_buff *skb, u32 mtu)
+{
+	struct dst_entry *dst = skb_dst(skb);
+
+	if (dst && dst->ops->update_pmtu)
+		skb_dst(skb)->ops->update_pmtu(dst, NULL, skb, mtu);
+}
+
 #endif /* _NET_DST_H */
net/ipv4/ip_tunnel.c +1 -2
index 5ddb1cb52bd4..6d21068f9b55 100644
@@ -520,8 +520,7 @@ static int tnl_update_pmtu(struct net_device *dev, struct sk_buff *skb,
 	else
 		mtu = skb_dst(skb) ? dst_mtu(skb_dst(skb)) : dev->mtu;
 
-	if (skb_dst(skb))
-		skb_dst(skb)->ops->update_pmtu(skb_dst(skb), NULL, skb, mtu);
+	skb_dst_update_pmtu(skb, mtu);
 
 	if (skb->protocol == htons(ETH_P_IP)) {
 		if (!skb_is_gso(skb) &&
net/ipv4/ip_vti.c +1 -1
index 949f432a5f04..51b1669334fe 100644
@@ -200,7 +200,7 @@ static netdev_tx_t vti_xmit(struct sk_buff *skb, struct net_device *dev,
 
 	mtu = dst_mtu(dst);
 	if (skb->len > mtu) {
-		skb_dst(skb)->ops->update_pmtu(skb_dst(skb), NULL, skb, mtu);
+		skb_dst_update_pmtu(skb, mtu);
 		if (skb->protocol == htons(ETH_P_IP)) {
 			icmp_send(skb, ICMP_DEST_UNREACH, ICMP_FRAG_NEEDED,
 				  htonl(mtu));
net/ipv6/ip6_tunnel.c +2 -4
index 9a7cf355bc8c..1ee5584c3555 100644
@@ -642,8 +642,7 @@ ip4ip6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 		if (rel_info > dst_mtu(skb_dst(skb2)))
 			goto out;
 
-		skb_dst(skb2)->ops->update_pmtu(skb_dst(skb2), NULL, skb2,
-						rel_info);
+		skb_dst_update_pmtu(skb2, rel_info);
 	}
 
 	icmp_send(skb2, rel_type, rel_code, htonl(rel_info));

@@ -1131,8 +1130,7 @@ int ip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev, __u8 dsfield,
 		mtu = 576;
 	}
 
-	if (skb_dst(skb) && !t->parms.collect_md)
-		skb_dst(skb)->ops->update_pmtu(skb_dst(skb), NULL, skb, mtu);
+	skb_dst_update_pmtu(skb, mtu);
 	if (skb->len - t->tun_hlen - eth_hlen > mtu && !skb_is_gso(skb)) {
 		*pmtu = mtu;
 		err = -EMSGSIZE;
net/ipv6/ip6_vti.c +1 -1
index dbb74f3c57a7..8c184f84f353 100644
@@ -483,7 +483,7 @@ vti6_xmit(struct sk_buff *skb, struct net_device *dev, struct flowi *fl)
 
 	mtu = dst_mtu(dst);
 	if (!skb->ignore_df && skb->len > mtu) {
-		skb_dst(skb)->ops->update_pmtu(dst, NULL, skb, mtu);
+		skb_dst_update_pmtu(skb, mtu);
 
 		if (skb->protocol == htons(ETH_P_IPV6)) {
 			if (mtu < IPV6_MIN_MTU)
net/ipv6/sit.c +2 -2
index d7dc23c1b2ca..3873d3877135 100644
@@ -934,8 +934,8 @@ static netdev_tx_t ipip6_tunnel_xmit(struct sk_buff *skb,
 			df = 0;
 		}
 
-		if (tunnel->parms.iph.daddr && skb_dst(skb))
-			skb_dst(skb)->ops->update_pmtu(skb_dst(skb), NULL, skb, mtu);
+		if (tunnel->parms.iph.daddr)
+			skb_dst_update_pmtu(skb, mtu);
 
 		if (skb->len > mtu && !skb_is_gso(skb)) {
 			icmpv6_send(skb, ICMPV6_PKT_TOOBIG, 0, mtu);
Signature · 1 line
2.15.1
#6 lore raw

Re: [PATCH net] net: don't call update_pmtu unconditionally

Quoted 5 lines · Nicolas Dichtel
include/net/dst.h +0 -0
index b091fd536098..6216edb58393 100644

include/net/dst.h static inline struct xfrm_state *dst_xfrm(const struct dst_entry *dst) · line 529

Quoted 9 lines
include/net/dst.h +6 -0
@@ -521,4 +521,12 @@ static inline struct xfrm_state *dst_xfrm(const struct dst_entry *dst)
 }
 #endif
 
+static inline void skb_dst_update_pmtu(struct sk_buff *skb, u32 mtu)
+{
+	struct dst_entry *dst = skb_dst(skb);
+
+	if (dst && dst->ops->update_pmtu)
+		skb_dst(skb)->ops->update_pmtu(dst, NULL, skb, mtu);

in that last line, s/skb_dst(skb)/dst/

maintainer
#7 lore raw

Re: [PATCH net] net: don't call update_pmtu unconditionally

In reply to Nicolas Dichtel

From: Nicolas Dichtel <[email protected]>
Date: Thu, 25 Jan 2018 19:03:03 +0100

Quoted 10 lines

Some dst_ops (e.g. md_dst_ops)) doesn't set this handler. It may result to:
"BUG: unable to handle kernel NULL pointer dereference at (null)"

Let's add a helper to check if update_pmtu is available before calling it.

Fixes 52a589d51f10 ("geneve: update skb dst pmtu on tx path")
Fixes a93bf0ff4490 ("vxlan: update skb dst pmtu on tx path")
CC Roman Kapl <[email protected]>
CC Xin Long <[email protected]>
Signed-off-by Nicolas Dichtel <[email protected]>

Applied with the fixup suggested by David Ahern and queued up for -stable, thank you!

#8 lore raw

Re: [PATCH net] net: don't call update_pmtu unconditionally

Le 25/01/2018 à 22:28, David Miller a écrit :

Quoted 8 lines

From: Nicolas Dichtel <[email protected]>
Date: Thu, 25 Jan 2018 19:03:03 +0100

Quoted 5 lines

Some dst_ops (e.g. md_dst_ops)) doesn't set this handler. It may result to:
"BUG: unable to handle kernel NULL pointer dereference at (null)"

Let's add a helper to check if update_pmtu is available before calling it.

Quoted 7 lines
Quoted 5 lines
Fixes 52a589d51f10 ("geneve: update skb dst pmtu on tx path")
Fixes a93bf0ff4490 ("vxlan: update skb dst pmtu on tx path")
CC Roman Kapl <[email protected]>
CC Xin Long <[email protected]>
Signed-off-by Nicolas Dichtel <[email protected]>

Applied with the fixup suggested by David Ahern and queued up for -stable,

Thank you for taking care of that.

Regards,
Nicolas