Re: [PATCH] net: make sure skb_dst is valid before using

Roman Kapl Wed, 24 Jan 2018 01:50:06 -0800

On 01/24/2018 10:16 AM, Xin Long wrote:

On Wed, Jan 24, 2018 at 6:42 AM, Roman Kapl <c...@rkapl.cz> wrote:

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.

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 mighthave slight impact on performance. Also it was not done when metadatadestinations were originally added, was there any good reason?

Let's see other's opinions.

Exactly.

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 <c...@rkapl.cz>
---
  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(-)

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_cm.c 
b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
index 2c13123bfd69..65eacafc898f 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_cm.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
@@ -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);
diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 195e0d0add8d..2e5d2bc8d851 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -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;

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 82090ae7ced1..b0e96746bc2e 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -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,
diff --git a/drivers/s390/net/qeth_l3_main.c b/drivers/s390/net/qeth_l3_main.c
index b0c888e86cd4..694efab9cccd 100644
--- a/drivers/s390/net/qeth_l3_main.c
+++ b/drivers/s390/net/qeth_l3_main.c
@@ -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;

diff --git a/include/net/ip6_tunnel.h b/include/net/ip6_tunnel.h
index 236e40ba06bf..1a704dfe5e1a 100644
--- a/include/net/ip6_tunnel.h
+++ b/include/net/ip6_tunnel.h
@@ -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)
  {
diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index 5ddb1cb52bd4..9256e8b9c538 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -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)) {
diff --git a/net/ipv4/ip_vti.c b/net/ipv4/ip_vti.c
index 949f432a5f04..fdb20a95b819 100644
--- a/net/ipv4/ip_vti.c
+++ b/net/ipv4/ip_vti.c
@@ -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;
         }
diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
index d7dc23c1b2ca..50a5be98462f 100644
--- a/net/ipv6/sit.c
+++ b/net/ipv6/sit.c
@@ -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

Re: [PATCH] net: make sure skb_dst is valid before using

Reply via email to