Yes, it looks ok.
I was a little worried that 'rem' might become negative because of the
subtraction of 'cpy', but it looks safe.
Acked-by:
Jon Maloy <jma...@redhat.com>
On 6/1/20 10:19 PM, Tuong Tong Lien wrote:
Hi Jon, all,
Would you please look at this patch and give me your 'Acked-by' before I post
it?
I can see the patch from YueHaibing ("tipc: Fix NULL pointer dereference in
__tipc_sendstream()") was applied, but this patch is needed nonetheless since it
will also cover the other case as mentioned below.
BR/Tuong
-----Original Message-----
From: Tuong Tong Lien <tuong.t.l...@dektech.com.au>
Sent: Thursday, May 28, 2020 6:35 PM
To: jma...@redhat.com; ma...@donjonn.com; ying....@windriver.com;
tipc-discussion@lists.sourceforge.net
Cc: tipc-dek <tipc-...@dektech.com.au>
Subject: [RFC PATCH] tipc: fix general protection fault in sendstream()
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
in base that we can make an empty 'txq' queue after the
'tipc_msg_append()' in Nagle mode, especially in the case, syzbot tried
to send messages of zero data length!
The same crash can be reproduced even without the patch but at the link
layer when it accesses the empty queue.
The commit solves the issue by building at least one skb to go with the
socket header and optional data section that may be empty like what we
had with the 'tipc_msg_build()'.
Reported-by: syzbot+8eac6d030e7807c21...@syzkaller.appspotmail.com
Fixes: c0bceb97db9e ("tipc: add smart nagle feature")
Signed-off-by: Tuong Lien <tuong.t.l...@dektech.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 23809039dda1..604c03adabc2 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) {
prev = skb;
skb = tipc_buf_acquire(mss, GFP_KERNEL);
@@ -246,7 +246,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;
}
_______________________________________________
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion
