tipc-discussion — User questions & problem reports, project news, developer discussion

Re: [tipc-discussion] [PATCH] tipc: remove dead code in struct tipc_topsrv

[Ying X. <yin...@wi...>]

On 1/24/19 10:06 AM, Zhaolong Zhang wrote:
> max_rcvbuf_size is no longer used since commit "414574a0af36".
> 
> Signed-off-by: Zhaolong Zhang <zha...@12...>

Acked-by: Ying Xue <yin...@wi...>

> ---
>  net/tipc/topsrv.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
> index efb16f6..4b1c083 100644
> --- a/net/tipc/topsrv.c
> +++ b/net/tipc/topsrv.c
> @@ -60,7 +60,6 @@
>   * @awork: accept work item
>   * @rcv_wq: receive workqueue
>   * @send_wq: send workqueue
> - * @max_rcvbuf_size: maximum permitted receive message length
>   * @listener: topsrv listener socket
>   * @name: server name
>   */
> @@ -72,7 +71,6 @@ struct tipc_topsrv {
>  	struct work_struct awork;
>  	struct workqueue_struct *rcv_wq;
>  	struct workqueue_struct *send_wq;
> -	int max_rcvbuf_size;
>  	struct socket *listener;
>  	char name[TIPC_SERVER_NAME_LEN];
>  };
> @@ -648,7 +646,6 @@ int tipc_topsrv_start(struct net *net)
>  		return -ENOMEM;
>  
>  	srv->net = net;
> -	srv->max_rcvbuf_size = sizeof(struct tipc_subscr);
>  	INIT_WORK(&srv->awork, tipc_topsrv_accept);
>  
>  	strscpy(srv->name, name, sizeof(srv->name));
> 

Re: [tipc-discussion] [PATCH net-next] tipc: mark expected switch fall-throughs

[David M. <da...@da...>]

From: "Gustavo A. R. Silva" <gu...@em...>
Date: Wed, 23 Jan 2019 01:09:31 -0600

> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.
> 
> This patch fixes the following warnings:
> 
> net/tipc/link.c:1125:6: warning: this statement may fall through [-Wimplicit-fallthrough=]
> net/tipc/socket.c:736:6: warning: this statement may fall through [-Wimplicit-fallthrough=]
> net/tipc/socket.c:2418:7: warning: this statement may fall through [-Wimplicit-fallthrough=]
> 
> Warning level 3 was used: -Wimplicit-fallthrough=3
> 
> This patch is part of the ongoing efforts to enabling
> -Wimplicit-fallthrough.
> 
> Signed-off-by: Gustavo A. R. Silva <gu...@em...>

Applied.

[tipc-discussion] [PATCH] tipc: smooth change between replicast and broadcast

[Hoang Le <hoa...@de...>]

We need some way to secure a smooth change from TIPC replicast to
broadcast and vice versa.

Currently, a multicast stream may start out using replicast, because
there are few destinations, and then it should ideally switch to
L2/broadcast IGMP/multicast when the number of destinations grows beyond
a certain limit. The opposite should happen when the number decreases
below the limit.

Currently, to guarantee sequence order, we don’t allow such a switch to
happen unless there is a 5 seconds pause in the traffic from the sender
socket. That means that is a sender never takes such a pause, he will
forever be stuck with the method he started out with.

Signed-off-by: Hoang Le <hoa...@de...>
---
 net/tipc/bcast.c  | 116 +++++++++++++++++++++++++++++++++++++++++++++-
 net/tipc/bcast.h  |   5 ++
 net/tipc/core.c   |   2 +
 net/tipc/core.h   |   3 ++
 net/tipc/msg.h    |  10 ++++
 net/tipc/node.c   |  10 ++++
 net/tipc/node.h   |   6 ++-
 net/tipc/socket.c |  10 ++++
 8 files changed, 159 insertions(+), 3 deletions(-)

diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c
index d8026543bf4c..3f9015b1b6bc 100644
--- a/net/tipc/bcast.c
+++ b/net/tipc/bcast.c
@@ -295,11 +295,15 @@ int tipc_mcast_xmit(struct net *net, struct sk_buff_head *pkts,
 		    struct tipc_mc_method *method, struct tipc_nlist *dests,
 		    u16 *cong_link_cnt)
 {
-	struct sk_buff_head inputq, localq;
+	struct sk_buff_head inputq, localq, tmpq;
+	bool rcast = method->rcast;
+	struct sk_buff *skb, *_skb;
+	struct tipc_msg *hdr, *_hdr;
 	int rc = 0;
 
 	skb_queue_head_init(&inputq);
 	skb_queue_head_init(&localq);
+	skb_queue_head_init(&tmpq);
 
 	/* Clone packets before they are consumed by next call */
 	if (dests->local && !tipc_msg_reassemble(pkts, &localq)) {
@@ -309,6 +313,53 @@ int tipc_mcast_xmit(struct net *net, struct sk_buff_head *pkts,
 	/* Send according to determined transmit method */
 	if (dests->remote) {
 		tipc_bcast_select_xmit_method(net, dests->remote, method);
+
+		if (tipc_net(net)->capabilities & TIPC_MCAST_RBCTL) {
+			skb = skb_peek(pkts);
+			hdr = buf_msg(skb);
+
+			if (msg_user(hdr) == MSG_FRAGMENTER)
+				hdr = msg_get_wrapped(hdr);
+			if (msg_type(hdr) != TIPC_MCAST_MSG)
+				goto xmit;
+
+			msg_set_syn(hdr, 0);
+			msg_set_is_rcast(hdr, method->rcast);
+
+			/* switch mode */
+			if (rcast != method->rcast) {
+				/* Build message's copied */
+				_skb = tipc_buf_acquire(MCAST_H_SIZE,
+							GFP_KERNEL);
+				if (!skb) {
+					rc = -ENOMEM;
+					goto exit;
+				}
+				skb_orphan(_skb);
+				skb_copy_to_linear_data(_skb, hdr,
+							MCAST_H_SIZE);
+
+				/* Build dummy header */
+				_hdr = buf_msg(_skb);
+				msg_set_size(_hdr, MCAST_H_SIZE);
+				__skb_queue_tail(&tmpq, _skb);
+
+				msg_set_syn(hdr, 1);
+				msg_set_syn(_hdr, 1);
+				msg_set_is_rcast(hdr, rcast);
+				/* Prepare for 'synching' */
+				if (rcast)
+					tipc_rcast_xmit(net, &tmpq, dests,
+							cong_link_cnt);
+				else
+					tipc_bcast_xmit(net, &tmpq,
+							cong_link_cnt);
+
+				/* This queue should normally be empty by now */
+				__skb_queue_purge(&tmpq);
+			}
+		}
+xmit:
 		if (method->rcast)
 			rc = tipc_rcast_xmit(net, pkts, dests, cong_link_cnt);
 		else
@@ -576,3 +627,66 @@ void tipc_nlist_purge(struct tipc_nlist *nl)
 	nl->remote = 0;
 	nl->local = false;
 }
+
+void tipc_mcast_filter_msg(struct sk_buff_head *defq,
+			   struct sk_buff_head *inputq)
+{
+	struct sk_buff *skb, *_skb;
+	struct tipc_msg *hdr, *_hdr;
+	u32 node, port, _node, _port;
+	bool match = false;
+
+	skb = __skb_dequeue(inputq);
+	if (!skb)
+		return;
+
+	hdr = buf_msg(skb);
+	node =  msg_orignode(hdr);
+	port = msg_origport(hdr);
+
+	/* Find a peer port if its existing in defer queue */
+	while ((_skb = skb_peek(defq))) {
+		_hdr = buf_msg(_skb);
+		_node = msg_orignode(_hdr);
+		_port = msg_origport(_hdr);
+
+		if (_node != node)
+			continue;
+		if (_port != port)
+			continue;
+
+		if (!match) {
+			if (msg_is_syn(hdr) &&
+			    msg_is_rcast(hdr) != msg_is_rcast(_hdr)) {
+				__skb_dequeue(defq);
+				if (msg_data_sz(hdr)) {
+					__skb_queue_tail(inputq, skb);
+					kfree_skb(_skb);
+				} else {
+					__skb_queue_tail(inputq, _skb);
+					kfree_skb(skb);
+				}
+				match = true;
+			} else {
+				break;
+			}
+		} else {
+			if (msg_is_syn(_hdr))
+				return;
+			/* Dequeued to receive buffer */
+			__skb_dequeue(defq);
+			__skb_queue_tail(inputq, _skb);
+		}
+	}
+
+	if (match)
+		return;
+
+	if (msg_is_syn(hdr)) {
+		/* Enqueue and defer to next synching */
+		__skb_queue_tail(defq, skb);
+	} else {
+		/* Direct enqueued */
+		__skb_queue_tail(inputq, skb);
+	}
+}
diff --git a/net/tipc/bcast.h b/net/tipc/bcast.h
index 751530ab0c49..165d88a503e4 100644
--- a/net/tipc/bcast.h
+++ b/net/tipc/bcast.h
@@ -63,11 +63,13 @@ void tipc_nlist_del(struct tipc_nlist *nl, u32 node);
 /* Cookie to be used between socket and broadcast layer
  * @rcast: replicast (instead of broadcast) was used at previous xmit
  * @mandatory: broadcast/replicast indication was set by user
+ * @deferredq: defer queue to make message in order
  * @expires: re-evaluate non-mandatory transmit method if we are past this
  */
 struct tipc_mc_method {
 	bool rcast;
 	bool mandatory;
+	struct sk_buff_head deferredq;
 	unsigned long expires;
 };
 
@@ -92,6 +94,9 @@ int tipc_nl_add_bc_link(struct net *net, struct tipc_nl_msg *msg);
 int tipc_nl_bc_link_set(struct net *net, struct nlattr *attrs[]);
 int tipc_bclink_reset_stats(struct net *net);
 
+void tipc_mcast_filter_msg(struct sk_buff_head *defq,
+			   struct sk_buff_head *inputq);
+
 static inline void tipc_bcast_lock(struct net *net)
 {
 	spin_lock_bh(&tipc_net(net)->bclock);
diff --git a/net/tipc/core.c b/net/tipc/core.c
index 5b38f5164281..27cccd101ef6 100644
--- a/net/tipc/core.c
+++ b/net/tipc/core.c
@@ -43,6 +43,7 @@
 #include "net.h"
 #include "socket.h"
 #include "bcast.h"
+#include "node.h"
 
 #include <linux/module.h>
 
@@ -59,6 +60,7 @@ static int __net_init tipc_init_net(struct net *net)
 	tn->node_addr = 0;
 	tn->trial_addr = 0;
 	tn->addr_trial_end = 0;
+	tn->capabilities = TIPC_NODE_CAPABILITIES;
 	memset(tn->node_id, 0, sizeof(tn->node_id));
 	memset(tn->node_id_string, 0, sizeof(tn->node_id_string));
 	tn->mon_threshold = TIPC_DEF_MON_THRESHOLD;
diff --git a/net/tipc/core.h b/net/tipc/core.h
index 8020a6c360ff..7a68e1b6a066 100644
--- a/net/tipc/core.h
+++ b/net/tipc/core.h
@@ -122,6 +122,9 @@ struct tipc_net {
 	/* Topology subscription server */
 	struct tipc_topsrv *topsrv;
 	atomic_t subscription_count;
+
+	/* Cluster capabilities */
+	u16 capabilities;
 };
 
 static inline struct tipc_net *tipc_net(struct net *net)
diff --git a/net/tipc/msg.h b/net/tipc/msg.h
index a0924956bb61..70ddff2206a0 100644
--- a/net/tipc/msg.h
+++ b/net/tipc/msg.h
@@ -257,6 +257,16 @@ static inline void msg_set_src_droppable(struct tipc_msg *m, u32 d)
 	msg_set_bits(m, 0, 18, 1, d);
 }
 
+static inline bool msg_is_rcast(struct tipc_msg *m)
+{
+	return msg_bits(m, 0, 18, 0x1);
+}
+
+static inline void msg_set_is_rcast(struct tipc_msg *m, bool d)
+{
+	msg_set_bits(m, 0, 18, 0x1, d);
+}
+
 static inline void msg_set_size(struct tipc_msg *m, u32 sz)
 {
 	m->hdr[0] = htonl((msg_word(m, 0) & ~0x1ffff) | sz);
diff --git a/net/tipc/node.c b/net/tipc/node.c
index db2a6c3e0be9..1386e44d965c 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -383,6 +383,11 @@ static struct tipc_node *tipc_node_create(struct net *net, u32 addr,
 				tipc_link_update_caps(l, capabilities);
 		}
 		write_unlock_bh(&n->lock);
+		/* Calculate cluster capabilities */
+		tn->capabilities = TIPC_NODE_CAPABILITIES;
+		list_for_each_entry_rcu(temp_node, &tn->node_list, list) {
+			tn->capabilities &= temp_node->capabilities;
+		}
 		goto exit;
 	}
 	n = kzalloc(sizeof(*n), GFP_ATOMIC);
@@ -433,6 +438,11 @@ static struct tipc_node *tipc_node_create(struct net *net, u32 addr,
 			break;
 	}
 	list_add_tail_rcu(&n->list, &temp_node->list);
+	/* Calculate cluster capabilities */
+	tn->capabilities = TIPC_NODE_CAPABILITIES;
+	list_for_each_entry_rcu(temp_node, &tn->node_list, list) {
+		tn->capabilities &= temp_node->capabilities;
+	}
 	trace_tipc_node_create(n, true, " ");
 exit:
 	spin_unlock_bh(&tn->node_list_lock);
diff --git a/net/tipc/node.h b/net/tipc/node.h
index 4f59a30e989a..2404225c5d58 100644
--- a/net/tipc/node.h
+++ b/net/tipc/node.h
@@ -51,7 +51,8 @@ enum {
 	TIPC_BLOCK_FLOWCTL    = (1 << 3),
 	TIPC_BCAST_RCAST      = (1 << 4),
 	TIPC_NODE_ID128       = (1 << 5),
-	TIPC_LINK_PROTO_SEQNO = (1 << 6)
+	TIPC_LINK_PROTO_SEQNO = (1 << 6),
+	TIPC_MCAST_RBCTL      = (1 << 7)
 };
 
 #define TIPC_NODE_CAPABILITIES (TIPC_SYN_BIT           |  \
@@ -60,7 +61,8 @@ enum {
 				TIPC_BCAST_RCAST       |   \
 				TIPC_BLOCK_FLOWCTL     |   \
 				TIPC_NODE_ID128        |   \
-				TIPC_LINK_PROTO_SEQNO)
+				TIPC_LINK_PROTO_SEQNO  |   \
+				TIPC_MCAST_RBCTL)
 #define INVALID_BEARER_ID -1
 
 void tipc_node_stop(struct net *net);
diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 1217c90a363b..f8d8fa7216b9 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -483,6 +483,7 @@ static int tipc_sk_create(struct net *net, struct socket *sock,
 		tsk_set_unreturnable(tsk, true);
 		if (sock->type == SOCK_DGRAM)
 			tsk_set_unreliable(tsk, true);
+		__skb_queue_head_init(&tsk->mc_method.deferredq);
 	}
 
 	trace_tipc_sk_create(sk, NULL, TIPC_DUMP_NONE, " ");
@@ -580,6 +581,7 @@ static int tipc_release(struct socket *sock)
 	sk->sk_shutdown = SHUTDOWN_MASK;
 	tipc_sk_leave(tsk);
 	tipc_sk_withdraw(tsk, 0, NULL);
+	__skb_queue_purge(&tsk->mc_method.deferredq);
 	sk_stop_timer(sk, &sk->sk_timer);
 	tipc_sk_remove(tsk);
 
@@ -817,6 +819,11 @@ static int tipc_sendmcast(struct  socket *sock, struct tipc_name_seq *seq,
 				     &tsk->cong_link_cnt);
 	}
 
+	/* Update broadcast sequence number */
+	if (rc == 0) {
+		method->mandatory = false;
+		method->expires = jiffies;
+	}
 	tipc_nlist_purge(&dsts);
 
 	return rc ? rc : dlen;
@@ -2157,6 +2164,9 @@ static void tipc_sk_filter_rcv(struct sock *sk, struct sk_buff *skb,
 	if (unlikely(grp))
 		tipc_group_filter_msg(grp, &inputq, xmitq);
 
+	if (msg_type(hdr) == TIPC_MCAST_MSG)
+		tipc_mcast_filter_msg(&tsk->mc_method.deferredq, &inputq);
+
 	/* Validate and add to receive buffer if there is space */
 	while ((skb = __skb_dequeue(&inputq))) {
 		hdr = buf_msg(skb);
-- 
2.17.1

Re: [tipc-discussion] [PATCH net-next] tipc: remove unneeded semicolon in trace.c

[David M. <da...@da...>]

From: YueHaibing <yue...@hu...>
Date: Thu, 17 Jan 2019 20:57:08 +0800

> Remove unneeded semicolon
> 
> Signed-off-by: YueHaibing <yue...@hu...>

Applied.

Re: [tipc-discussion] [net 0/6] tipc: fix uninit-value issues reported by syzbot

[David M. <da...@da...>]

From: Ying Xue <yin...@wi...>
Date: Mon, 14 Jan 2019 17:22:23 +0800

> Recently, syzbot complained that TIPC module exits several issues
> associated with uninit-value type. So, in this series, we try to
> fix them as many as possible.

Series applied, thanks Ying.

[tipc-discussion] [net 5/6] tipc: fix uninit-value in tipc_nl_compat_name_table_dump

[Ying X. <yin...@wi...>]

syzbot reported:

BUG: KMSAN: uninit-value in __arch_swab32 arch/x86/include/uapi/asm/swab.h:10 [inline]
BUG: KMSAN: uninit-value in __fswab32 include/uapi/linux/swab.h:59 [inline]
BUG: KMSAN: uninit-value in tipc_nl_compat_name_table_dump+0x4a8/0xba0 net/tipc/netlink_compat.c:826
CPU: 0 PID: 6290 Comm: syz-executor848 Not tainted 4.19.0-rc8+ #70
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x306/0x460 lib/dump_stack.c:113
 kmsan_report+0x1a2/0x2e0 mm/kmsan/kmsan.c:917
 __msan_warning+0x7c/0xe0 mm/kmsan/kmsan_instr.c:500
 __arch_swab32 arch/x86/include/uapi/asm/swab.h:10 [inline]
 __fswab32 include/uapi/linux/swab.h:59 [inline]
 tipc_nl_compat_name_table_dump+0x4a8/0xba0 net/tipc/netlink_compat.c:826
 __tipc_nl_compat_dumpit+0x59e/0xdb0 net/tipc/netlink_compat.c:205
 tipc_nl_compat_dumpit+0x63a/0x820 net/tipc/netlink_compat.c:270
 tipc_nl_compat_handle net/tipc/netlink_compat.c:1151 [inline]
 tipc_nl_compat_recv+0x1402/0x2760 net/tipc/netlink_compat.c:1210
 genl_family_rcv_msg net/netlink/genetlink.c:601 [inline]
 genl_rcv_msg+0x185c/0x1a20 net/netlink/genetlink.c:626
 netlink_rcv_skb+0x394/0x640 net/netlink/af_netlink.c:2454
 genl_rcv+0x63/0x80 net/netlink/genetlink.c:637
 netlink_unicast_kernel net/netlink/af_netlink.c:1317 [inline]
 netlink_unicast+0x166d/0x1720 net/netlink/af_netlink.c:1343
 netlink_sendmsg+0x1391/0x1420 net/netlink/af_netlink.c:1908
 sock_sendmsg_nosec net/socket.c:621 [inline]
 sock_sendmsg net/socket.c:631 [inline]
 ___sys_sendmsg+0xe47/0x1200 net/socket.c:2116
 __sys_sendmsg net/socket.c:2154 [inline]
 __do_sys_sendmsg net/socket.c:2163 [inline]
 __se_sys_sendmsg+0x307/0x460 net/socket.c:2161
 __x64_sys_sendmsg+0x4a/0x70 net/socket.c:2161
 do_syscall_64+0xbe/0x100 arch/x86/entry/common.c:291
 entry_SYSCALL_64_after_hwframe+0x63/0xe7
RIP: 0033:0x440179
Code: 18 89 d0 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 fb 13 fc ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007ffecec49318 EFLAGS: 00000213 ORIG_RAX: 000000000000002e
RAX: ffffffffffffffda RBX: 00000000004002c8 RCX: 0000000000440179
RDX: 0000000000000000 RSI: 0000000020000100 RDI: 0000000000000003
RBP: 00000000006ca018 R08: 0000000000000000 R09: 00000000004002c8
R10: 0000000000000000 R11: 0000000000000213 R12: 0000000000401a00
R13: 0000000000401a90 R14: 0000000000000000 R15: 0000000000000000

Uninit was created at:
 kmsan_save_stack_with_flags mm/kmsan/kmsan.c:255 [inline]
 kmsan_internal_poison_shadow+0xc8/0x1d0 mm/kmsan/kmsan.c:180
 kmsan_kmalloc+0xa4/0x120 mm/kmsan/kmsan_hooks.c:104
 kmsan_slab_alloc+0x10/0x20 mm/kmsan/kmsan_hooks.c:113
 slab_post_alloc_hook mm/slab.h:446 [inline]
 slab_alloc_node mm/slub.c:2727 [inline]
 __kmalloc_node_track_caller+0xb43/0x1400 mm/slub.c:4360
 __kmalloc_reserve net/core/skbuff.c:138 [inline]
 __alloc_skb+0x422/0xe90 net/core/skbuff.c:206
 alloc_skb include/linux/skbuff.h:996 [inline]
 netlink_alloc_large_skb net/netlink/af_netlink.c:1189 [inline]
 netlink_sendmsg+0xcaf/0x1420 net/netlink/af_netlink.c:1883
 sock_sendmsg_nosec net/socket.c:621 [inline]
 sock_sendmsg net/socket.c:631 [inline]
 ___sys_sendmsg+0xe47/0x1200 net/socket.c:2116
 __sys_sendmsg net/socket.c:2154 [inline]
 __do_sys_sendmsg net/socket.c:2163 [inline]
 __se_sys_sendmsg+0x307/0x460 net/socket.c:2161
 __x64_sys_sendmsg+0x4a/0x70 net/socket.c:2161
 do_syscall_64+0xbe/0x100 arch/x86/entry/common.c:291
 entry_SYSCALL_64_after_hwframe+0x63/0xe7

We cannot take for granted the thing that the length of data contained
in TLV is longer than the size of struct tipc_name_table_query in
tipc_nl_compat_name_table_dump().

Reported-by: syz...@sy...
Signed-off-by: Ying Xue <yin...@wi...>
---
 net/tipc/netlink_compat.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
index 89e6ae3..b90786c 100644
--- a/net/tipc/netlink_compat.c
+++ b/net/tipc/netlink_compat.c
@@ -824,6 +824,8 @@ static int tipc_nl_compat_name_table_dump_header(struct tipc_nl_compat_msg *msg)
 	};
 
 	ntq = (struct tipc_name_table_query *)TLV_DATA(msg->req);
+	if (TLV_GET_DATA_LEN(msg->req) < sizeof(struct tipc_name_table_query))
+		return -EINVAL;
 
 	depth = ntohl(ntq->depth);
 
-- 
2.7.4

[tipc-discussion] [net 6/6] tipc: fix uninit-value in tipc_nl_compat_doit

[Ying X. <yin...@wi...>]

BUG: KMSAN: uninit-value in tipc_nl_compat_doit+0x404/0xa10 net/tipc/netlink_compat.c:335
CPU: 0 PID: 4514 Comm: syz-executor485 Not tainted 4.16.0+ #87
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:17 [inline]
 dump_stack+0x185/0x1d0 lib/dump_stack.c:53
 kmsan_report+0x142/0x240 mm/kmsan/kmsan.c:1067
 __msan_warning_32+0x6c/0xb0 mm/kmsan/kmsan_instr.c:683
 tipc_nl_compat_doit+0x404/0xa10 net/tipc/netlink_compat.c:335
 tipc_nl_compat_recv+0x164b/0x2700 net/tipc/netlink_compat.c:1153
 genl_family_rcv_msg net/netlink/genetlink.c:599 [inline]
 genl_rcv_msg+0x1686/0x1810 net/netlink/genetlink.c:624
 netlink_rcv_skb+0x378/0x600 net/netlink/af_netlink.c:2447
 genl_rcv+0x63/0x80 net/netlink/genetlink.c:635
 netlink_unicast_kernel net/netlink/af_netlink.c:1311 [inline]
 netlink_unicast+0x166b/0x1740 net/netlink/af_netlink.c:1337
 netlink_sendmsg+0x1048/0x1310 net/netlink/af_netlink.c:1900
 sock_sendmsg_nosec net/socket.c:630 [inline]
 sock_sendmsg net/socket.c:640 [inline]
 ___sys_sendmsg+0xec0/0x1310 net/socket.c:2046
 __sys_sendmsg net/socket.c:2080 [inline]
 SYSC_sendmsg+0x2a3/0x3d0 net/socket.c:2091
 SyS_sendmsg+0x54/0x80 net/socket.c:2087
 do_syscall_64+0x309/0x430 arch/x86/entry/common.c:287
 entry_SYSCALL_64_after_hwframe+0x3d/0xa2
RIP: 0033:0x43fda9
RSP: 002b:00007ffd0c184ba8 EFLAGS: 00000213 ORIG_RAX: 000000000000002e
RAX: ffffffffffffffda RBX: 00000000004002c8 RCX: 000000000043fda9
RDX: 0000000000000000 RSI: 0000000020023000 RDI: 0000000000000003
RBP: 00000000006ca018 R08: 00000000004002c8 R09: 00000000004002c8
R10: 00000000004002c8 R11: 0000000000000213 R12: 00000000004016d0
R13: 0000000000401760 R14: 0000000000000000 R15: 0000000000000000

Uninit was created at:
 kmsan_save_stack_with_flags mm/kmsan/kmsan.c:278 [inline]
 kmsan_internal_poison_shadow+0xb8/0x1b0 mm/kmsan/kmsan.c:188
 kmsan_kmalloc+0x94/0x100 mm/kmsan/kmsan.c:314
 kmsan_slab_alloc+0x11/0x20 mm/kmsan/kmsan.c:321
 slab_post_alloc_hook mm/slab.h:445 [inline]
 slab_alloc_node mm/slub.c:2737 [inline]
 __kmalloc_node_track_caller+0xaed/0x11c0 mm/slub.c:4369
 __kmalloc_reserve net/core/skbuff.c:138 [inline]
 __alloc_skb+0x2cf/0x9f0 net/core/skbuff.c:206
 alloc_skb include/linux/skbuff.h:984 [inline]
 netlink_alloc_large_skb net/netlink/af_netlink.c:1183 [inline]
 netlink_sendmsg+0x9a6/0x1310 net/netlink/af_netlink.c:1875
 sock_sendmsg_nosec net/socket.c:630 [inline]
 sock_sendmsg net/socket.c:640 [inline]
 ___sys_sendmsg+0xec0/0x1310 net/socket.c:2046
 __sys_sendmsg net/socket.c:2080 [inline]
 SYSC_sendmsg+0x2a3/0x3d0 net/socket.c:2091
 SyS_sendmsg+0x54/0x80 net/socket.c:2087
 do_syscall_64+0x309/0x430 arch/x86/entry/common.c:287
 entry_SYSCALL_64_after_hwframe+0x3d/0xa2

In tipc_nl_compat_recv(), when the len variable returned by
nlmsg_attrlen() is 0, the message is still treated as a valid one,
which is obviously unresonable. When len is zero, it means the
message not only doesn't contain any valid TLV payload, but also
TLV header is not included. Under this stituation, tlv_type field
in TLV header is still accessed in tipc_nl_compat_dumpit() or
tipc_nl_compat_doit(), but the field space is obviously illegal.
Of course, it is not initialized.

Reported-by: syz...@sy...
Reported-by: syz...@sy...
Signed-off-by: Ying Xue <yin...@wi...>
---
 net/tipc/netlink_compat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
index b90786c..4ad3586 100644
--- a/net/tipc/netlink_compat.c
+++ b/net/tipc/netlink_compat.c
@@ -1256,7 +1256,7 @@ static int tipc_nl_compat_recv(struct sk_buff *skb, struct genl_info *info)
 	}
 
 	len = nlmsg_attrlen(req_nlh, GENL_HDRLEN + TIPC_GENL_HDRLEN);
-	if (len && !TLV_OK(msg.req, len)) {
+	if (!len || !TLV_OK(msg.req, len)) {
 		msg.rep = tipc_get_err_tlv(TIPC_CFG_NOT_SUPPORTED);
 		err = -EOPNOTSUPP;
 		goto send;
-- 
2.7.4

[tipc-discussion] [net 2/6] tipc: fix uninit-value in tipc_nl_compat_link_reset_stats

[Ying X. <yin...@wi...>]

syzbot reports following splat:

BUG: KMSAN: uninit-value in strlen+0x3b/0xa0 lib/string.c:486
CPU: 1 PID: 11057 Comm: syz-executor0 Not tainted 4.20.0-rc7+ #2
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x173/0x1d0 lib/dump_stack.c:113
 kmsan_report+0x12e/0x2a0 mm/kmsan/kmsan.c:613
 __msan_warning+0x82/0xf0 mm/kmsan/kmsan_instr.c:295
 strlen+0x3b/0xa0 lib/string.c:486
 nla_put_string include/net/netlink.h:1154 [inline]
 tipc_nl_compat_link_reset_stats+0x1f0/0x360 net/tipc/netlink_compat.c:760
 __tipc_nl_compat_doit net/tipc/netlink_compat.c:311 [inline]
 tipc_nl_compat_doit+0x3aa/0xaf0 net/tipc/netlink_compat.c:344
 tipc_nl_compat_handle net/tipc/netlink_compat.c:1107 [inline]
 tipc_nl_compat_recv+0x14d7/0x2760 net/tipc/netlink_compat.c:1210
 genl_family_rcv_msg net/netlink/genetlink.c:601 [inline]
 genl_rcv_msg+0x185f/0x1a60 net/netlink/genetlink.c:626
 netlink_rcv_skb+0x444/0x640 net/netlink/af_netlink.c:2477
 genl_rcv+0x63/0x80 net/netlink/genetlink.c:637
 netlink_unicast_kernel net/netlink/af_netlink.c:1310 [inline]
 netlink_unicast+0xf40/0x1020 net/netlink/af_netlink.c:1336
 netlink_sendmsg+0x127f/0x1300 net/netlink/af_netlink.c:1917
 sock_sendmsg_nosec net/socket.c:621 [inline]
 sock_sendmsg net/socket.c:631 [inline]
 ___sys_sendmsg+0xdb9/0x11b0 net/socket.c:2116
 __sys_sendmsg net/socket.c:2154 [inline]
 __do_sys_sendmsg net/socket.c:2163 [inline]
 __se_sys_sendmsg+0x305/0x460 net/socket.c:2161
 __x64_sys_sendmsg+0x4a/0x70 net/socket.c:2161
 do_syscall_64+0xbc/0xf0 arch/x86/entry/common.c:291
 entry_SYSCALL_64_after_hwframe+0x63/0xe7
RIP: 0033:0x457ec9
Code: 6d b7 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 3b b7 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007f2557338c78 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 0000000000457ec9
RDX: 0000000000000000 RSI: 00000000200001c0 RDI: 0000000000000003
RBP: 000000000073bf00 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00007f25573396d4
R13: 00000000004cb478 R14: 00000000004d86c8 R15: 00000000ffffffff

Uninit was created at:
 kmsan_save_stack_with_flags mm/kmsan/kmsan.c:204 [inline]
 kmsan_internal_poison_shadow+0x92/0x150 mm/kmsan/kmsan.c:158
 kmsan_kmalloc+0xa6/0x130 mm/kmsan/kmsan_hooks.c:176
 kmsan_slab_alloc+0xe/0x10 mm/kmsan/kmsan_hooks.c:185
 slab_post_alloc_hook mm/slab.h:446 [inline]
 slab_alloc_node mm/slub.c:2759 [inline]
 __kmalloc_node_track_caller+0xe18/0x1030 mm/slub.c:4383
 __kmalloc_reserve net/core/skbuff.c:137 [inline]
 __alloc_skb+0x309/0xa20 net/core/skbuff.c:205
 alloc_skb include/linux/skbuff.h:998 [inline]
 netlink_alloc_large_skb net/netlink/af_netlink.c:1182 [inline]
 netlink_sendmsg+0xb82/0x1300 net/netlink/af_netlink.c:1892
 sock_sendmsg_nosec net/socket.c:621 [inline]
 sock_sendmsg net/socket.c:631 [inline]
 ___sys_sendmsg+0xdb9/0x11b0 net/socket.c:2116
 __sys_sendmsg net/socket.c:2154 [inline]
 __do_sys_sendmsg net/socket.c:2163 [inline]
 __se_sys_sendmsg+0x305/0x460 net/socket.c:2161
 __x64_sys_sendmsg+0x4a/0x70 net/socket.c:2161
 do_syscall_64+0xbc/0xf0 arch/x86/entry/common.c:291
 entry_SYSCALL_64_after_hwframe+0x63/0xe7

The uninitialised access happened in tipc_nl_compat_link_reset_stats:
    nla_put_string(skb, TIPC_NLA_LINK_NAME, name)

This is because name string is not validated before it's used.

Reported-by: syz...@sy...
Signed-off-by: Ying Xue <yin...@wi...>
---
 net/tipc/netlink_compat.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
index 77e4b24..b2b115b 100644
--- a/net/tipc/netlink_compat.c
+++ b/net/tipc/netlink_compat.c
@@ -87,6 +87,11 @@ static int tipc_skb_tailroom(struct sk_buff *skb)
 	return limit;
 }
 
+static inline int TLV_GET_DATA_LEN(struct tlv_desc *tlv)
+{
+	return TLV_GET_LEN(tlv) - TLV_SPACE(0);
+}
+
 static int tipc_add_tlv(struct sk_buff *skb, u16 type, void *data, u16 len)
 {
 	struct tlv_desc *tlv = (struct tlv_desc *)skb_tail_pointer(skb);
@@ -166,6 +171,11 @@ static struct sk_buff *tipc_get_err_tlv(char *str)
 	return buf;
 }
 
+static inline bool string_is_valid(char *s, int len)
+{
+	return memchr(s, '\0', len) ? true : false;
+}
+
 static int __tipc_nl_compat_dumpit(struct tipc_nl_compat_cmd_dump *cmd,
 				   struct tipc_nl_compat_msg *msg,
 				   struct sk_buff *arg)
@@ -750,6 +760,7 @@ static int tipc_nl_compat_link_reset_stats(struct tipc_nl_compat_cmd_doit *cmd,
 {
 	char *name;
 	struct nlattr *link;
+	int len;
 
 	name = (char *)TLV_DATA(msg->req);
 
@@ -757,6 +768,10 @@ static int tipc_nl_compat_link_reset_stats(struct tipc_nl_compat_cmd_doit *cmd,
 	if (!link)
 		return -EMSGSIZE;
 
+	len = min_t(int, TLV_GET_DATA_LEN(msg->req), TIPC_MAX_LINK_NAME);
+	if (!string_is_valid(name, len))
+		return -EINVAL;
+
 	if (nla_put_string(skb, TIPC_NLA_LINK_NAME, name))
 		return -EMSGSIZE;
 
-- 
2.7.4

[tipc-discussion] [net 1/6] tipc: fix uninit-value in in tipc_conn_rcv_sub

[Ying X. <yin...@wi...>]

syzbot reported:

BUG: KMSAN: uninit-value in tipc_conn_rcv_sub+0x184/0x950 net/tipc/topsrv.c:373
CPU: 0 PID: 66 Comm: kworker/u4:4 Not tainted 4.17.0-rc3+ #88
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Workqueue: tipc_rcv tipc_conn_recv_work
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x185/0x1d0 lib/dump_stack.c:113
 kmsan_report+0x142/0x240 mm/kmsan/kmsan.c:1067
 __msan_warning_32+0x6c/0xb0 mm/kmsan/kmsan_instr.c:683
 tipc_conn_rcv_sub+0x184/0x950 net/tipc/topsrv.c:373
 tipc_conn_rcv_from_sock net/tipc/topsrv.c:409 [inline]
 tipc_conn_recv_work+0x3cd/0x560 net/tipc/topsrv.c:424
 process_one_work+0x12c6/0x1f60 kernel/workqueue.c:2145
 worker_thread+0x113c/0x24f0 kernel/workqueue.c:2279
 kthread+0x539/0x720 kernel/kthread.c:239
 ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:412

Local variable description: ----s.i@tipc_conn_recv_work
Variable was created at:
 tipc_conn_recv_work+0x65/0x560 net/tipc/topsrv.c:419
 process_one_work+0x12c6/0x1f60 kernel/workqueue.c:2145

In tipc_conn_rcv_from_sock(), it always supposes the length of message
received from sock_recvmsg() is not smaller than the size of struct
tipc_subscr. However, this assumption is false. Especially when the
length of received message is shorter than struct tipc_subscr size,
we will end up touching uninitialized fields in tipc_conn_rcv_sub().

Reported-by: syz...@sy...
Reported-by: syz...@sy...
Signed-off-by: Ying Xue <yin...@wi...>
---
 net/tipc/topsrv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
index efb16f6..a457c0f 100644
--- a/net/tipc/topsrv.c
+++ b/net/tipc/topsrv.c
@@ -398,7 +398,7 @@ static int tipc_conn_rcv_from_sock(struct tipc_conn *con)
 	ret = sock_recvmsg(con->sock, &msg, MSG_DONTWAIT);
 	if (ret == -EWOULDBLOCK)
 		return -EWOULDBLOCK;
-	if (ret > 0) {
+	if (ret == sizeof(s)) {
 		read_lock_bh(&sk->sk_callback_lock);
 		ret = tipc_conn_rcv_sub(srv, con, &s);
 		read_unlock_bh(&sk->sk_callback_lock);
-- 
2.7.4

[tipc-discussion] [net 0/6] tipc: fix uninit-value issues reported by syzbot

[Ying X. <yin...@wi...>]

Recently, syzbot complained that TIPC module exits several issues
associated with uninit-value type. So, in this series, we try to
fix them as many as possible.

Ying Xue (6):
  tipc: fix uninit-value in in tipc_conn_rcv_sub
  tipc: fix uninit-value in tipc_nl_compat_link_reset_stats
  tipc: fix uninit-value in tipc_nl_compat_bearer_enable
  tipc: fix uninit-value in tipc_nl_compat_link_set
  tipc: fix uninit-value in tipc_nl_compat_name_table_dump
  tipc: fix uninit-value in tipc_nl_compat_doit

 net/tipc/netlink_compat.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++-
 net/tipc/topsrv.c         |  2 +-
 2 files changed, 50 insertions(+), 2 deletions(-)

-- 
2.7.4

[tipc-discussion] [net 4/6] tipc: fix uninit-value in tipc_nl_compat_link_set

[Ying X. <yin...@wi...>]

syzbot reports following splat:

BUG: KMSAN: uninit-value in strlen+0x3b/0xa0 lib/string.c:486
CPU: 1 PID: 9306 Comm: syz-executor172 Not tainted 4.20.0-rc7+ #2
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0x173/0x1d0 lib/dump_stack.c:113
  kmsan_report+0x12e/0x2a0 mm/kmsan/kmsan.c:613
  __msan_warning+0x82/0xf0 mm/kmsan/kmsan_instr.c:313
  strlen+0x3b/0xa0 lib/string.c:486
  nla_put_string include/net/netlink.h:1154 [inline]
  __tipc_nl_compat_link_set net/tipc/netlink_compat.c:708 [inline]
  tipc_nl_compat_link_set+0x929/0x1220 net/tipc/netlink_compat.c:744
  __tipc_nl_compat_doit net/tipc/netlink_compat.c:311 [inline]
  tipc_nl_compat_doit+0x3aa/0xaf0 net/tipc/netlink_compat.c:344
  tipc_nl_compat_handle net/tipc/netlink_compat.c:1107 [inline]
  tipc_nl_compat_recv+0x14d7/0x2760 net/tipc/netlink_compat.c:1210
  genl_family_rcv_msg net/netlink/genetlink.c:601 [inline]
  genl_rcv_msg+0x185f/0x1a60 net/netlink/genetlink.c:626
  netlink_rcv_skb+0x444/0x640 net/netlink/af_netlink.c:2477
  genl_rcv+0x63/0x80 net/netlink/genetlink.c:637
  netlink_unicast_kernel net/netlink/af_netlink.c:1310 [inline]
  netlink_unicast+0xf40/0x1020 net/netlink/af_netlink.c:1336
  netlink_sendmsg+0x127f/0x1300 net/netlink/af_netlink.c:1917
  sock_sendmsg_nosec net/socket.c:621 [inline]
  sock_sendmsg net/socket.c:631 [inline]
  ___sys_sendmsg+0xdb9/0x11b0 net/socket.c:2116
  __sys_sendmsg net/socket.c:2154 [inline]
  __do_sys_sendmsg net/socket.c:2163 [inline]
  __se_sys_sendmsg+0x305/0x460 net/socket.c:2161
  __x64_sys_sendmsg+0x4a/0x70 net/socket.c:2161
  do_syscall_64+0xbc/0xf0 arch/x86/entry/common.c:291
  entry_SYSCALL_64_after_hwframe+0x63/0xe7

The uninitialised access happened in
    nla_put_string(skb, TIPC_NLA_LINK_NAME, lc->name)

This is because lc->name string is not validated before it's used.

Reported-by: syz...@sy...
Signed-off-by: Ying Xue <yin...@wi...>
---
 net/tipc/netlink_compat.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
index 68a0b73..89e6ae3 100644
--- a/net/tipc/netlink_compat.c
+++ b/net/tipc/netlink_compat.c
@@ -762,9 +762,14 @@ static int tipc_nl_compat_link_set(struct tipc_nl_compat_cmd_doit *cmd,
 	struct tipc_link_config *lc;
 	struct tipc_bearer *bearer;
 	struct tipc_media *media;
+	int len;
 
 	lc = (struct tipc_link_config *)TLV_DATA(msg->req);
 
+	len = min_t(int, TLV_GET_DATA_LEN(msg->req), TIPC_MAX_LINK_NAME);
+	if (!string_is_valid(lc->name, len))
+		return -EINVAL;
+
 	media = tipc_media_find(lc->name);
 	if (media) {
 		cmd->doit = &__tipc_nl_media_set;
-- 
2.7.4

[tipc-discussion] [net 3/6] tipc: fix uninit-value in tipc_nl_compat_bearer_enable

[Ying X. <yin...@wi...>]

syzbot reported:

BUG: KMSAN: uninit-value in strlen+0x3b/0xa0 lib/string.c:484
CPU: 1 PID: 6371 Comm: syz-executor652 Not tainted 4.19.0-rc8+ #70
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x306/0x460 lib/dump_stack.c:113
 kmsan_report+0x1a2/0x2e0 mm/kmsan/kmsan.c:917
 __msan_warning+0x7c/0xe0 mm/kmsan/kmsan_instr.c:500
 strlen+0x3b/0xa0 lib/string.c:484
 nla_put_string include/net/netlink.h:1011 [inline]
 tipc_nl_compat_bearer_enable+0x238/0x7b0 net/tipc/netlink_compat.c:389
 __tipc_nl_compat_doit net/tipc/netlink_compat.c:311 [inline]
 tipc_nl_compat_doit+0x39f/0xae0 net/tipc/netlink_compat.c:344
 tipc_nl_compat_recv+0x147c/0x2760 net/tipc/netlink_compat.c:1107
 genl_family_rcv_msg net/netlink/genetlink.c:601 [inline]
 genl_rcv_msg+0x185c/0x1a20 net/netlink/genetlink.c:626
 netlink_rcv_skb+0x394/0x640 net/netlink/af_netlink.c:2454
 genl_rcv+0x63/0x80 net/netlink/genetlink.c:637
 netlink_unicast_kernel net/netlink/af_netlink.c:1317 [inline]
 netlink_unicast+0x166d/0x1720 net/netlink/af_netlink.c:1343
 netlink_sendmsg+0x1391/0x1420 net/netlink/af_netlink.c:1908
 sock_sendmsg_nosec net/socket.c:621 [inline]
 sock_sendmsg net/socket.c:631 [inline]
 ___sys_sendmsg+0xe47/0x1200 net/socket.c:2116
 __sys_sendmsg net/socket.c:2154 [inline]
 __do_sys_sendmsg net/socket.c:2163 [inline]
 __se_sys_sendmsg+0x307/0x460 net/socket.c:2161
 __x64_sys_sendmsg+0x4a/0x70 net/socket.c:2161
 do_syscall_64+0xbe/0x100 arch/x86/entry/common.c:291
 entry_SYSCALL_64_after_hwframe+0x63/0xe7
RIP: 0033:0x440179
Code: 18 89 d0 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 fb 13 fc ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007fffef7beee8 EFLAGS: 00000213 ORIG_RAX: 000000000000002e
RAX: ffffffffffffffda RBX: 00000000004002c8 RCX: 0000000000440179
RDX: 0000000000000000 RSI: 0000000020000100 RDI: 0000000000000003
RBP: 00000000006ca018 R08: 0000000000000000 R09: 00000000004002c8
R10: 0000000000000000 R11: 0000000000000213 R12: 0000000000401a00
R13: 0000000000401a90 R14: 0000000000000000 R15: 0000000000000000

Uninit was created at:
 kmsan_save_stack_with_flags mm/kmsan/kmsan.c:255 [inline]
 kmsan_internal_poison_shadow+0xc8/0x1d0 mm/kmsan/kmsan.c:180
 kmsan_kmalloc+0xa4/0x120 mm/kmsan/kmsan_hooks.c:104
 kmsan_slab_alloc+0x10/0x20 mm/kmsan/kmsan_hooks.c:113
 slab_post_alloc_hook mm/slab.h:446 [inline]
 slab_alloc_node mm/slub.c:2727 [inline]
 __kmalloc_node_track_caller+0xb43/0x1400 mm/slub.c:4360
 __kmalloc_reserve net/core/skbuff.c:138 [inline]
 __alloc_skb+0x422/0xe90 net/core/skbuff.c:206
 alloc_skb include/linux/skbuff.h:996 [inline]
 netlink_alloc_large_skb net/netlink/af_netlink.c:1189 [inline]
 netlink_sendmsg+0xcaf/0x1420 net/netlink/af_netlink.c:1883
 sock_sendmsg_nosec net/socket.c:621 [inline]
 sock_sendmsg net/socket.c:631 [inline]
 ___sys_sendmsg+0xe47/0x1200 net/socket.c:2116
 __sys_sendmsg net/socket.c:2154 [inline]
 __do_sys_sendmsg net/socket.c:2163 [inline]
 __se_sys_sendmsg+0x307/0x460 net/socket.c:2161
 __x64_sys_sendmsg+0x4a/0x70 net/socket.c:2161
 do_syscall_64+0xbe/0x100 arch/x86/entry/common.c:291
 entry_SYSCALL_64_after_hwframe+0x63/0xe7

The root cause is that we don't validate whether bear name is a valid
string in tipc_nl_compat_bearer_enable().

Meanwhile, we also fix the same issue in the following functions:
tipc_nl_compat_bearer_disable()
tipc_nl_compat_link_stat_dump()
tipc_nl_compat_media_set()
tipc_nl_compat_bearer_set()

Reported-by: syz...@sy...
Signed-off-by: Ying Xue <yin...@wi...>
---
 net/tipc/netlink_compat.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
index b2b115b..68a0b73 100644
--- a/net/tipc/netlink_compat.c
+++ b/net/tipc/netlink_compat.c
@@ -389,6 +389,7 @@ static int tipc_nl_compat_bearer_enable(struct tipc_nl_compat_cmd_doit *cmd,
 	struct nlattr *prop;
 	struct nlattr *bearer;
 	struct tipc_bearer_config *b;
+	int len;
 
 	b = (struct tipc_bearer_config *)TLV_DATA(msg->req);
 
@@ -396,6 +397,10 @@ static int tipc_nl_compat_bearer_enable(struct tipc_nl_compat_cmd_doit *cmd,
 	if (!bearer)
 		return -EMSGSIZE;
 
+	len = min_t(int, TLV_GET_DATA_LEN(msg->req), TIPC_MAX_BEARER_NAME);
+	if (!string_is_valid(b->name, len))
+		return -EINVAL;
+
 	if (nla_put_string(skb, TIPC_NLA_BEARER_NAME, b->name))
 		return -EMSGSIZE;
 
@@ -421,6 +426,7 @@ static int tipc_nl_compat_bearer_disable(struct tipc_nl_compat_cmd_doit *cmd,
 {
 	char *name;
 	struct nlattr *bearer;
+	int len;
 
 	name = (char *)TLV_DATA(msg->req);
 
@@ -428,6 +434,10 @@ static int tipc_nl_compat_bearer_disable(struct tipc_nl_compat_cmd_doit *cmd,
 	if (!bearer)
 		return -EMSGSIZE;
 
+	len = min_t(int, TLV_GET_DATA_LEN(msg->req), TIPC_MAX_BEARER_NAME);
+	if (!string_is_valid(name, len))
+		return -EINVAL;
+
 	if (nla_put_string(skb, TIPC_NLA_BEARER_NAME, name))
 		return -EMSGSIZE;
 
@@ -488,6 +498,7 @@ static int tipc_nl_compat_link_stat_dump(struct tipc_nl_compat_msg *msg,
 	struct nlattr *prop[TIPC_NLA_PROP_MAX + 1];
 	struct nlattr *stats[TIPC_NLA_STATS_MAX + 1];
 	int err;
+	int len;
 
 	if (!attrs[TIPC_NLA_LINK])
 		return -EINVAL;
@@ -514,6 +525,11 @@ static int tipc_nl_compat_link_stat_dump(struct tipc_nl_compat_msg *msg,
 		return err;
 
 	name = (char *)TLV_DATA(msg->req);
+
+	len = min_t(int, TLV_GET_DATA_LEN(msg->req), TIPC_MAX_LINK_NAME);
+	if (!string_is_valid(name, len))
+		return -EINVAL;
+
 	if (strcmp(name, nla_data(link[TIPC_NLA_LINK_NAME])) != 0)
 		return 0;
 
@@ -654,6 +670,7 @@ static int tipc_nl_compat_media_set(struct sk_buff *skb,
 	struct nlattr *prop;
 	struct nlattr *media;
 	struct tipc_link_config *lc;
+	int len;
 
 	lc = (struct tipc_link_config *)TLV_DATA(msg->req);
 
@@ -661,6 +678,10 @@ static int tipc_nl_compat_media_set(struct sk_buff *skb,
 	if (!media)
 		return -EMSGSIZE;
 
+	len = min_t(int, TLV_GET_DATA_LEN(msg->req), TIPC_MAX_MEDIA_NAME);
+	if (!string_is_valid(lc->name, len))
+		return -EINVAL;
+
 	if (nla_put_string(skb, TIPC_NLA_MEDIA_NAME, lc->name))
 		return -EMSGSIZE;
 
@@ -681,6 +702,7 @@ static int tipc_nl_compat_bearer_set(struct sk_buff *skb,
 	struct nlattr *prop;
 	struct nlattr *bearer;
 	struct tipc_link_config *lc;
+	int len;
 
 	lc = (struct tipc_link_config *)TLV_DATA(msg->req);
 
@@ -688,6 +710,10 @@ static int tipc_nl_compat_bearer_set(struct sk_buff *skb,
 	if (!bearer)
 		return -EMSGSIZE;
 
+	len = min_t(int, TLV_GET_DATA_LEN(msg->req), TIPC_MAX_MEDIA_NAME);
+	if (!string_is_valid(lc->name, len))
+		return -EINVAL;
+
 	if (nla_put_string(skb, TIPC_NLA_BEARER_NAME, lc->name))
 		return -EMSGSIZE;
 
-- 
2.7.4

[tipc-discussion] FW: [tipc:tipcutils] New commit [f6ae0c] by Tuong Lien

[Tuong L. T. <tuo...@de...>]

Hi all,

Just in case you didn't notice this commit, I'd like to notify that,
In addition to the TIPC trace-events in kernel, we now have a 'front-end' tool named 'tipc-trace' as part of the tipcutils which offers some options for us to do the traces more convenient.
Please feel free to try and enhance it!
Thanks a lot!

BR/Tuong

-----Original Message-----
From: TIPC Cluster Domain Sockets Git repository <no...@ti...> 
Sent: Tuesday, January 8, 2019 2:30 PM
To: TIPC Cluster Domain Sockets Git repository <no...@ti...>
Subject: [tipc:tipcutils] New commit [f6ae0c] by Tuong Lien


## Branch: master  

tipcutils: introduce a front-end tool for TIPC traces

This commit adds 'tipc-trace' to tipcutils which is a front-end
tool for TIPC traces. The tool allows tracing TIPC by means of the
TIPC trace_events in kernel, it offers some options to do the traces
more conveniently:

- Print all available kernel TIPC trace-events on node with a short
  description, check the trace status;
- Provide some 'trace-suites' with a ‘built-in’ configurations in
  order to automatically enable other traces, ... when a particular
  event happens (e.g. link lost, etc.);
- Configure the traces: enable, disable, set filter/trigger for
  traces or save traces as a trace-suite for later use;
- Start, stop the traces, collect the trace outputs;
- Interpret the trace outputs online or offline;
- Etc.

Tested-by: Jon Maloy <jon...@er...>
Acked-by: Jon Maloy <jon...@er...>
Signed-off-by: Tuong Lien <tuo...@de...>

By Tuong Lien on 12/14/2018 12:00
[**View Changes**](https://sourceforge.net/p/tipc/tipcutils/ci/f6ae0ce5fba595b5e725e2b5adfde1791e860c29/)





---

Sent from sourceforge.net because you indicated interest in <https://sourceforge.net/p/tipc/tipcutils/>



To unsubscribe from further messages, please visit <https://sourceforge.net/auth/subscriptions/>

Re: [tipc-discussion] [PATCH net] tipc: fix uninit-value in tipc_nl_compat_link_set

[Ying X. <yin...@wi...>]

On 1/7/19 9:38 PM, David Miller wrote:
> From: Ying Xue <yin...@wi...>
> Date: Mon, 7 Jan 2019 19:29:52 +0800
> 
>> This is because lc->name string is not validated before it's used.
> 
> It looks like we have several situations like this, not just this one.
> 
> For example, tipc_nl_compat_bearer_{enable,disable}() with b->name.
> 
> Next, tipc_nl_compat_media_set() and tipc_nl_compat_bearer_set().
> 
> On input, tipc_nl_compat_link_stat_dump() blindly does a strcmp()
> on one of these strings.
> 
> In fact, this entire file is full of errors of this sort.
> 
> Can you please address all of them, perhaps using a helper of
> some kind to consolidate the logic?
> 

Thank you for your good suggestions. I will solve them as soon as possible.

Regards,
Ying

> Thank you.
> 

Re: [tipc-discussion] [PATCH] tipc: fix memory leak in tipc_nl_compat_publ_dump

[David M. <da...@da...>]

From: "Gustavo A. R. Silva" <gu...@em...>
Date: Sat, 5 Jan 2019 10:52:23 -0600

> There is a memory leak in case genlmsg_put fails.
> 
> Fix this by freeing *args* before return.
> 
> Addresses-Coverity-ID: 1476406 ("Resource leak")
> Fixes: 46273cf7e009 ("tipc: fix a missing check of genlmsg_put")
> Signed-off-by: Gustavo A. R. Silva <gu...@em...>

Applied, thank you.

Re: [tipc-discussion] [PATCH net] tipc: fix uninit-value in tipc_nl_compat_link_set

[David M. <da...@da...>]

From: Ying Xue <yin...@wi...>
Date: Mon, 7 Jan 2019 19:29:52 +0800

> This is because lc->name string is not validated before it's used.

It looks like we have several situations like this, not just this one.

For example, tipc_nl_compat_bearer_{enable,disable}() with b->name.

Next, tipc_nl_compat_media_set() and tipc_nl_compat_bearer_set().

On input, tipc_nl_compat_link_stat_dump() blindly does a strcmp()
on one of these strings.

In fact, this entire file is full of errors of this sort.

Can you please address all of them, perhaps using a helper of
some kind to consolidate the logic?

Thank you.

[tipc-discussion] [PATCH net] tipc: fix uninit-value in tipc_nl_compat_link_set

[Ying X. <yin...@wi...>]

syzbot reports following splat:

BUG: KMSAN: uninit-value in strlen+0x3b/0xa0 lib/string.c:486
CPU: 1 PID: 9306 Comm: syz-executor172 Not tainted 4.20.0-rc7+ #2
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0x173/0x1d0 lib/dump_stack.c:113
  kmsan_report+0x12e/0x2a0 mm/kmsan/kmsan.c:613
  __msan_warning+0x82/0xf0 mm/kmsan/kmsan_instr.c:313
  strlen+0x3b/0xa0 lib/string.c:486
  nla_put_string include/net/netlink.h:1154 [inline]
  __tipc_nl_compat_link_set net/tipc/netlink_compat.c:708 [inline]
  tipc_nl_compat_link_set+0x929/0x1220 net/tipc/netlink_compat.c:744
  __tipc_nl_compat_doit net/tipc/netlink_compat.c:311 [inline]
  tipc_nl_compat_doit+0x3aa/0xaf0 net/tipc/netlink_compat.c:344
  tipc_nl_compat_handle net/tipc/netlink_compat.c:1107 [inline]
  tipc_nl_compat_recv+0x14d7/0x2760 net/tipc/netlink_compat.c:1210
  genl_family_rcv_msg net/netlink/genetlink.c:601 [inline]
  genl_rcv_msg+0x185f/0x1a60 net/netlink/genetlink.c:626
  netlink_rcv_skb+0x444/0x640 net/netlink/af_netlink.c:2477
  genl_rcv+0x63/0x80 net/netlink/genetlink.c:637
  netlink_unicast_kernel net/netlink/af_netlink.c:1310 [inline]
  netlink_unicast+0xf40/0x1020 net/netlink/af_netlink.c:1336
  netlink_sendmsg+0x127f/0x1300 net/netlink/af_netlink.c:1917
  sock_sendmsg_nosec net/socket.c:621 [inline]
  sock_sendmsg net/socket.c:631 [inline]
  ___sys_sendmsg+0xdb9/0x11b0 net/socket.c:2116
  __sys_sendmsg net/socket.c:2154 [inline]
  __do_sys_sendmsg net/socket.c:2163 [inline]
  __se_sys_sendmsg+0x305/0x460 net/socket.c:2161
  __x64_sys_sendmsg+0x4a/0x70 net/socket.c:2161
  do_syscall_64+0xbc/0xf0 arch/x86/entry/common.c:291
  entry_SYSCALL_64_after_hwframe+0x63/0xe7

The uninitialised access happened in
    nla_put_string(skb, TIPC_NLA_LINK_NAME, lc->name)

This is because lc->name string is not validated before it's used.

Reported-by: syz...@sy...
Signed-off-by: Ying Xue <yin...@wi...>
---
 net/tipc/netlink_compat.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
index 21f6ccc..bbf3f5a 100644
--- a/net/tipc/netlink_compat.c
+++ b/net/tipc/netlink_compat.c
@@ -705,6 +705,9 @@ static int __tipc_nl_compat_link_set(struct sk_buff *skb,
 	if (!link)
 		return -EMSGSIZE;
 
+	if (!memchr(lc->name, '\0', TIPC_MAX_LINK_NAME))
+		return -EINVAL;
+
 	if (nla_put_string(skb, TIPC_NLA_LINK_NAME, lc->name))
 		return -EMSGSIZE;
 
-- 
2.7.4

Re: [tipc-discussion] [PATCH] tipc: fix memory leak in tipc_nl_compat_publ_dump

[Ying X. <yin...@wi...>]

On 1/6/19 12:52 AM, Gustavo A. R. Silva wrote:
> There is a memory leak in case genlmsg_put fails.
> 
> Fix this by freeing *args* before return.
> 
> Addresses-Coverity-ID: 1476406 ("Resource leak")
> Fixes: 46273cf7e009 ("tipc: fix a missing check of genlmsg_put")
> Signed-off-by: Gustavo A. R. Silva <gu...@em...>

Acked-by: Ying Xue <yin...@wi...>

> ---
>  net/tipc/netlink_compat.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
> index 40f5cae623a7..77e4b2418f30 100644
> --- a/net/tipc/netlink_compat.c
> +++ b/net/tipc/netlink_compat.c
> @@ -904,8 +904,10 @@ static int tipc_nl_compat_publ_dump(struct tipc_nl_compat_msg *msg, u32 sock)
>  
>  	hdr = genlmsg_put(args, 0, 0, &tipc_genl_family, NLM_F_MULTI,
>  			  TIPC_NL_PUBL_GET);
> -	if (!hdr)
> +	if (!hdr) {
> +		kfree_skb(args);
>  		return -EMSGSIZE;
> +	}
>  
>  	nest = nla_nest_start(args, TIPC_NLA_SOCK);
>  	if (!nest) {
> 

Re: [tipc-discussion] [PATCH] tipc: fix a missing check of genlmsg_put

[David M. <da...@da...>]

From: Kangjie Lu <kj...@um...>
Date: Wed, 26 Dec 2018 00:09:04 -0600

> genlmsg_put could fail. The fix inserts a check of its return value, and
> if it fails, returns -EMSGSIZE.
> 
> Signed-off-by: Kangjie Lu <kj...@um...>

Applied.

Re: [tipc-discussion] [PATCH] net/netlink_compat: Fix a missing check of nla_parse_nested

[David M. <da...@da...>]

From: Aditya Pakki <pak...@um...>
Date: Sun, 23 Dec 2018 18:54:53 -0600

> In tipc_nl_compat_sk_dump(), if nla_parse_nested() fails, it could return
> an error. To be consistent with other invocations of the function call,
> on error, the fix passes the return value upstream.
> 
> Signed-off-by: Aditya Pakki <pak...@um...>

Applied, thanks.

Re: [tipc-discussion] FW: [net 1/1] tipc: sanity check on received netlink buffer

[Jon M. <jon...@er...>]

> -----Original Message-----
> From: Xin Long <luc...@gm...>
> Sent: 21-Dec-18 05:29
> To: Jon Maloy <jon...@er...>
> Cc: Ying Xue <yin...@wi...>; lx...@re...; tipc-
> dis...@li...
> Subject: Re: [tipc-discussion] FW: [net 1/1] tipc: sanity check on received
> netlink buffer
> 
> On Fri, Dec 21, 2018 at 12:35 AM Jon Maloy <jon...@er...>
> wrote:
> >
> > Hi Ying and Xin,
> > Any viewpoints on this before I send it in?
> > It should be noted that skb->tail in the worst case will be the same as skb-
> >end, which points to the first byte of the skb_shared_info area. Lucklily, and
> not only due to luck, I think, this byte happens to be named  " __unused" in
> that structure. So the change should be safe, and my tests have not revealed
> any problems. Whether this really solves the problem reported by syzbot I
> don't know, since I am unable to reproduce it, but this is my take on it.
> 
> I built a KMSAN env a couple of weeks ago, but it's gone now.
> I can rebuild one and try to reproduce it if you still need.
> 

According to Yings's previous comment that doesn't seem to be necessary.
///jon

> >
> > Regards
> > ///jon
> >
> >
> > -----Original Message-----
> > From: Jon Maloy
> > Sent: 18-Dec-18 15:29
> > To: Jon Maloy <jon...@er...>; Jon Maloy
> <ma...@do...>
> > Cc: Mohan Krishna Ghanta Krishnamurthy
> > <moh...@er...>;
> > par...@gm...; Tung Quang Nguyen
> > <tun...@de...>; Hoang Huu Le
> > <hoa...@de...>; Canh Duc Luu
> <can...@de...>;
> > Tuong Tong Lien <tuo...@de...>; Gordan Mihaljevic
> > <gor...@de...>; yin...@wi...;
> > tip...@li...
> > Subject: [net 1/1] tipc: sanity check on received netlink buffer
> >
> > When tipc receives a sk buffer in tipc_net_link_compat_rcv() it performs
> no controls that the buffer has the required minimum size. Furthermore, the
> buffer may contain a string, which we have no guarantee is zero- terminated.
> >
> > We now introduce a check that the buffer at least is large enough to
> contain a generic and a TIPC specific netlink header, since those must be
> present in all valid messages.
> >
> > We also set the buffer tail to point to a zero character. This ensures that
> subsequent string operations on buffer data never can fail, even if the given
> string is invalid.
> >
> > Reported-by: syz...@sy...
> > Reported-by: syz...@sy...
> > Signed-off-by: Jon Maloy <jon...@er...>
> > ---
> >  net/tipc/netlink_compat.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
> > index 6376467..b37ed6e 100644
> > --- a/net/tipc/netlink_compat.c
> > +++ b/net/tipc/netlink_compat.c
> > @@ -1188,6 +1188,13 @@ static int tipc_nl_compat_recv(struct sk_buff
> > *skb, struct genl_info *info)
> >
> >         memset(&msg, 0, sizeof(msg));
> >
> > +       if (skb_headlen(skb) < GENL_HDRLEN + TIPC_GENL_HDRLEN) {
> > +               err = -EINVAL;
> > +               msg.rep = tipc_get_err_tlv(TIPC_CFG_TLV_ERROR);
> > +               goto send;
> > +       }
> 1. This check may not be necessary, as genl_family_rcv_msg() could cover it,
> no?
> 
>         hdrlen = GENL_HDRLEN + family->hdrsize;
>         if (nlh->nlmsg_len < nlmsg_msg_size(hdrlen))
>                 return -EINVAL;
> 
> 
> > +       *skb_tail_pointer(skb) = 0;
> > +
> It may be a little bit tricky, but yes, a very easy fix. I'm thinking:
> 
> 2. https://www.spinics.net/lists/netdev/msg540733.html
> 
> This is a common issue in TIPC netlink when parsing name string. I was
> looking at the processing for ifname in rtnl_setlink(), and it's using
> nla_strlcpy(). So maybe it's better to do the same here?
> 
> --- a/net/tipc/bearer.c
> +++ b/net/tipc/bearer.c
> @@ -857,7 +857,7 @@ int tipc_nl_bearer_disable(struct sk_buff *skb, struct
> genl_info *info)  int __tipc_nl_bearer_enable(struct sk_buff *skb, struct
> genl_info *info)  {
>         int err;
> -       char *bearer;
> +       char bearer[TIPC_MAX_BEARER_NAME];
>         struct nlattr *attrs[TIPC_NLA_BEARER_MAX + 1];
>         struct net *net = sock_net(skb->sk);
>         u32 domain = 0;
> @@ -868,6 +868,7 @@ int __tipc_nl_bearer_enable(struct sk_buff *skb,
> struct genl_info *info)
>         if (!info->attrs[TIPC_NLA_BEARER])
>                 return -EINVAL;
> 
> +       nla_strlcpy(bearer, info->attrs[TIPC_NLA_BEARER],
> + TIPC_MAX_BEARER_NAME);
>         err = nla_parse_nested(attrs, TIPC_NLA_BEARER_MAX,
>                                info->attrs[TIPC_NLA_BEARER],
>                                tipc_nl_bearer_policy, info->extack);
> 
> 3. https://www.spinics.net/lists/netdev/msg540734.html
> 
> the similar thing below in tipc_nl_compat_link_set()?
> but should do something more for tipc_link_config which is more than a
> string.
> 
> @@ -723,19 +723,21 @@ static int tipc_nl_compat_link_set(struct
> tipc_nl_compat_cmd_doit *cmd,
>                                    struct sk_buff *skb,
>                                    struct tipc_nl_compat_msg *msg)  {
> -       struct tipc_link_config *lc;
> +       int len = TLV_GET_LEN(msg->req) - TLV_LENGTH(0);
> +       struct tipc_link_config lc = {0};
>         struct tipc_bearer *bearer;
>         struct tipc_media *media;
> 
> -       lc = (struct tipc_link_config *)TLV_DATA(msg->req);
> +       memcpy(&lc, TLV_DATA(msg->req),
> +              len >= sizeof(lc) ? sizeof(lc) - 1 : len);
> 
> -       media = tipc_media_find(lc->name);
> +       media = tipc_media_find(lc.name);
> 
> 
> >         req_nlh = (struct nlmsghdr *)skb->data;
> >         msg.req = nlmsg_data(req_nlh) + GENL_HDRLEN +
> TIPC_GENL_HDRLEN;
> >         msg.cmd = req_userhdr->cmd;
> > --
> > 2.1.4
> >
> >
> >
> > _______________________________________________
> > tipc-discussion mailing list
> > tip...@li...
> > https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] FW: [net 1/1] tipc: sanity check on received netlink buffer

[Xin L. <luc...@gm...>]

On Fri, Dec 21, 2018 at 12:35 AM Jon Maloy <jon...@er...> wrote:
>
> Hi Ying and Xin,
> Any viewpoints on this before I send it in?
> It should be noted that skb->tail in the worst case will be the same as skb->end, which points to the first byte of the skb_shared_info area. Lucklily, and not only due to luck, I think, this byte happens to be named  " __unused" in that structure. So the change should be safe, and my tests have not revealed any problems. Whether this really solves the problem reported by syzbot I don't know, since I am unable to reproduce it, but this is my take on it.

I built a KMSAN env a couple of weeks ago, but it's gone now.
I can rebuild one and try to reproduce it if you still need.

>
> Regards
> ///jon
>
>
> -----Original Message-----
> From: Jon Maloy
> Sent: 18-Dec-18 15:29
> To: Jon Maloy <jon...@er...>; Jon Maloy <ma...@do...>
> Cc: Mohan Krishna Ghanta Krishnamurthy <moh...@er...>; par...@gm...; Tung Quang Nguyen <tun...@de...>; Hoang Huu Le <hoa...@de...>; Canh Duc Luu <can...@de...>; Tuong Tong Lien <tuo...@de...>; Gordan Mihaljevic <gor...@de...>; yin...@wi...; tip...@li...
> Subject: [net 1/1] tipc: sanity check on received netlink buffer
>
> When tipc receives a sk buffer in tipc_net_link_compat_rcv() it performs no controls that the buffer has the required minimum size. Furthermore, the buffer may contain a string, which we have no guarantee is zero- terminated.
>
> We now introduce a check that the buffer at least is large enough to contain a generic and a TIPC specific netlink header, since those must be present in all valid messages.
>
> We also set the buffer tail to point to a zero character. This ensures that subsequent string operations on buffer data never can fail, even if the given string is invalid.
>
> Reported-by: syz...@sy...
> Reported-by: syz...@sy...
> Signed-off-by: Jon Maloy <jon...@er...>
> ---
>  net/tipc/netlink_compat.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c index 6376467..b37ed6e 100644
> --- a/net/tipc/netlink_compat.c
> +++ b/net/tipc/netlink_compat.c
> @@ -1188,6 +1188,13 @@ static int tipc_nl_compat_recv(struct sk_buff *skb, struct genl_info *info)
>
>         memset(&msg, 0, sizeof(msg));
>
> +       if (skb_headlen(skb) < GENL_HDRLEN + TIPC_GENL_HDRLEN) {
> +               err = -EINVAL;
> +               msg.rep = tipc_get_err_tlv(TIPC_CFG_TLV_ERROR);
> +               goto send;
> +       }
1. This check may not be necessary, as genl_family_rcv_msg() could cover it, no?

        hdrlen = GENL_HDRLEN + family->hdrsize;
        if (nlh->nlmsg_len < nlmsg_msg_size(hdrlen))
                return -EINVAL;


> +       *skb_tail_pointer(skb) = 0;
> +
It may be a little bit tricky, but yes, a very easy fix. I'm thinking:

2. https://www.spinics.net/lists/netdev/msg540733.html

This is a common issue in TIPC netlink when parsing name string. I was
looking at the processing for ifname in rtnl_setlink(), and it's using
nla_strlcpy(). So maybe it's better to do the same here?

--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -857,7 +857,7 @@ int tipc_nl_bearer_disable(struct sk_buff *skb,
struct genl_info *info)
 int __tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info)
 {
        int err;
-       char *bearer;
+       char bearer[TIPC_MAX_BEARER_NAME];
        struct nlattr *attrs[TIPC_NLA_BEARER_MAX + 1];
        struct net *net = sock_net(skb->sk);
        u32 domain = 0;
@@ -868,6 +868,7 @@ int __tipc_nl_bearer_enable(struct sk_buff *skb,
struct genl_info *info)
        if (!info->attrs[TIPC_NLA_BEARER])
                return -EINVAL;

+       nla_strlcpy(bearer, info->attrs[TIPC_NLA_BEARER], TIPC_MAX_BEARER_NAME);
        err = nla_parse_nested(attrs, TIPC_NLA_BEARER_MAX,
                               info->attrs[TIPC_NLA_BEARER],
                               tipc_nl_bearer_policy, info->extack);

3. https://www.spinics.net/lists/netdev/msg540734.html

the similar thing below in tipc_nl_compat_link_set()?
but should do something more for tipc_link_config which is more than a string.

@@ -723,19 +723,21 @@ static int tipc_nl_compat_link_set(struct
tipc_nl_compat_cmd_doit *cmd,
                                   struct sk_buff *skb,
                                   struct tipc_nl_compat_msg *msg)
 {
-       struct tipc_link_config *lc;
+       int len = TLV_GET_LEN(msg->req) - TLV_LENGTH(0);
+       struct tipc_link_config lc = {0};
        struct tipc_bearer *bearer;
        struct tipc_media *media;

-       lc = (struct tipc_link_config *)TLV_DATA(msg->req);
+       memcpy(&lc, TLV_DATA(msg->req),
+              len >= sizeof(lc) ? sizeof(lc) - 1 : len);

-       media = tipc_media_find(lc->name);
+       media = tipc_media_find(lc.name);


>         req_nlh = (struct nlmsghdr *)skb->data;
>         msg.req = nlmsg_data(req_nlh) + GENL_HDRLEN + TIPC_GENL_HDRLEN;
>         msg.cmd = req_userhdr->cmd;
> --
> 2.1.4
>
>
>
> _______________________________________________
> tipc-discussion mailing list
> tip...@li...
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion

[tipc-discussion] FW: [net 1/1] tipc: sanity check on received netlink buffer

[Jon M. <jon...@er...>]

Hi Ying and Xin,
Any viewpoints on this before I send it in?
It should be noted that skb->tail in the worst case will be the same as skb->end, which points to the first byte of the skb_shared_info area. Lucklily, and not only due to luck, I think, this byte happens to be named  " __unused" in that structure. So the change should be safe, and my tests have not revealed any problems. Whether this really solves the problem reported by syzbot I don't know, since I am unable to reproduce it, but this is my take on it.

Regards
///jon


-----Original Message-----
From: Jon Maloy 
Sent: 18-Dec-18 15:29
To: Jon Maloy <jon...@er...>; Jon Maloy <ma...@do...>
Cc: Mohan Krishna Ghanta Krishnamurthy <moh...@er...>; par...@gm...; Tung Quang Nguyen <tun...@de...>; Hoang Huu Le <hoa...@de...>; Canh Duc Luu <can...@de...>; Tuong Tong Lien <tuo...@de...>; Gordan Mihaljevic <gor...@de...>; yin...@wi...; tip...@li...
Subject: [net 1/1] tipc: sanity check on received netlink buffer

When tipc receives a sk buffer in tipc_net_link_compat_rcv() it performs no controls that the buffer has the required minimum size. Furthermore, the buffer may contain a string, which we have no guarantee is zero- terminated.

We now introduce a check that the buffer at least is large enough to contain a generic and a TIPC specific netlink header, since those must be present in all valid messages.

We also set the buffer tail to point to a zero character. This ensures that subsequent string operations on buffer data never can fail, even if the given string is invalid.

Reported-by: syz...@sy...
Reported-by: syz...@sy...
Signed-off-by: Jon Maloy <jon...@er...>
---
 net/tipc/netlink_compat.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c index 6376467..b37ed6e 100644
--- a/net/tipc/netlink_compat.c
+++ b/net/tipc/netlink_compat.c
@@ -1188,6 +1188,13 @@ static int tipc_nl_compat_recv(struct sk_buff *skb, struct genl_info *info)
 
 	memset(&msg, 0, sizeof(msg));
 
+	if (skb_headlen(skb) < GENL_HDRLEN + TIPC_GENL_HDRLEN) {
+		err = -EINVAL;
+		msg.rep = tipc_get_err_tlv(TIPC_CFG_TLV_ERROR);
+		goto send;
+	}
+	*skb_tail_pointer(skb) = 0;
+
 	req_nlh = (struct nlmsghdr *)skb->data;
 	msg.req = nlmsg_data(req_nlh) + GENL_HDRLEN + TIPC_GENL_HDRLEN;
 	msg.cmd = req_userhdr->cmd;
--
2.1.4

[tipc-discussion] [iproute2-next] tipc: enable broadcast configurable for bearer

[Hoang Le <hoa...@de...>]

Introduce to configure bearer to indicate it does support
broadcast whether or not.

Usage:
- Enable broadcast for bearer:
$tipc bearer set broadcast 1 media eth dev <device>

- Disable broadcast for bearer:
$tipc bearer set broadcast 0 media eth dev <device>

Signed-off-by: Hoang Le <hoa...@de...>
---
 include/uapi/linux/tipc_netlink.h |  1 +
 tipc/bearer.c                     | 11 +++++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/tipc_netlink.h b/include/uapi/linux/tipc_netlink.h
index 0ebe02ef1a86..92ecf3cd4013 100644
--- a/include/uapi/linux/tipc_netlink.h
+++ b/include/uapi/linux/tipc_netlink.h
@@ -281,6 +281,7 @@ enum {
 	TIPC_NLA_PROP_TOL,		/* u32 */
 	TIPC_NLA_PROP_WIN,		/* u32 */
 	TIPC_NLA_PROP_MTU,		/* u32 */
+	TIPC_NLA_PROP_BCAST,		/* u32 */
 
 	__TIPC_NLA_PROP_MAX,
 	TIPC_NLA_PROP_MAX = __TIPC_NLA_PROP_MAX - 1
diff --git a/tipc/bearer.c b/tipc/bearer.c
index 05dc84aa8ded..8433e3dfb9cb 100644
--- a/tipc/bearer.c
+++ b/tipc/bearer.c
@@ -43,7 +43,8 @@ static void _print_bearer_opts(void)
 		" priority              - Bearer link priority\n"
 		" tolerance             - Bearer link tolerance\n"
 		" window                - Bearer link window\n"
-		" mtu                   - Bearer link mtu\n");
+		" mtu                   - Bearer link mtu\n"
+		" broadcast             - Bearer link broadcast\n");
 }
 
 void print_bearer_media(void)
@@ -570,6 +571,8 @@ static int cmd_bearer_set_prop(struct nlmsghdr *nlh, const struct cmd *cmd,
 		prop = TIPC_NLA_PROP_WIN;
 	else if ((strcmp(cmd->cmd, "mtu") == 0))
 		prop = TIPC_NLA_PROP_MTU;
+	else if ((strcmp(cmd->cmd, "broadcast") == 0))
+		prop = TIPC_NLA_PROP_BCAST;
 	else
 		return -EINVAL;
 
@@ -620,6 +623,7 @@ static int cmd_bearer_set(struct nlmsghdr *nlh, const struct cmd *cmd,
 		{ "tolerance",	cmd_bearer_set_prop,	cmd_bearer_set_help },
 		{ "window",	cmd_bearer_set_prop,	cmd_bearer_set_help },
 		{ "mtu",	cmd_bearer_set_prop,	cmd_bearer_set_help },
+		{ "broadcast",	cmd_bearer_set_prop,	cmd_bearer_set_help },
 		{ NULL }
 	};
 
@@ -902,6 +906,8 @@ static int cmd_bearer_get_prop(struct nlmsghdr *nlh, const struct cmd *cmd,
 		prop = TIPC_NLA_PROP_WIN;
 	else if ((strcmp(cmd->cmd, "mtu") == 0))
 		prop = TIPC_NLA_PROP_MTU;
+	else if ((strcmp(cmd->cmd, "broadcast") == 0))
+		prop = TIPC_NLA_PROP_BCAST;
 	else
 		return -EINVAL;
 
@@ -942,6 +948,7 @@ static int cmd_bearer_get(struct nlmsghdr *nlh, const struct cmd *cmd,
 		{ "window",	cmd_bearer_get_prop,	cmd_bearer_get_help },
 		{ "mtu",	cmd_bearer_get_prop,	cmd_bearer_get_help },
 		{ "media",	cmd_bearer_get_media,	cmd_bearer_get_help },
+		{ "broadcast",	cmd_bearer_get_prop,	cmd_bearer_get_help },
 		{ NULL }
 	};
 
@@ -995,7 +1002,7 @@ void cmd_bearer_help(struct cmdl *cmdl)
 		"Usage: %s bearer COMMAND [ARGS] ...\n"
 		"\n"
 		"COMMANDS\n"
-		" add			- Add data to existing bearer\n"
+		" add                   - Add data to existing bearer\n"
 		" enable                - Enable a bearer\n"
 		" disable               - Disable a bearer\n"
 		" set                   - Set various bearer properties\n"
-- 
2.17.1

[tipc-discussion] [net-next] tipc: enable broadcast configurable for bearer

[Hoang Le <hoa...@de...>]

Introduce to configure bearer to indicate it does support
broadcast whether or not.

Signed-off-by: Hoang Le <hoa...@de...>
---
 include/uapi/linux/tipc_netlink.h |  1 +
 net/tipc/bcast.c                  | 13 +++++++++++--
 net/tipc/bcast.h                  |  2 ++
 net/tipc/bearer.c                 | 16 +++++++++++++---
 net/tipc/netlink.c                |  3 ++-
 5 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/include/uapi/linux/tipc_netlink.h b/include/uapi/linux/tipc_netlink.h
index 0ebe02ef1a86..92ecf3cd4013 100644
--- a/include/uapi/linux/tipc_netlink.h
+++ b/include/uapi/linux/tipc_netlink.h
@@ -281,6 +281,7 @@ enum {
 	TIPC_NLA_PROP_TOL,		/* u32 */
 	TIPC_NLA_PROP_WIN,		/* u32 */
 	TIPC_NLA_PROP_MTU,		/* u32 */
+	TIPC_NLA_PROP_BCAST,		/* u32 */
 
 	__TIPC_NLA_PROP_MAX,
 	TIPC_NLA_PROP_MAX = __TIPC_NLA_PROP_MAX - 1
diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c
index d8026543bf4c..c7b1f619855c 100644
--- a/net/tipc/bcast.c
+++ b/net/tipc/bcast.c
@@ -88,6 +88,16 @@ void tipc_bcast_disable_rcast(struct net *net)
 	tipc_bc_base(net)->rcast_support = false;
 }
 
+bool tipc_bcast_get_bcast(struct net *net)
+{
+	return tipc_bc_base(net)->bcast_support;
+}
+
+void tipc_bcast_set_bcast(struct net *net, bool sup)
+{
+	tipc_bc_base(net)->bcast_support = sup;
+}
+
 static void tipc_bcbase_calc_bc_threshold(struct net *net)
 {
 	struct tipc_bc_base *bb = tipc_bc_base(net);
@@ -106,7 +116,6 @@ static void tipc_bcbase_select_primary(struct net *net)
 	int i, mtu, prim;
 
 	bb->primary_bearer = INVALID_BEARER_ID;
-	bb->bcast_support = true;
 
 	if (!all_dests)
 		return;
@@ -130,7 +139,7 @@ static void tipc_bcbase_select_primary(struct net *net)
 	}
 	prim = bb->primary_bearer;
 	if (prim != INVALID_BEARER_ID)
-		bb->bcast_support = tipc_bearer_bcast_support(net, prim);
+		bb->bcast_support &= tipc_bearer_bcast_support(net, prim);
 }
 
 void tipc_bcast_inc_bearer_dst_cnt(struct net *net, int bearer_id)
diff --git a/net/tipc/bcast.h b/net/tipc/bcast.h
index 751530ab0c49..8880dc595c0b 100644
--- a/net/tipc/bcast.h
+++ b/net/tipc/bcast.h
@@ -91,6 +91,8 @@ int tipc_bcast_sync_rcv(struct net *net, struct tipc_link *l,
 int tipc_nl_add_bc_link(struct net *net, struct tipc_nl_msg *msg);
 int tipc_nl_bc_link_set(struct net *net, struct nlattr *attrs[]);
 int tipc_bclink_reset_stats(struct net *net);
+void tipc_bcast_set_bcast(struct net *net, bool sup);
+bool tipc_bcast_get_bcast(struct net *net);
 
 static inline void tipc_bcast_lock(struct net *net)
 {
diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index fb2c0d8f359f..8508ef168a4f 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -676,9 +676,10 @@ void tipc_bearer_stop(struct net *net)
 }
 
 /* Caller should hold rtnl_lock to protect the bearer */
-static int __tipc_nl_add_bearer(struct tipc_nl_msg *msg,
+static int __tipc_nl_add_bearer(struct net *net, struct tipc_nl_msg *msg,
 				struct tipc_bearer *bearer, int nlflags)
 {
+	bool bcast = tipc_bcast_get_bcast(net);
 	void *hdr;
 	struct nlattr *attrs;
 	struct nlattr *prop;
@@ -707,6 +708,8 @@ static int __tipc_nl_add_bearer(struct tipc_nl_msg *msg,
 	if (bearer->media->type_id == TIPC_MEDIA_TYPE_UDP)
 		if (nla_put_u32(msg->skb, TIPC_NLA_PROP_MTU, bearer->mtu))
 			goto prop_msg_full;
+	if (nla_put_u32(msg->skb, TIPC_NLA_PROP_BCAST, bcast))
+		goto prop_msg_full;
 
 	nla_nest_end(msg->skb, prop);
 
@@ -754,7 +757,7 @@ int tipc_nl_bearer_dump(struct sk_buff *skb, struct netlink_callback *cb)
 		if (!bearer)
 			continue;
 
-		err = __tipc_nl_add_bearer(&msg, bearer, NLM_F_MULTI);
+		err = __tipc_nl_add_bearer(net, &msg, bearer, NLM_F_MULTI);
 		if (err)
 			break;
 	}
@@ -802,7 +805,7 @@ int tipc_nl_bearer_get(struct sk_buff *skb, struct genl_info *info)
 		goto err_out;
 	}
 
-	err = __tipc_nl_add_bearer(&msg, bearer, 0);
+	err = __tipc_nl_add_bearer(net, &msg, bearer, 0);
 	if (err)
 		goto err_out;
 	rtnl_unlock();
@@ -1006,6 +1009,13 @@ int __tipc_nl_bearer_set(struct sk_buff *skb, struct genl_info *info)
 			tipc_node_apply_property(net, b, TIPC_NLA_PROP_MTU);
 #endif
 		}
+
+		if (props[TIPC_NLA_PROP_BCAST]) {
+			if (nla_get_u32(props[TIPC_NLA_PROP_BCAST]) & 0x1)
+				tipc_bcast_set_bcast(net, true);
+			else
+				tipc_bcast_set_bcast(net, false);
+		}
 	}
 
 	return 0;
diff --git a/net/tipc/netlink.c b/net/tipc/netlink.c
index 99ee419210ba..ca93699b30e2 100644
--- a/net/tipc/netlink.c
+++ b/net/tipc/netlink.c
@@ -110,7 +110,8 @@ const struct nla_policy tipc_nl_prop_policy[TIPC_NLA_PROP_MAX + 1] = {
 	[TIPC_NLA_PROP_UNSPEC]		= { .type = NLA_UNSPEC },
 	[TIPC_NLA_PROP_PRIO]		= { .type = NLA_U32 },
 	[TIPC_NLA_PROP_TOL]		= { .type = NLA_U32 },
-	[TIPC_NLA_PROP_WIN]		= { .type = NLA_U32 }
+	[TIPC_NLA_PROP_WIN]		= { .type = NLA_U32 },
+	[TIPC_NLA_PROP_BCAST]		= { .type = NLA_U32 }
 };
 
 const struct nla_policy tipc_nl_bearer_policy[TIPC_NLA_BEARER_MAX + 1]	= {
-- 
2.17.1