netdev - Re: [Patch net] tipc: fix a null pointer deref on error path

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fb73a71c-9638-29fb-70d8-e4a986277b7f@windriver.com>
Date:   Mon, 4 Dec 2017 20:43:55 +0800
From:   Ying Xue <ying.xue@...driver.com>
To:     Cong Wang <xiyou.wangcong@...il.com>, <netdev@...r.kernel.org>
CC:     <tipc-discussion@...ts.sourceforge.net>,
        Jon Maloy <jon.maloy@...csson.com>
Subject: Re: [Patch net] tipc: fix a null pointer deref on error path

On 12/04/2017 11:54 AM, Cong Wang wrote:
> In tipc_topsrv_kern_subscr() when s->tipc_conn_new() fails
> we call tipc_close_conn() to clean up, but in this case
> con->usr_data is NULL, tipc_subscrb_delete() should be skipped.
> 
> This fixes the folllowing crash:
> 
>  kasan: GPF could be caused by NULL-ptr deref or user memory access
>  general protection fault: 0000 [#1] SMP KASAN
>  Dumping ftrace buffer:
>     (ftrace buffer empty)
>  Modules linked in:
>  CPU: 0 PID: 3085 Comm: syzkaller064164 Not tainted 4.15.0-rc1+ #137
>  Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
>  task: 00000000c24413a5 task.stack: 000000005e8160b5
>  RIP: 0010:__lock_acquire+0xd55/0x47f0 kernel/locking/lockdep.c:3378
>  RSP: 0018:ffff8801cb5474a8 EFLAGS: 00010002
>  RAX: dffffc0000000000 RBX: 0000000000000000 RCX: 0000000000000000
>  RDX: 0000000000000004 RSI: 0000000000000000 RDI: ffffffff85ecb400
>  RBP: ffff8801cb547830 R08: 0000000000000001 R09: 0000000000000000
>  R10: 0000000000000000 R11: ffffffff87489d60 R12: ffff8801cd2980c0
>  R13: 0000000000000000 R14: 0000000000000001 R15: 0000000000000020
>  FS:  00000000014ee880(0000) GS:ffff8801db400000(0000) knlGS:0000000000000000
>  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>  CR2: 00007ffee2426e40 CR3: 00000001cb85a000 CR4: 00000000001406f0
>  DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>  DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>  Call Trace:
>   lock_acquire+0x1d5/0x580 kernel/locking/lockdep.c:4004
>   __raw_spin_lock_bh include/linux/spinlock_api_smp.h:135 [inline]
>   _raw_spin_lock_bh+0x31/0x40 kernel/locking/spinlock.c:175
>   spin_lock_bh include/linux/spinlock.h:320 [inline]
>   tipc_subscrb_subscrp_delete+0x8f/0x470 net/tipc/subscr.c:201
>   tipc_subscrb_delete net/tipc/subscr.c:238 [inline]
>   tipc_subscrb_release_cb+0x17/0x30 net/tipc/subscr.c:316
>   tipc_close_conn+0x171/0x270 net/tipc/server.c:204
>   tipc_topsrv_kern_subscr+0x724/0x810 net/tipc/server.c:514
>   tipc_group_create+0x702/0x9c0 net/tipc/group.c:184
>   tipc_sk_join net/tipc/socket.c:2747 [inline]
>   tipc_setsockopt+0x249/0xc10 net/tipc/socket.c:2861
>   SYSC_setsockopt net/socket.c:1851 [inline]
>   SyS_setsockopt+0x189/0x360 net/socket.c:1830
>   entry_SYSCALL_64_fastpath+0x1f/0x96
> 
> Reported-by: syzbot <syzkaller@...glegroups.com>
> Cc: Jon Maloy <jon.maloy@...csson.com>
> Cc: Ying Xue <ying.xue@...driver.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@...il.com>
> ---
>  net/tipc/subscr.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/tipc/subscr.c b/net/tipc/subscr.c
> index 251065dfd8df..2402a019b8ec 100644
> --- a/net/tipc/subscr.c
> +++ b/net/tipc/subscr.c
> @@ -235,6 +235,8 @@ static struct tipc_subscriber *tipc_subscrb_create(int conid)
>  
>  static void tipc_subscrb_delete(struct tipc_subscriber *subscriber)
>  {
> +	if (!subscriber)
> +		return;

The real root cause of this crash is because when it was failed to
create a connection through s->tipc_conn_new() in
tipc_topsrv_kern_subscr(), we just should release the connection
instance with conn_put() rather than calling tipc_close_conn() to close
the connection. So a proper fix might look like:

diff --git a/net/tipc/server.c b/net/tipc/server.c
index acaef80..9f7afad 100644
--- a/net/tipc/server.c
+++ b/net/tipc/server.c
@@ -508,15 +508,15 @@ bool tipc_topsrv_kern_subscr(struct net *net, u32
port, u32 type,
                return false;

        *conid = con->conid;
+       con->sock = NULL;
        s = con->server;
        scbr = s->tipc_conn_new(*conid);
        if (!scbr) {
-               tipc_close_conn(con);
+               conn_put(con);
                return false;
        }

        con->usr_data = scbr;
-       con->sock = NULL;
        s->tipc_conn_recvmsg(net, *conid, NULL, scbr, &sub, sizeof(sub));
        return true;
 }

>  	tipc_subscrb_subscrp_delete(subscriber, NULL);
>  	tipc_subscrb_put(subscriber);
>  }
>