[PATCH net-next v1 1/4] tipc: add net device to skb before UDP xmit

Prior to this patch enabling a IPv4 UDP bearer caused a null pointer
dereference in iptunnel_xmit_stats(), when it tried to dereference the
net device from the skb. To resolve this we now point the skb device
to the net device resolved from the routing table.

Fixes: 039f50629b7f (ip_tunnel: Move stats update to iptunnel_xmit())
Signed-off-by: Richard Alpe <redacted>
Acked-by: Jon Maloy <redacted>
Reviewed-by: Erik Hugne <redacted>
---
 net/tipc/udp_media.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c
index d63a911..f22a5bb1 100644
--- a/net/tipc/udp_media.c
+++ b/net/tipc/udp_media.c

@@ -181,6 +181,8 @@ static int tipc_udp_send_msg(struct net *net, struct sk_buff *skb,
 			err = PTR_ERR(rt);
 			goto tx_error;
 		}
+
+		skb->dev = rt->dst.dev;
 		ttl = ip4_dst_hoplimit(&rt->dst);
 		udp_tunnel_xmit_skb(rt, ub->ubsock->sk, skb, src->ipv4.s_addr,
 				    dst->ipv4.s_addr, 0, ttl, 0, src->udp_port,

-- 
2.1.4

Make sure the user has provided a scope for multicast and link local
addresses used locally by a UDP bearer.

Signed-off-by: Richard Alpe <redacted>
Acked-by: Jon Maloy <redacted>
Reviewed-by: Erik Hugne <redacted>
---
 net/tipc/udp_media.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c
index 6fe8740..fb2f7ec6 100644
--- a/net/tipc/udp_media.c
+++ b/net/tipc/udp_media.c

@@ -310,9 +310,14 @@ err:
 
 #if IS_ENABLED(CONFIG_IPV6)
 	} else if ((sa_local.ss_family & sa_remote.ss_family) == AF_INET6) {
+		int atype;
 		struct sockaddr_in6 *ip6;
 
 		ip6 = (struct sockaddr_in6 *)&sa_local;
+		atype = ipv6_addr_type(&ip6->sin6_addr);
+		if (__ipv6_addr_needs_scope_id(atype) && !ip6->sin6_scope_id)
+			return -EINVAL;
+
 		local->proto = htons(ETH_P_IPV6);
 		local->udp_port = ip6->sin6_port;
 		memcpy(&local->ipv6, &ip6->sin6_addr, sizeof(struct in6_addr));

-- 
2.1.4


------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140

Make sure we have a link before checking if it has been reset or not.

Prior to this patch tipc_link_is_reset() could be called with a non
existing link, resulting in a null pointer dereference.

Signed-off-by: Richard Alpe <redacted>
Acked-by: Jon Maloy <redacted>
Reviewed-by: Erik Hugne <redacted>
---
 net/tipc/node.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/tipc/node.c b/net/tipc/node.c
index cdb7950..590d597 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c

@@ -843,7 +843,7 @@ void tipc_node_check_dest(struct net *net, u32 onode,
 	memcpy(&le->maddr, maddr, sizeof(*maddr));
 exit:
 	tipc_node_write_unlock(n);
-	if (reset && !tipc_link_is_reset(l))
+	if (reset && l && !tipc_link_is_reset(l))
 		tipc_node_link_down(n, b->identity, false);
 	tipc_node_put(n);
 }

-- 
2.1.4

The netlink policy for TIPC_NLA_UDP_LOCAL and TIPC_NLA_UDP_REMOTE
is of type binary with a defined length. This causes the policy
framework to threat the defined length as maximum length.

There is however no protection against a user sending a smaller
amount of data. Prior to this patch this wasn't handled which could
result in a partially incomplete sockaddr_storage struct containing
uninitialized data.

In this patch we use nla_memcpy() when copying the user data. This
ensures a potential gap at the end is cleared out properly.

This was found by Julia with Coccinelle tool.

Reported-by: Daniel Borkmann <daniel@iogearbox.net>
Reported-by: Julia Lawall <redacted>
Signed-off-by: Richard Alpe <redacted>
Acked-by: Jon Maloy <redacted>
Reviewed-by: Erik Hugne <redacted>
---
 net/tipc/udp_media.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c
index f22a5bb1..6fe8740 100644
--- a/net/tipc/udp_media.c
+++ b/net/tipc/udp_media.c

@@ -276,7 +276,7 @@ static int parse_options(struct nlattr *attrs[], struct udp_bearer *ub,
 			 struct udp_media_addr *remote)
 {
 	struct nlattr *opts[TIPC_NLA_UDP_MAX + 1];
-	struct sockaddr_storage *sa_local, *sa_remote;
+	struct sockaddr_storage sa_local, sa_remote;
 
 	if (!attrs[TIPC_NLA_BEARER_UDP_OPTS])
 		goto err;

@@ -285,41 +285,43 @@ static int parse_options(struct nlattr *attrs[], struct udp_bearer *ub,
 			     tipc_nl_udp_policy))
 		goto err;
 	if (opts[TIPC_NLA_UDP_LOCAL] && opts[TIPC_NLA_UDP_REMOTE]) {
-		sa_local = nla_data(opts[TIPC_NLA_UDP_LOCAL]);
-		sa_remote = nla_data(opts[TIPC_NLA_UDP_REMOTE]);
+		nla_memcpy(&sa_local, opts[TIPC_NLA_UDP_LOCAL],
+			   sizeof(sa_local));
+		nla_memcpy(&sa_remote, opts[TIPC_NLA_UDP_REMOTE],
+			   sizeof(sa_remote));
 	} else {
 err:
 		pr_err("Invalid UDP bearer configuration");
 		return -EINVAL;
 	}
-	if ((sa_local->ss_family & sa_remote->ss_family) == AF_INET) {
+	if ((sa_local.ss_family & sa_remote.ss_family) == AF_INET) {
 		struct sockaddr_in *ip4;
 
-		ip4 = (struct sockaddr_in *)sa_local;
+		ip4 = (struct sockaddr_in *)&sa_local;
 		local->proto = htons(ETH_P_IP);
 		local->udp_port = ip4->sin_port;
 		local->ipv4.s_addr = ip4->sin_addr.s_addr;
 
-		ip4 = (struct sockaddr_in *)sa_remote;
+		ip4 = (struct sockaddr_in *)&sa_remote;
 		remote->proto = htons(ETH_P_IP);
 		remote->udp_port = ip4->sin_port;
 		remote->ipv4.s_addr = ip4->sin_addr.s_addr;
 		return 0;
 
 #if IS_ENABLED(CONFIG_IPV6)
-	} else if ((sa_local->ss_family & sa_remote->ss_family) == AF_INET6) {
+	} else if ((sa_local.ss_family & sa_remote.ss_family) == AF_INET6) {
 		struct sockaddr_in6 *ip6;
 
-		ip6 = (struct sockaddr_in6 *)sa_local;
+		ip6 = (struct sockaddr_in6 *)&sa_local;
 		local->proto = htons(ETH_P_IPV6);
 		local->udp_port = ip6->sin6_port;
-		local->ipv6 = ip6->sin6_addr;
+		memcpy(&local->ipv6, &ip6->sin6_addr, sizeof(struct in6_addr));
 		ub->ifindex = ip6->sin6_scope_id;
 
-		ip6 = (struct sockaddr_in6 *)sa_remote;
+		ip6 = (struct sockaddr_in6 *)&sa_remote;
 		remote->proto = htons(ETH_P_IPV6);
 		remote->udp_port = ip6->sin6_port;
-		remote->ipv6 = ip6->sin6_addr;
+		memcpy(&remote->ipv6, &ip6->sin6_addr, sizeof(struct in6_addr));
 		return 0;
 #endif
 	}

-- 
2.1.4

As I just saw, you had submitted the series to net-next. Anyway, this is a very good job!

Thanks,
Ying

-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On Behalf Of Richard Alpe
Sent: 2016年3月3日 21:21
To: netdev@vger.kernel.org
Cc: tipc-discussion@lists.sourceforge.net; Richard Alpe
Subject: [PATCH net-next v1 1/4] tipc: add net device to skb before UDP xmit

Prior to this patch enabling a IPv4 UDP bearer caused a null pointer dereference in iptunnel_xmit_stats(), when it tried to dereference the net device from the skb. To resolve this we now point the skb device to the net device resolved from the routing table.

Fixes: 039f50629b7f (ip_tunnel: Move stats update to iptunnel_xmit())
Signed-off-by: Richard Alpe <redacted>
Acked-by: Jon Maloy <redacted>
Reviewed-by: Erik Hugne <redacted>
---
 net/tipc/udp_media.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c index d63a911..f22a5bb1 100644
--- a/net/tipc/udp_media.c
+++ b/net/tipc/udp_media.c

@@ -181,6 +181,8 @@ static int tipc_udp_send_msg(struct net *net, struct sk_buff *skb,
 			err = PTR_ERR(rt);
 			goto tx_error;
 		}
+
+		skb->dev = rt->dst.dev;
 		ttl = ip4_dst_hoplimit(&rt->dst);
 		udp_tunnel_xmit_skb(rt, ub->ubsock->sk, skb, src->ipv4.s_addr,
 				    dst->ipv4.s_addr, 0, ttl, 0, src->udp_port,
--

2.1.4

From: Richard Alpe <redacted>
Date: Thu, 3 Mar 2016 14:20:40 +0100

Prior to this patch enabling a IPv4 UDP bearer caused a null pointer
dereference in iptunnel_xmit_stats(), when it tried to dereference the
net device from the skb. To resolve this we now point the skb device
to the net device resolved from the routing table.

Fixes: 039f50629b7f (ip_tunnel: Move stats update to iptunnel_xmit())
Signed-off-by: Richard Alpe <redacted>
Acked-by: Jon Maloy <redacted>
Reviewed-by: Erik Hugne <redacted>

Applied.

From: Richard Alpe <redacted>
Date: Thu, 3 Mar 2016 14:20:41 +0100

Make sure we have a link before checking if it has been reset or not.

Prior to this patch tipc_link_is_reset() could be called with a non
existing link, resulting in a null pointer dereference.

Signed-off-by: Richard Alpe <redacted>
Acked-by: Jon Maloy <redacted>
Reviewed-by: Erik Hugne <redacted>

Applied.

From: Richard Alpe <redacted>
Date: Thu, 3 Mar 2016 14:20:42 +0100

The netlink policy for TIPC_NLA_UDP_LOCAL and TIPC_NLA_UDP_REMOTE
is of type binary with a defined length. This causes the policy
framework to threat the defined length as maximum length.

There is however no protection against a user sending a smaller
amount of data. Prior to this patch this wasn't handled which could
result in a partially incomplete sockaddr_storage struct containing
uninitialized data.

In this patch we use nla_memcpy() when copying the user data. This
ensures a potential gap at the end is cleared out properly.

This was found by Julia with Coccinelle tool.

Reported-by: Daniel Borkmann <daniel@iogearbox.net>
Reported-by: Julia Lawall <redacted>
Signed-off-by: Richard Alpe <redacted>
Acked-by: Jon Maloy <redacted>
Reviewed-by: Erik Hugne <redacted>

Applied.

From: Richard Alpe <redacted>
Date: Thu, 3 Mar 2016 14:20:43 +0100

Make sure the user has provided a scope for multicast and link local
addresses used locally by a UDP bearer.

Signed-off-by: Richard Alpe <redacted>
Acked-by: Jon Maloy <redacted>
Reviewed-by: Erik Hugne <redacted>

Applied.