All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net] tipc: check minimum bearer MTU
@ 2016-11-30  9:57 Michal Kubecek
  2016-11-30 10:24 ` Michal Kubecek
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Michal Kubecek @ 2016-11-30  9:57 UTC (permalink / raw)
  To: Jon Maloy, Ying Xue
  Cc: David S. Miller, tipc-discussion, netdev, linux-kernel,
	Ben Hutchings, Qian Zhang

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 <mkubecek@suse.cz>
Reported-by: Qian Zhang (张谦) <zhangqian-c@360.cn>
---
 net/tipc/bearer.c    |  9 +++++++--
 net/tipc/bearer.h    | 13 +++++++++++++
 net/tipc/udp_media.c |  5 +++++
 3 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index 975dbeb60ab0..dd4b19e8bb43 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,9 @@ 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))
+			return -EINVAL;
+		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..1a0b7434ec24 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 */
+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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH net] tipc: check minimum bearer MTU
  2016-11-30  9:57 [PATCH net] tipc: check minimum bearer MTU Michal Kubecek
@ 2016-11-30 10:24 ` Michal Kubecek
  2016-12-01  0:05   ` Ben Hutchings
  2016-11-30 10:28   ` Ying Xue
  2016-12-01  7:21 ` kbuild test robot
  2 siblings, 1 reply; 9+ messages in thread
From: Michal Kubecek @ 2016-11-30 10:24 UTC (permalink / raw)
  To: Jon Maloy, Ying Xue
  Cc: David S. Miller, tipc-discussion, netdev, linux-kernel,
	Ben Hutchings, Qian Zhang

On Wed, Nov 30, 2016 at 10:57:02AM +0100, Michal Kubecek wrote:
> 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 <mkubecek@suse.cz>
> Reported-by: Qian Zhang (张谦) <zhangqian-c@360.cn>

Self-NACK.

Im sorry, while testing this, I overlooked that an attempt to change
MTU of an underlying device to low value issues a warning but it
succeeds anyway.

> @@ -624,6 +626,9 @@ 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))
> +			return -EINVAL;
> +		b->mtu = dev->mtu;
>  		tipc_reset_bearer(net, b);
>  		break;
>  	case NETDEV_CHANGEADDR:

This is a notifier so that error value needs to be encoded into notifier
error. I'll send v2 after retesting

Michal Kubecek

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net] tipc: check minimum bearer MTU
  2016-11-30  9:57 [PATCH net] tipc: check minimum bearer MTU Michal Kubecek
@ 2016-11-30 10:28   ` Ying Xue
  2016-11-30 10:28   ` Ying Xue
  2016-12-01  7:21 ` kbuild test robot
  2 siblings, 0 replies; 9+ messages in thread
From: Ying Xue @ 2016-11-30 10:28 UTC (permalink / raw)
  To: Michal Kubecek, Jon Maloy
  Cc: David S. Miller, tipc-discussion, netdev, linux-kernel,
	Ben Hutchings, Qian Zhang

On 11/30/2016 05:57 PM, Michal Kubecek wrote:
> 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 <mkubecek@suse.cz>
> Reported-by: Qian Zhang (张谦) <zhangqian-c@360.cn>
> ---
>  net/tipc/bearer.c    |  9 +++++++--
>  net/tipc/bearer.h    | 13 +++++++++++++
>  net/tipc/udp_media.c |  5 +++++
>  3 files changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
> index 975dbeb60ab0..dd4b19e8bb43 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,9 @@ 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))
> +			return -EINVAL;
> +		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..1a0b7434ec24 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 */
> +inline bool tipc_check_mtu(struct net_device *dev, unsigned int reserve)

It's unnecessary to explicitly declare a function as inline, instead 
please let GCC smartly decide this.

> +{
> +	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;
> +		}

For UDP bearer, it seems insufficient for us to check MTU size only when 
UDP bearer is enabled. Meanwhile, we should update MTU size for UDP 
bearer with Path MTU discovery protocol once MTU size is changed after 
bearer is enabled.

Regards,
Ying

