[PATCH net v2] tipc: check minimum bearer MTU

Qian Zhang (张谦) reported a potential socket buffer overflow in
tipc_msg_build() which is also known as CVE-2016-8632: due to
insufficient checks, a buffer overflow can occur if MTU is too short for
even tipc headers. As anyone can set device MTU in a user/net namespace,
this issue can be abused by a regular user.

As agreed in the discussion on Ben Hutchings' original patch, we should
check the MTU at the moment a bearer is attached rather than for each
processed packet. We also need to repeat the check when bearer MTU is
adjusted to new device MTU. UDP case also needs a check to avoid
overflow when calculating bearer MTU.

Fixes: b97bf3fd8f6a ("[TIPC] Initial merge")
Signed-off-by: Michal Kubecek <redacted>
Reported-by: Qian Zhang (张谦) <redacted>
---
changes v1 to v2:
- add missing "static" to tipc_check_mtu() helper declaration
- rather than blocking device MTU change to too low value, disable tipc
  bearer if that happens (suggested by Ben Hutchings)
---
 net/tipc/bearer.c    | 11 +++++++++--
 net/tipc/bearer.h    | 13 +++++++++++++
 net/tipc/udp_media.c |  5 +++++
 3 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index 975dbeb60ab0..1c004a46486f 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c

@@ -421,6 +421,10 @@ int tipc_enable_l2_media(struct net *net, struct tipc_bearer *b,
 	dev = dev_get_by_name(net, driver_name);
 	if (!dev)
 		return -ENODEV;
+	if (tipc_check_mtu(dev, 0)) {
+		dev_put(dev);
+		return -EINVAL;
+	}
 
 	/* Associate TIPC bearer with L2 bearer */
 	rcu_assign_pointer(b->media_ptr, dev);

@@ -610,8 +614,6 @@ static int tipc_l2_device_event(struct notifier_block *nb, unsigned long evt,
 	if (!b)
 		return NOTIFY_DONE;
 
-	b->mtu = dev->mtu;
-
 	switch (evt) {
 	case NETDEV_CHANGE:
 		if (netif_carrier_ok(dev))

@@ -624,6 +626,11 @@ static int tipc_l2_device_event(struct notifier_block *nb, unsigned long evt,
 		tipc_reset_bearer(net, b);
 		break;
 	case NETDEV_CHANGEMTU:
+		if (tipc_check_mtu(dev, 0)) {
+			bearer_disable(net, b);
+			break;
+		}
+		b->mtu = dev->mtu;
 		tipc_reset_bearer(net, b);
 		break;
 	case NETDEV_CHANGEADDR:

diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h
index 78892e2f53e3..6b8a1d9d313a 100644
--- a/net/tipc/bearer.h
+++ b/net/tipc/bearer.h

@@ -39,6 +39,7 @@
 
 #include "netlink.h"
 #include "core.h"
+#include "msg.h"
 #include <net/genetlink.h>
 
 #define MAX_MEDIA	3

@@ -59,6 +60,9 @@
 #define TIPC_MEDIA_TYPE_IB	2
 #define TIPC_MEDIA_TYPE_UDP	3
 
+/* minimum bearer MTU */
+#define TIPC_MIN_BEARER_MTU	(MAX_H_SIZE + INT_H_SIZE)
+
 /**
  * struct tipc_media_addr - destination address used by TIPC bearers
  * @value: address info (format defined by media)

@@ -215,4 +219,13 @@ void tipc_bearer_xmit(struct net *net, u32 bearer_id,
 void tipc_bearer_bc_xmit(struct net *net, u32 bearer_id,
 			 struct sk_buff_head *xmitq);
 
+/* check if device MTU is sufficient for tipc headers */
+static inline bool tipc_check_mtu(struct net_device *dev, unsigned int reserve)
+{
+	if (dev->mtu >= TIPC_MIN_BEARER_MTU + reserve)
+		return false;
+	netdev_warn(dev, "MTU too low for tipc bearer\n");
+	return true;
+}
+
 #endif	/* _TIPC_BEARER_H */

diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c
index 78cab9c5a445..376ed3e3ed46 100644
--- a/net/tipc/udp_media.c
+++ b/net/tipc/udp_media.c

@@ -697,6 +697,11 @@ static int tipc_udp_enable(struct net *net, struct tipc_bearer *b,
 		udp_conf.local_ip.s_addr = htonl(INADDR_ANY);
 		udp_conf.use_udp_checksums = false;
 		ub->ifindex = dev->ifindex;
+		if (tipc_check_mtu(dev, sizeof(struct iphdr) +
+					sizeof(struct udphdr))) {
+			err = -EINVAL;
+			goto err;
+		}
 		b->mtu = dev->mtu - sizeof(struct iphdr)
 			- sizeof(struct udphdr);
 #if IS_ENABLED(CONFIG_IPV6)

-- 
2.10.2

On Thu, 2016-12-01 at 12:02 +0100, Michal Kubecek wrote:
[...] 

+/* check if device MTU is sufficient for tipc headers */
+static inline bool tipc_check_mtu(struct net_device *dev, unsigned int reserve)
+{
+	if (dev->mtu >= TIPC_MIN_BEARER_MTU + reserve)
+		return false;
+	netdev_warn(dev, "MTU too low for tipc bearer\n");
+	return true;
+}

[...]

The comment says "check if ... sufficient" but the return value
indicates the opposite.  Could you make these consistent?

Other than that, this looks OK to me.  I haven't tested any version as
I don't know how to use TIPC.

Ben.

-- 
Ben Hutchings
A free society is one where it is safe to be unpopular. - Adlai
Stevenson

On Thu, Dec 01, 2016 at 04:11:18PM +0000, Ben Hutchings wrote:

On Thu, 2016-12-01 at 12:02 +0100, Michal Kubecek wrote:
[...] 

quoted

+/* check if device MTU is sufficient for tipc headers */
+static inline bool tipc_check_mtu(struct net_device *dev, unsigned int reserve)
+{
+	if (dev->mtu >= TIPC_MIN_BEARER_MTU + reserve)
+		return false;
+	netdev_warn(dev, "MTU too low for tipc bearer\n");
+	return true;
+}

[...]

The comment says "check if ... sufficient" but the return value
indicates the opposite.  Could you make these consistent?

Good point. I suppose renaming the function to e.g. tipc_mtu_bad() (and
rewording the commment accordingly) would also make the code more 
readable without looking at the definition.

I'll wait for other comments and send v3 tomorrow.

Other than that, this looks OK to me.  I haven't tested any version as
I don't know how to use TIPC.

I checked that the patch doesn't allow enabling a bearer on top of
device with insufficient MTU and it does for sufficient (100/128), both
in eth and udp case. I also checked that lowering MTU under 100 in eth
case disables attached bearer. I didn't run any deeper test like sending
an actual traffic but the patch shouldn't affect that.

Michal Kubecek