ipv6: Allocate unique metrics for icmp6 packets to prevent tainting dst metrics

PATCH Superseded Nick Jones 3 participants 8 messages Mar 15, 2012 – Mar 20, 2012

Subsystems (3)

Networking Network drivers IPv4/IPv6

lore

Share & tools

lore

Nick Jones

author

2012-03-15 15:42 UTC #0 lore raw

PATCH v2 1/1 ipv6: Allocate unique metrics for icmp6 packets to prevent tainting dst metrics

The generation of an icmp6 packet, targeted to a specific desination address, will cause the shared metrics of the ip6_dst and inetpeer of that address to be tainted with the hoplimit value 255. All subsequent packets to the dst, will have this hoplimit value, and if the destination is a router, not even advertisements specifying a new hoplimit value will have any effect reverting this due to the way ip6_dst_hoplimit works.

By allocating a unique metrics array for the icmp6 packet, the shared metrics will not be tainted. A ip6_dst flag is added to indicate that the metrics for the dst don't belong to the peer, thus are not unique and should be freed when the dst is destroyed.

Signed-off-by Nick Jones <[email protected]>

---

v2: Take care of destroy side, that was neglected in the previous
version, by setting a flag on the dst to indicate its metrics are unique, thus should be destroyed along with the dst.

 include/net/dst.h |    1 +
 net/ipv6/route.c  |   12 ++++++++++--
 2 files changed, 11 insertions(+), 2 deletions(-)

include/net/dst.h +1 -0