>  		b->mtu = dev->mtu - sizeof(struct iphdr)
>  			- sizeof(struct udphdr);
>  #if IS_ENABLED(CONFIG_IPV6)
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net] tipc: check minimum bearer MTU
@ 2016-11-30 10:28   ` Ying Xue
  0 siblings, 0 replies; 9+ messages in thread
From: Ying Xue @ 2016-11-30 10:28 UTC (permalink / raw)
  To: Michal Kubecek, Jon Maloy
  Cc: Qian, netdev, Zhang, linux-kernel, tipc-discussion,
	Ben Hutchings, David S. Miller

On 11/30/2016 05:57 PM, Michal Kubecek wrote:
> 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 <mkubecek@suse.cz>
> Reported-by: Qian Zhang (张谦) <zhangqian-c@360.cn>
> ---
>  net/tipc/bearer.c    |  9 +++++++--
>  net/tipc/bearer.h    | 13 +++++++++++++
>  net/tipc/udp_media.c |  5 +++++
>  3 files changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
> index 975dbeb60ab0..dd4b19e8bb43 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,9 @@ 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))
> +			return -EINVAL;
> +		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..1a0b7434ec24 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 */
> +inline bool tipc_check_mtu(struct net_device *dev, unsigned int reserve)

It's unnecessary to explicitly declare a function as inline, instead 
please let GCC smartly decide this.

> +{
> +	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;
> +		}

For UDP bearer, it seems insufficient for us to check MTU size only when 
UDP bearer is enabled. Meanwhile, we should update MTU size for UDP 
bearer with Path MTU discovery protocol once MTU size is changed after 
bearer is enabled.

Regards,
Ying

>  		b->mtu = dev->mtu - sizeof(struct iphdr)
>  			- sizeof(struct udphdr);
>  #if IS_ENABLED(CONFIG_IPV6)
>


------------------------------------------------------------------------------
_______________________________________________
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net] tipc: check minimum bearer MTU
  2016-11-30 10:28   ` Ying Xue
  (?)
@ 2016-11-30 13:23   ` Michal Kubecek
  2016-12-01 10:07       ` Ying Xue
  -1 siblings, 1 reply; 9+ messages in thread
From: Michal Kubecek @ 2016-11-30 13:23 UTC (permalink / raw)
  To: Ying Xue
  Cc: Jon Maloy, David S. Miller, tipc-discussion, netdev,
	linux-kernel, Ben Hutchings, Qian Zhang

On Wed, Nov 30, 2016 at 06:28:14PM +0800, Ying Xue wrote:
...
> >diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h
> >index 78892e2f53e3..1a0b7434ec24 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 */
> >+inline bool tipc_check_mtu(struct net_device *dev, unsigned int reserve)
> 
> It's unnecessary to explicitly declare a function as inline, instead
> please let GCC smartly decide this.

This is a header file. But I just noticed the last change (adding
missing "static" keyword) is missing in the version I sent.

> 
> >+{
> >+	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;
> >+		}
> 
> For UDP bearer, it seems insufficient for us to check MTU size only
> when UDP bearer is enabled. Meanwhile, we should update MTU size for
> UDP bearer with Path MTU discovery protocol once MTU size is changed
> after bearer is enabled.

I should admit I'm not that familiar with tipc. Do you mean updating
b->mtu in response to PMTU updates of the route used for ub->ubsock?
The way I understand it, it would be certainly useful but it's not
directly related to the security issue this patch addresses as if there
are no updates, b->mtu cannot get too low and there is no risk of
a buffer overflow. In other words, reflecting PMTU updates is something
that can be IMHO left for later.

                                                      Michal Kubecek

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net] tipc: check minimum bearer MTU
  2016-11-30 10:24 ` Michal Kubecek
@ 2016-12-01  0:05   ` Ben Hutchings
  0 siblings, 0 replies; 9+ messages in thread
From: Ben Hutchings @ 2016-12-01  0:05 UTC (permalink / raw)
  To: Michal Kubecek, Jon Maloy, Ying Xue
  Cc: David S. Miller, tipc-discussion, netdev, linux-kernel, Qian Zhang

[-- Attachment #1: Type: text/plain, Size: 1488 bytes --]

On Wed, 2016-11-30 at 11:24 +0100, Michal Kubecek wrote:
> On Wed, Nov 30, 2016 at 10:57:02AM +0100, Michal Kubecek wrote:
> > 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 <mkubecek@suse.cz>
> > Reported-by: Qian Zhang (张谦) <zhangqian-c@360.cn>
> 
> Self-NACK.
> 
> Im sorry, while testing this, I overlooked that an attempt to change
> MTU of an underlying device to low value issues a warning but it
> succeeds anyway.
[...]

I'm not sure that TIPC should block the MTU change, anyway.  For IPv4
and IPv6 we disable the protocol on a device if its MTU is reduced
below the minimum.  I think TIPC should behave the same way.

Ben.

-- 
Ben Hutchings
Never attribute to conspiracy what can adequately be explained by
stupidity.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net] tipc: check minimum bearer MTU
  2016-11-30  9:57 [PATCH net] tipc: check minimum bearer MTU Michal Kubecek
  2016-11-30 10:24 ` Michal Kubecek
  2016-11-30 10:28   ` Ying Xue
@ 2016-12-01  7:21 ` kbuild test robot
  2 siblings, 0 replies; 9+ messages in thread
From: kbuild test robot @ 2016-12-01  7:21 UTC (permalink / raw)
  To: Michal Kubecek
  Cc: kbuild-all, Jon Maloy, Ying Xue, David S. Miller,
	tipc-discussion, netdev, linux-kernel, Ben Hutchings, Qian Zhang

[-- Attachment #1: Type: text/plain, Size: 3127 bytes --]

Hi Michal,

[auto build test ERROR on net/master]

url:    https://github.com/0day-ci/linux/commits/Michal-Kubecek/tipc-check-minimum-bearer-MTU/20161201-140555
config: i386-randconfig-s0-201648 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   net/tipc/bearer.o: In function `tipc_check_mtu':
>> bearer.c:(.text+0xadd): multiple definition of `tipc_check_mtu'
   net/tipc/bcast.o:bcast.c:(.text+0x598): first defined here
   net/tipc/core.o: In function `tipc_check_mtu':
   core.c:(.text+0x239): multiple definition of `tipc_check_mtu'
   net/tipc/bcast.o:bcast.c:(.text+0x598): first defined here
   net/tipc/link.o: In function `tipc_check_mtu':
   link.c:(.text+0x110e): multiple definition of `tipc_check_mtu'
   net/tipc/bcast.o:bcast.c:(.text+0x598): first defined here
   net/tipc/discover.o: In function `tipc_check_mtu':
   discover.c:(.text+0x2c7): multiple definition of `tipc_check_mtu'
   net/tipc/bcast.o:bcast.c:(.text+0x598): first defined here
   net/tipc/name_distr.o: In function `tipc_check_mtu':
   name_distr.c:(.text+0x536): multiple definition of `tipc_check_mtu'
   net/tipc/bcast.o:bcast.c:(.text+0x598): first defined here
   net/tipc/monitor.o: In function `tipc_check_mtu':
   monitor.c:(.text+0x906): multiple definition of `tipc_check_mtu'
   net/tipc/bcast.o:bcast.c:(.text+0x598): first defined here
   net/tipc/name_table.o: In function `tipc_check_mtu':
   name_table.c:(.text+0x875): multiple definition of `tipc_check_mtu'
   net/tipc/bcast.o:bcast.c:(.text+0x598): first defined here
   net/tipc/net.o: In function `tipc_check_mtu':
   net.c:(.text+0xc4): multiple definition of `tipc_check_mtu'
   net/tipc/bcast.o:bcast.c:(.text+0x598): first defined here
   net/tipc/netlink.o: In function `tipc_check_mtu':
   netlink.c:(.text+0x0): multiple definition of `tipc_check_mtu'
   net/tipc/bcast.o:bcast.c:(.text+0x598): first defined here
   net/tipc/netlink_compat.o: In function `tipc_check_mtu':
   netlink_compat.c:(.text+0x2a9e): multiple definition of `tipc_check_mtu'
   net/tipc/bcast.o:bcast.c:(.text+0x598): first defined here
   net/tipc/node.o: In function `tipc_check_mtu':
   node.c:(.text+0x1dd8): multiple definition of `tipc_check_mtu'
   net/tipc/bcast.o:bcast.c:(.text+0x598): first defined here
   net/tipc/socket.o: In function `tipc_check_mtu':
   socket.c:(.text+0x62bb): multiple definition of `tipc_check_mtu'
   net/tipc/bcast.o:bcast.c:(.text+0x598): first defined here
   net/tipc/eth_media.o: In function `tipc_check_mtu':
   eth_media.c:(.text+0x117): multiple definition of `tipc_check_mtu'
   net/tipc/bcast.o:bcast.c:(.text+0x598): first defined here
   net/tipc/udp_media.o: In function `tipc_check_mtu':
   udp_media.c:(.text+0x10b9): multiple definition of `tipc_check_mtu'
   net/tipc/bcast.o:bcast.c:(.text+0x598): first defined here

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 25827 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net] tipc: check minimum bearer MTU
  2016-11-30 13:23   ` Michal Kubecek
