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  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Tue, 21 Jul 2020 20:26:42 +0800
From:   Xin Long <lucien.xin@...il.com>
To:     Tuong Tong Lien <tuong.t.lien@...tech.com.au>
Cc:     davem <davem@...emloft.net>,
        "jmaloy@...hat.com" <jmaloy@...hat.com>,
        "maloy@...jonn.com" <maloy@...jonn.com>,
        Ying Xue <ying.xue@...driver.com>,
        network dev <netdev@...r.kernel.org>,
        "tipc-discussion@...ts.sourceforge.net" 
        <tipc-discussion@...ts.sourceforge.net>
Subject: Re: [tipc-discussion] [net-next] tipc: fix NULL pointer dereference
 in streaming

On Tue, Jul 21, 2020 at 7:26 PM Tuong Tong Lien
<tuong.t.lien@...tech.com.au> wrote:
>
>
>
> > -----Original Message-----
> > From: Xin Long <lucien.xin@...il.com>
> > Sent: Tuesday, July 21, 2020 6:23 PM
> > To: Tuong Tong Lien <tuong.t.lien@...tech.com.au>
> > Cc: davem <davem@...emloft.net>; jmaloy@...hat.com; maloy@...jonn.com; Ying Xue <ying.xue@...driver.com>; network dev
> > <netdev@...r.kernel.org>; tipc-discussion@...ts.sourceforge.net
> > Subject: Re: [tipc-discussion] [net-next] tipc: fix NULL pointer dereference in streaming
> >
> > On Wed, Jun 3, 2020 at 1:06 PM Tuong Lien <tuong.t.lien@...tech.com.au> wrote:
> > >
> > > syzbot found the following crash:
> > >
> > > general protection fault, probably for non-canonical address 0xdffffc0000000019: 0000 [#1] PREEMPT SMP KASAN
> > > KASAN: null-ptr-deref in range [0x00000000000000c8-0x00000000000000cf]
> > > CPU: 1 PID: 7060 Comm: syz-executor394 Not tainted 5.7.0-rc6-syzkaller #0
> > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> > > RIP: 0010:__tipc_sendstream+0xbde/0x11f0 net/tipc/socket.c:1591
> > > Code: 00 00 00 00 48 39 5c 24 28 48 0f 44 d8 e8 fa 3e db f9 48 b8 00 00 00 00 00 fc ff df 48 8d bb c8 00 00 00 48 89 fa 48 c1 ea 03 <80> 3c
> > 02 00 0f 85 e2 04 00 00 48 8b 9b c8 00 00 00 48 b8 00 00 00
> > > RSP: 0018:ffffc90003ef7818 EFLAGS: 00010202
> > > RAX: dffffc0000000000 RBX: 0000000000000000 RCX: ffffffff8797fd9d
> > > RDX: 0000000000000019 RSI: ffffffff8797fde6 RDI: 00000000000000c8
> > > RBP: ffff888099848040 R08: ffff88809a5f6440 R09: fffffbfff1860b4c
> > > R10: ffffffff8c305a5f R11: fffffbfff1860b4b R12: ffff88809984857e
> > > R13: 0000000000000000 R14: ffff888086aa4000 R15: 0000000000000000
> > > FS:  00000000009b4880(0000) GS:ffff8880ae700000(0000) knlGS:0000000000000000
> > > CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > CR2: 0000000020000140 CR3: 00000000a7fdf000 CR4: 00000000001406e0
> > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > > Call Trace:
> > >  tipc_sendstream+0x4c/0x70 net/tipc/socket.c:1533
> > >  sock_sendmsg_nosec net/socket.c:652 [inline]
> > >  sock_sendmsg+0xcf/0x120 net/socket.c:672
> > >  ____sys_sendmsg+0x32f/0x810 net/socket.c:2352
> > >  ___sys_sendmsg+0x100/0x170 net/socket.c:2406
> > >  __sys_sendmmsg+0x195/0x480 net/socket.c:2496
> > >  __do_sys_sendmmsg net/socket.c:2525 [inline]
> > >  __se_sys_sendmmsg net/socket.c:2522 [inline]
> > >  __x64_sys_sendmmsg+0x99/0x100 net/socket.c:2522
> > >  do_syscall_64+0xf6/0x7d0 arch/x86/entry/common.c:295
> > >  entry_SYSCALL_64_after_hwframe+0x49/0xb3
> > > RIP: 0033:0x440199
> > > ...
> > >
> > > This bug was bisected to commit 0a3e060f340d ("tipc: add test for Nagle
> > > algorithm effectiveness"). However, it is not the case, the trouble was
> > > from the base in the case of zero data length message sending, we would
> > > unexpectedly make an empty 'txq' queue after the 'tipc_msg_append()' in
> > > Nagle mode.
> > >
> > > A similar crash can be generated even without the bisected patch but at
> > > the link layer when it accesses the empty queue.
> > >
> > > We solve the issues by building at least one buffer to go with socket's
> > > header and an optional data section that may be empty like what we had
> > > with the 'tipc_msg_build()'.
> > >
> > > Note: the previous commit 4c21daae3dbc ("tipc: Fix NULL pointer
> > > dereference in __tipc_sendstream()") is obsoleted by this one since the
> > > 'txq' will be never empty and the check of 'skb != NULL' is unnecessary
> > > but it is safe anyway.
> > Hi, Tuong
> >
> > If commit 4c21daae3dbc is obsoleted by this one, can you please
> > send a patch to revert it?
> >
> > Thanks.
> Hi Xin,
>
> That patch includes a sanity check which is always true and safe, so I don’t think
> we need to revert it. Do you agree?
surely it's safe.

People may be confused when reading the code:
                                if (skb) {
                                        msg_set_ack_required(buf_msg(skb));
                                        tsk->expect_ack = true;
                                } else {
                                        tsk->expect_ack = false;  <----- [1]
                                }

like why expect_ack needs to be set to false in [1]

>
> BR/Tuong
> >
> > >
> > > Reported-by: syzbot+8eac6d030e7807c21d32@...kaller.appspotmail.com
> > > Fixes: c0bceb97db9e ("tipc: add smart nagle feature")
> > > Acked-by: Jon Maloy <jmaloy@...hat.com>
> > > Signed-off-by: Tuong Lien <tuong.t.lien@...tech.com.au>
> > > ---
> > >  net/tipc/msg.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/net/tipc/msg.c b/net/tipc/msg.c
> > > index c0afcd627c5e..046e4cb3acea 100644
> > > --- a/net/tipc/msg.c
> > > +++ b/net/tipc/msg.c
> > > @@ -221,7 +221,7 @@ int tipc_msg_append(struct tipc_msg *_hdr, struct msghdr *m, int dlen,
> > >         accounted = skb ? msg_blocks(buf_msg(skb)) : 0;
> > >         total = accounted;
> > >
> > > -       while (rem) {
> > > +       do {
> > >                 if (!skb || skb->len >= mss) {
> > >                         skb = tipc_buf_acquire(mss, GFP_KERNEL);
> > >                         if (unlikely(!skb))
> > > @@ -245,7 +245,7 @@ int tipc_msg_append(struct tipc_msg *_hdr, struct msghdr *m, int dlen,
> > >                 skb_put(skb, cpy);
> > >                 rem -= cpy;
> > >                 total += msg_blocks(hdr) - curr;
> > > -       }
> > > +       } while (rem);
> > >         return total - accounted;
> > >  }
> > >
> > > --
> > > 2.13.7
> > >
> > >
> > >
> > > _______________________________________________
> > > tipc-discussion mailing list
> > > tipc-discussion@...ts.sourceforge.net
> > > https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Powered by blists - more mailing lists