index 344c8dd..ee7a781 100644
@@ -54,6 +54,7 @@ struct dst_entry {
 #define DST_NOCACHE		0x0010
 #define DST_NOCOUNT		0x0020
 #define DST_NOPEER		0x0040
+#define DST_NOPEERMETRICS	0x0080
 
 	short			error;
 	short			obsolete;

net/ipv6/route.c +10 -2

index 92be12b..f0a1839 100644
@@ -278,7 +278,7 @@ static void ip6_dst_destroy(struct dst_entry *dst)
 	struct inet6_dev *idev = rt->rt6i_idev;
 	struct inet_peer *peer = rt->rt6i_peer;
 
-	if (!(rt->dst.flags & DST_HOST))
+	if (!(rt->dst.flags & DST_HOST) || (rt->dst.flags & DST_NOPEERMETRICS))
 		dst_destroy_metrics_generic(dst);
 
 	if (idev) {

@@ -1110,13 +1110,21 @@ struct dst_entry *icmp6_dst_alloc(struct net_device *dev,
 		}
 	}
 
-	rt->dst.flags |= DST_HOST;
+	rt->dst.flags |= (DST_HOST | DST_NOPEERMETRICS);
 	rt->dst.output  = ip6_output;
 	dst_set_neighbour(&rt->dst, neigh);
 	atomic_set(&rt->dst.__refcnt, 1);
 	rt->rt6i_dst.addr = fl6->daddr;
 	rt->rt6i_dst.plen = 128;
 	rt->rt6i_idev     = idev;
+
+	u32 *metrics = kzalloc(sizeof(u32) * RTAX_MAX, GFP_ATOMIC);
+	if (unlikely(!metrics)) {
+		in6_dev_put(idev);
+		dst_free(&rt->dst);
+		return ERR_CAST(-ENOMEM);
+	}
+	dst_init_metrics(&rt->dst, metrics, 0);
 	dst_metric_set(&rt->dst, RTAX_HOPLIMIT, 255);
 
 	spin_lock_bh(&icmp6_dst_lock);

Signature · 1 line

1.7.1

David Miller

maintainer

2012-03-17 06:02 UTC #1 lore raw

From: Nick Jones <[email protected]>
Date: Thu, 15 Mar 2012 23:41:57 +0800

net/ipv6/route.c struct dst_entry *icmp6_dst_alloc(struct net_device *dev, · line 1121

Quoted 1 line

net/ipv6/route.c +1 -0

+	u32 *metrics = kzalloc(sizeof(u32) * RTAX_MAX, GFP_ATOMIC);

Please do not declare local variables in the middle of a function's statements. Put it at the top of the basic block where it belongs.

Nick Jones

author

2012-03-17 15:47 UTC #2 lore raw

PATCH v3 1/1 ipv6: Allocate unique metrics for icmp6 packets to prevent tainting dst metrics

The generation of an icmp6 packet, targeted to a specific desination address, will cause the shared metrics of the ip6_dst and inetpeer of that address to be tainted with the hoplimit value 255. All packets, icmp6 or otherwise, will have this hoplimit value, and if the destination is a router, not even advertisements specifying a new hoplimit value will have any effect due to the way ip6_dst_hoplimit works.

Signed-off-by Nick Jones <[email protected]>

---

v2: Take care of destroy side, that was neglected in the previous
version, by setting a flag on the dst to indicate its metrics are unique, thus should be destroyed along with the dst.
v3: declare metrics pointer variable at function top

 include/net/dst.h |    1 +
 net/ipv6/route.c  |   13 +++++++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

include/net/dst.h +1 -0

index 344c8dd..ee7a781 100644
@@ -54,6 +54,7 @@ struct dst_entry {
 #define DST_NOCACHE		0x0010
 #define DST_NOCOUNT		0x0020
 #define DST_NOPEER		0x0040
+#define DST_NOPEERMETRICS	0x0080
 
 	short			error;
 	short			obsolete;

net/ipv6/route.c +11 -2

index 92be12b..1f217cb 100644
@@ -278,7 +278,7 @@ static void ip6_dst_destroy(struct dst_entry *dst)
 	struct inet6_dev *idev = rt->rt6i_idev;
 	struct inet_peer *peer = rt->rt6i_peer;
 
-	if (!(rt->dst.flags & DST_HOST))
+	if (!(rt->dst.flags & DST_HOST) || (rt->dst.flags & DST_NOPEERMETRICS))
 		dst_destroy_metrics_generic(dst);
 
 	if (idev) {

@@ -1088,6 +1088,7 @@ struct dst_entry *icmp6_dst_alloc(struct net_device *dev,
 	struct rt6_info *rt;
 	struct inet6_dev *idev = in6_dev_get(dev);
 	struct net *net = dev_net(dev);
+	u32 *metrics;
 
 	if (unlikely(!idev))
 		return NULL;

@@ -1110,13 +1111,21 @@ struct dst_entry *icmp6_dst_alloc(struct net_device *dev,
 		}
 	}
 
-	rt->dst.flags |= DST_HOST;
+	rt->dst.flags |= (DST_HOST | DST_NOPEERMETRICS);
 	rt->dst.output  = ip6_output;
 	dst_set_neighbour(&rt->dst, neigh);
 	atomic_set(&rt->dst.__refcnt, 1);
 	rt->rt6i_dst.addr = fl6->daddr;
 	rt->rt6i_dst.plen = 128;
 	rt->rt6i_idev     = idev;
+
+	metrics = kzalloc(sizeof(u32) * RTAX_MAX, GFP_ATOMIC);
+	if (unlikely(!metrics)) {
+		in6_dev_put(idev);
+		dst_free(&rt->dst);
+		return ERR_CAST(-ENOMEM);
+	}
+	dst_init_metrics(&rt->dst, metrics, 0);
 	dst_metric_set(&rt->dst, RTAX_HOPLIMIT, 255);
 
 	spin_lock_bh(&icmp6_dst_lock);

Signature · 1 line

1.7.1

Eric Dumazet

2012-03-17 16:36 UTC #3 lore raw

Quoted 16 lines · Nick Jones

The generation of an icmp6 packet, targeted to a specific desination address, will cause the shared metrics of the ip6_dst and inetpeer of that address to be tainted with the hoplimit value 255. All packets, icmp6 or otherwise, will have this hoplimit value, and if the destination is a router, not even advertisements specifying a new hoplimit value will have any effect due to the way ip6_dst_hoplimit works.
By allocating a unique metrics array for the icmp6 packet, the shared metrics will not be tainted. A ip6_dst flag is added to indicate that the metrics for the dst don't belong to the peer, thus are not unique and should be freed when the dst is freed.
Signed-off-by Nick Jones <[email protected]>
---

net/ipv6/route.c struct dst_entry *icmp6_dst_alloc(struct net_device *dev, · line 1128

Quoted 2 lines

net/ipv6/route.c +2 -0

+	}
+	dst_init_metrics(&rt->dst, metrics, 0);

Dont be fooled by 8e2ec639 precedent, third argument is a bool, so please use 'false' instead of 0

Nick Jones

author

2012-03-17 17:43 UTC #4 lore raw

PATCH v4 1/1 ipv6: Allocate unique metrics for icmp6 packets to prevent tainting dst metrics

The generation of an icmp6 packet, targeted to a specific desination address, will cause the shared metrics of the ip6_dst and inetpeer of that address to be tainted with the hoplimit value 255. All packets, icmp6 or otherwise, will have this hoplimit value, and if the destination is a router, not even advertisements specifying a new hoplimit value will have any effect due to the way ip6_dst_hoplimit works.

Signed-off-by Nick Jones <[email protected]>

---

v2: Forgot to handle destroy side, as pointed out by David Miller.
Achieved by setting a flag on the dst to indicate its metrics are unique, thus should be destroyed along with the dst.
v3: David Miller reminds that metrics pointer should be declared at
function top.
v4: Eric Dumazet suggest argument of dst_init_metrics change to from
literal 0 to keyword 'false'.

 include/net/dst.h |    1 +
 net/ipv6/route.c  |   13 +++++++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

include/net/dst.h +1 -0

index 344c8dd..ee7a781 100644
@@ -54,6 +54,7 @@ struct dst_entry {
 #define DST_NOCACHE		0x0010
 #define DST_NOCOUNT		0x0020
 #define DST_NOPEER		0x0040
+#define DST_NOPEERMETRICS	0x0080
 
 	short			error;
 	short			obsolete;

net/ipv6/route.c +11 -2

index 92be12b..d96ccc5 100644
@@ -278,7 +278,7 @@ static void ip6_dst_destroy(struct dst_entry *dst)
 	struct inet6_dev *idev = rt->rt6i_idev;
 	struct inet_peer *peer = rt->rt6i_peer;
 
-	if (!(rt->dst.flags & DST_HOST))
+	if (!(rt->dst.flags & DST_HOST) || (rt->dst.flags & DST_NOPEERMETRICS))
 		dst_destroy_metrics_generic(dst);
 
 	if (idev) {

@@ -1088,6 +1088,7 @@ struct dst_entry *icmp6_dst_alloc(struct net_device *dev,
 	struct rt6_info *rt;
 	struct inet6_dev *idev = in6_dev_get(dev);
 	struct net *net = dev_net(dev);
+	u32 *metrics;
 
 	if (unlikely(!idev))
 		return NULL;

@@ -1110,13 +1111,21 @@ struct dst_entry *icmp6_dst_alloc(struct net_device *dev,
 		}
 	}
 
-	rt->dst.flags |= DST_HOST;
+	rt->dst.flags |= (DST_HOST | DST_NOPEERMETRICS);
 	rt->dst.output  = ip6_output;
 	dst_set_neighbour(&rt->dst, neigh);
 	atomic_set(&rt->dst.__refcnt, 1);
 	rt->rt6i_dst.addr = fl6->daddr;
 	rt->rt6i_dst.plen = 128;
 	rt->rt6i_idev     = idev;
+
+	metrics = kzalloc(sizeof(u32) * RTAX_MAX, GFP_ATOMIC);
+	if (unlikely(!metrics)) {
+		in6_dev_put(idev);
+		dst_free(&rt->dst);
+		return ERR_CAST(-ENOMEM);
+	}
+	dst_init_metrics(&rt->dst, metrics, false);
 	dst_metric_set(&rt->dst, RTAX_HOPLIMIT, 255);
 
 	spin_lock_bh(&icmp6_dst_lock);

Signature · 1 line

1.7.1

David Miller

maintainer

2012-03-19 22:04 UTC #5 lore raw

From: Nick Jones <[email protected]>
Date: Sun, 18 Mar 2012 01:43:39 +0800

net/ipv6/route.c struct dst_entry *icmp6_dst_alloc(struct net_device *dev, · line 1126

Quoted 1 line

net/ipv6/route.c +1 -0
+		return ERR_CAST(-ENOMEM);

Really, please, stop wasting my time. There is no way your compiler didn't emit a warning for that garbage.

Furthermore, callers are only ready to handle NULL vs. non-NULL as return values from this function. So this return value you are adding will result in crashes.

Nick Jones

author

2012-03-20 05:54 UTC #6 lore raw

Quoted 8 lines · David Miller

From: Nick Jones <[email protected]>
Date: Sun, 18 Mar 2012 01:43:39 +0800
net/ipv6/route.c struct dst_entry *icmp6_dst_alloc(struct net_device *dev, · line 1126
Quoted 1 line
net/ipv6/route.c +1 -0
+		return ERR_CAST(-ENOMEM);
Really, please, stop wasting my time. There is no way your compiler didn't emit a warning for that garbage.

This is really sloppy, I apologise. I'll work on tightening up my workflow for build testing, runtime testing and patch submission before I bother you again.

Quoted 3 lines

Furthermore, callers are only ready to handle NULL vs. non-NULL as return values from this function. So this return value you are adding will result in crashes.

This one I did check. icmp6_dst_alloc returns error encoded pointers and all call sites test the return value with IS_ERR, I simply didn't understand the correct formatting macro, nor check the build properly. I think you got it mixed up with ip6_dst_alloc

The 'return NULL' at the bottom of the second fragment was a bug, but this again exposes my newness to kernel patch submission workflow: that bug was fixed recently and this patch wouldn't have applied, further wasting your time... wonderful.

I'll let the merge window close to let things settle down for you before bringing this fix up again.

David Miller

maintainer

2012-03-20 07:10 UTC #7 lore raw

From: Nick Jones <[email protected]>
Date: Tue, 20 Mar 2012 13:48:01 +0800

Quoted 9 lines · David Miller

Quoted 3 lines
Furthermore, callers are only ready to handle NULL vs. non-NULL as return values from this function. So this return value you are adding will result in crashes.
This one I did check. icmp6_dst_alloc returns error encoded pointers and all call sites test the return value with IS_ERR, I simply didn't understand the correct formatting macro, nor check the build properly. I think you got it mixed up with ip6_dst_alloc

Indeed you're right on this one. We changed this recently, in fact.