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

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

[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.

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 <code@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

[Xin Long]

On Wed, Jan 24, 2018 at 6:42 AM, Roman Kapl <code@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.

Let's see other's opinions.

>
> 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 <code@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

[Roman Kapl]

On 01/24/2018 10:16 AM, Xin Long wrote:
> On Wed, Jan 24, 2018 at 6:42 AM, Roman Kapl <code@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 might 
have slight impact on performance. Also it was not done when metadata 
destinations 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 <code@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

[Nicolas Dichtel]

Le 24/01/2018 à 10:49, Roman Kapl a écrit :
> On 01/24/2018 10:16 AM, Xin Long wrote:
>> On Wed, Jan 24, 2018 at 6:42 AM, Roman Kapl <code@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 might have slight
> impact on performance. Also it was not done when metadata destinations were
> originally added, was there any good reason?
>> 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

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

[David Miller]

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Wed, 24 Jan 2018 16:37:56 +0100

> 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.

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

[Nicolas Dichtel]

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 <code@rkapl.cz>
CC: Xin Long <lucien.xin@gmail.com>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 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(-)

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_cm.c b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
index 2c13123bfd69..71ea9e26666c 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_cm.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
@@ -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)
diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 0a48b3073d3d..64fda2e1040e 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -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);
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 31f4b7911ef8..c3e34e3c82a7 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -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);
diff --git a/include/net/dst.h b/include/net/dst.h
index b091fd536098..6216edb58393 100644
--- a/include/net/dst.h
+++ b/include/net/dst.h
@@ -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 */
diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index 5ddb1cb52bd4..6d21068f9b55 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -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) &&
diff --git a/net/ipv4/ip_vti.c b/net/ipv4/ip_vti.c
index 949f432a5f04..51b1669334fe 100644
--- a/net/ipv4/ip_vti.c
+++ b/net/ipv4/ip_vti.c
@@ -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));
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 9a7cf355bc8c..1ee5584c3555 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -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;
diff --git a/net/ipv6/ip6_vti.c b/net/ipv6/ip6_vti.c
index dbb74f3c57a7..8c184f84f353 100644
--- a/net/ipv6/ip6_vti.c
+++ b/net/ipv6/ip6_vti.c
@@ -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)
diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
index d7dc23c1b2ca..3873d3877135 100644
--- a/net/ipv6/sit.c
+++ b/net/ipv6/sit.c
@@ -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);
-- 
2.15.1

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

[David Ahern]

On 1/25/18 11:03 AM, Nicolas Dichtel wrote:
> diff --git a/include/net/dst.h b/include/net/dst.h
> index b091fd536098..6216edb58393 100644
> --- a/include/net/dst.h
> +++ b/include/net/dst.h
> @@ -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/

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

[David Miller]

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Thu, 25 Jan 2018 19:03:03 +0100

> 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 <code@rkapl.cz>
> CC: Xin Long <lucien.xin@gmail.com>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

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

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

[Nicolas Dichtel]

Le 25/01/2018 à 22:28, David Miller a écrit :
> From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> Date: Thu, 25 Jan 2018 19:03:03 +0100
> 
>> 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 <code@rkapl.cz>
>> CC: Xin Long <lucien.xin@gmail.com>
>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> 
> Applied with the fixup suggested by David Ahern and queued up for -stable,
Thank you for taking care of that.


Regards,
Nicolas