@ 2016-12-01 10:07       ` Ying Xue
  0 siblings, 0 replies; 9+ messages in thread
From: Ying Xue @ 2016-12-01 10:07 UTC (permalink / raw)
  To: Michal Kubecek
  Cc: Jon Maloy, David S. Miller, tipc-discussion, netdev,
	linux-kernel, Ben Hutchings, Qian Zhang

>>
>> For UDP bearer, it seems insufficient for us to check MTU size only
>> when UDP bearer is enabled. Meanwhile, we should update MTU size for
>> UDP bearer with Path MTU discovery protocol once MTU size is changed
>> after bearer is enabled.
>
> I should admit I'm not that familiar with tipc. Do you mean updating
> b->mtu in response to PMTU updates of the route used for ub->ubsock?

Yes.

> The way I understand it, it would be certainly useful but it's not
> directly related to the security issue this patch addresses as if there
> are no updates, b->mtu cannot get too low and there is no risk of
> a buffer overflow. In other words, reflecting PMTU updates is something
> that can be IMHO left for later.
>

Agreed.

Regards,
Ying

>                                                       Michal Kubecek
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net] tipc: check minimum bearer MTU
@ 2016-12-01 10:07       ` Ying Xue
  0 siblings, 0 replies; 9+ messages in thread
From: Ying Xue @ 2016-12-01 10:07 UTC (permalink / raw)
  To: Michal Kubecek
  Cc: Jon Maloy, Qian Zhang, netdev, linux-kernel, tipc-discussion,
	Ben Hutchings, David S. Miller

>>
>> For UDP bearer, it seems insufficient for us to check MTU size only
>> when UDP bearer is enabled. Meanwhile, we should update MTU size for
>> UDP bearer with Path MTU discovery protocol once MTU size is changed
>> after bearer is enabled.
>
> I should admit I'm not that familiar with tipc. Do you mean updating
> b->mtu in response to PMTU updates of the route used for ub->ubsock?

Yes.

> The way I understand it, it would be certainly useful but it's not
> directly related to the security issue this patch addresses as if there
> are no updates, b->mtu cannot get too low and there is no risk of
> a buffer overflow. In other words, reflecting PMTU updates is something
> that can be IMHO left for later.
>

Agreed.

Regards,
Ying

>                                                       Michal Kubecek
>


------------------------------------------------------------------------------

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2016-12-01 10:08 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-30  9:57 [PATCH net] tipc: check minimum bearer MTU Michal Kubecek
2016-11-30 10:24 ` Michal Kubecek
2016-12-01  0:05   ` Ben Hutchings
2016-11-30 10:28 ` Ying Xue
2016-11-30 10:28   ` Ying Xue
2016-11-30 13:23   ` Michal Kubecek
2016-12-01 10:07     ` Ying Xue
2016-12-01 10:07       ` Ying Xue
2016-12-01  7:21 ` kbuild test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.