Re: [tipc-discussion] [PATCH 2/2] tipc: fix a null pointer deref

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

No need to declare a separate stack variable if you access it only once.
Better with:
 if (node == tipc_own_addr(net)) {...}

Otherwise,
Acked-by: jon


> -----Original Message-----
> From: Hoang Le <hoa...@de...>
> Sent: 21-Mar-19 09:29
> To: Jon Maloy <jon...@er...>; ma...@do...;
> yin...@wi...; tip...@li...
> Subject: [PATCH 2/2] tipc: fix a null pointer deref
> 
> In commit c55c8edafa91 ("tipc: smooth change between replicast and
> broadcast") we introduced new method to eliminate the risk of message
> reordering that happen in between different nodes.
> Unfortunately, we forgot checking at receiving side to ignore intra node.
> 
> We fix this by checking and returning if arrived message from intra node.
> 
> syzbot report:
> ==========================================================
> ========
> kasan: CONFIG_KASAN_INLINE enabled
> kasan: GPF could be caused by NULL-ptr deref or user memory access
> general protection fault: 0000 [#1] PREEMPT SMP KASAN
> CPU: 0 PID: 7820 Comm: syz-executor418 Not tainted 5.0.0+ #61 Hardware
> name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> RIP: 0010:tipc_mcast_filter_msg+0x21b/0x13d0 net/tipc/bcast.c:782
> Code: 45 c0 0f 84 39 06 00 00 48 89 5d 98 e8 ce ab a5 fa 49 8d bc
>  24 c8 00 00 00 48 b9 00 00 00 00 00 fc ff df 48 89 f8 48 c1 e8 03  <80> 3c 08 00 0f
> 85 9a 0e 00 00 49 8b 9c 24 c8 00 00 00 48 be 00 00
> RSP: 0018:ffff8880959defc8 EFLAGS: 00010202
> RAX: 0000000000000019 RBX: ffff888081258a48 RCX: dffffc0000000000
> RDX: 0000000000000000 RSI: ffffffff86cab862 RDI: 00000000000000c8
> RBP: ffff8880959df030 R08: ffff8880813d0200 R09: ffffed1015d05bc8
> R10: ffffed1015d05bc7 R11: ffff8880ae82de3b R12: 0000000000000000
> R13: 000000000000002c R14: 0000000000000000 R15: ffff888081258a48
> FS:  000000000106a880(0000) GS:ffff8880ae800000(0000)
>  knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000020001cc0 CR3: 0000000094a20000 CR4: 00000000001406f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call
> Trace:
>  tipc_sk_filter_rcv+0x182d/0x34f0 net/tipc/socket.c:2168  tipc_sk_enqueue
> net/tipc/socket.c:2254 [inline]
>  tipc_sk_rcv+0xc45/0x25a0 net/tipc/socket.c:2305
>  tipc_sk_mcast_rcv+0x724/0x1020 net/tipc/socket.c:1209
>  tipc_mcast_xmit+0x7fe/0x1200 net/tipc/bcast.c:410
>  tipc_sendmcast+0xb36/0xfc0 net/tipc/socket.c:820
>  __tipc_sendmsg+0x10df/0x18d0 net/tipc/socket.c:1358
>  tipc_sendmsg+0x53/0x80 net/tipc/socket.c:1291  sock_sendmsg_nosec
> net/socket.c:651 [inline]
>  sock_sendmsg+0xdd/0x130 net/socket.c:661
>  ___sys_sendmsg+0x806/0x930 net/socket.c:2260
>  __sys_sendmsg+0x105/0x1d0 net/socket.c:2298  __do_sys_sendmsg
> net/socket.c:2307 [inline]  __se_sys_sendmsg net/socket.c:2305 [inline]
>  __x64_sys_sendmsg+0x78/0xb0 net/socket.c:2305
>  do_syscall_64+0x103/0x610 arch/x86/entry/common.c:290
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
> RIP: 0033:0x4401c9
> 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:00007ffd887fa9d8 EFLAGS: 00000246 ORIG_RAX:
> 000000000000002e
> RAX: ffffffffffffffda RBX: 00000000004002c8 RCX: 00000000004401c9
> RDX: 0000000000000000 RSI: 0000000020002140 RDI: 0000000000000003
> RBP: 00000000006ca018 R08: 0000000000000000 R09: 00000000004002c8
> R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000401a50
> R13: 0000000000401ae0 R14: 0000000000000000 R15: 0000000000000000
> Modules linked in:
> ---[ end trace ba79875754e1708f ]---
> 
> Reported-by: syz...@sy...
> Fixes: c55c8eda ("tipc: smooth change between replicast and broadcast")
> Signed-off-by: Hoang Le <hoa...@de...>
> ---
>  net/tipc/bcast.c  | 6 +++++-
>  net/tipc/bcast.h  | 2 +-
>  net/tipc/socket.c | 2 +-
>  3 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c index
> 5264a8ff6e01..b3e6b4892425 100644
> --- a/net/tipc/bcast.c
> +++ b/net/tipc/bcast.c
> @@ -760,10 +760,11 @@ u32 tipc_bcast_get_broadcast_ratio(struct net
> *net)
>  	return bb->rc_ratio;
>  }
> 
> -void tipc_mcast_filter_msg(struct sk_buff_head *defq,
> +void tipc_mcast_filter_msg(struct net *net, struct sk_buff_head *defq,
>  			   struct sk_buff_head *inputq)
>  {
>  	struct sk_buff *skb, *_skb, *tmp;
> +	u32 self = tipc_own_addr(net);
>  	struct tipc_msg *hdr, *_hdr;
>  	bool match = false;
>  	u32 node, port;
> @@ -775,6 +776,9 @@ void tipc_mcast_filter_msg(struct sk_buff_head
> *defq,
>  		return;
> 
>  	node = msg_orignode(hdr);
> +	if (node == self)
> +		return;
> +
>  	port = msg_origport(hdr);
> 
>  	/* Has the twin SYN message already arrived ? */ diff --git
> a/net/tipc/bcast.h b/net/tipc/bcast.h index 484bde289d3a..dadad953e2be
> 100644
> --- a/net/tipc/bcast.h
> +++ b/net/tipc/bcast.h
> @@ -101,7 +101,7 @@ int tipc_bclink_reset_stats(struct net *net);
>  u32 tipc_bcast_get_broadcast_mode(struct net *net);
>  u32 tipc_bcast_get_broadcast_ratio(struct net *net);
> 
> -void tipc_mcast_filter_msg(struct sk_buff_head *defq,
> +void tipc_mcast_filter_msg(struct net *net, struct sk_buff_head *defq,
>  			   struct sk_buff_head *inputq);
> 
>  static inline void tipc_bcast_lock(struct net *net) diff --git
> a/net/tipc/socket.c b/net/tipc/socket.c index a7b3e1a070e4..8ac8ddf1e324
> 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -2166,7 +2166,7 @@ static void tipc_sk_filter_rcv(struct sock *sk, struct
> sk_buff *skb,
>  		tipc_group_filter_msg(grp, &inputq, xmitq);
> 
>  	if (unlikely(!grp) && mtyp == TIPC_MCAST_MSG)
> -		tipc_mcast_filter_msg(&tsk->mc_method.deferredq,
> &inputq);
> +		tipc_mcast_filter_msg(net, &tsk->mc_method.deferredq,
> &inputq);
> 
>  	/* Validate and add to receive buffer if there is space */
>  	while ((skb = __skb_dequeue(&inputq))) {
> --
> 2.1.4