memory leak in sctp_process_init

memory leak in sctp_process_init

[syzbot]

Hello,

syzbot found the following crash on:

HEAD commit:    9c7db500 Merge tag 'selinux-pr-20190521' of git://git.kern..
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=10388530a00000
kernel config:  https://syzkaller.appspot.com/x/.config?x=61dd9e15a761691d
dashboard link: https://syzkaller.appspot.com/bug?extid=f7e9153b037eac9b1df8
compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=10e32f8ca00000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=177fa530a00000

IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+f7e9153b037eac9b1df8@syzkaller.appspotmail.com

  0 to HW filter on device batadv0
executing program
executing program
executing program
BUG: memory leak
unreferenced object 0xffff88810ef68400 (size 1024):
   comm "syz-executor273", pid 7046, jiffies 4294945598 (age 28.770s)
   hex dump (first 32 bytes):
     1d de 28 8d de 0b 1b e3 b5 c2 f9 68 fd 1a 97 25  ..(........h...%
     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
   backtrace:
     [<00000000a02cebbd>] kmemleak_alloc_recursive  
include/linux/kmemleak.h:55 [inline]
     [<00000000a02cebbd>] slab_post_alloc_hook mm/slab.h:439 [inline]
     [<00000000a02cebbd>] slab_alloc mm/slab.c:3326 [inline]
     [<00000000a02cebbd>] __do_kmalloc mm/slab.c:3658 [inline]
     [<00000000a02cebbd>] __kmalloc_track_caller+0x15d/0x2c0 mm/slab.c:3675
     [<000000009e6245e6>] kmemdup+0x27/0x60 mm/util.c:119
     [<00000000dfdc5d2d>] kmemdup include/linux/string.h:432 [inline]
     [<00000000dfdc5d2d>] sctp_process_init+0xa7e/0xc20  
net/sctp/sm_make_chunk.c:2437
     [<00000000b58b62f8>] sctp_cmd_process_init net/sctp/sm_sideeffect.c:682  
[inline]
     [<00000000b58b62f8>] sctp_cmd_interpreter net/sctp/sm_sideeffect.c:1384  
[inline]
     [<00000000b58b62f8>] sctp_side_effects net/sctp/sm_sideeffect.c:1194  
[inline]
     [<00000000b58b62f8>] sctp_do_sm+0xbdc/0x1d60  
net/sctp/sm_sideeffect.c:1165
     [<0000000044e11f96>] sctp_assoc_bh_rcv+0x13c/0x200  
net/sctp/associola.c:1074
     [<00000000ec43804d>] sctp_inq_push+0x7f/0xb0 net/sctp/inqueue.c:95
     [<00000000726aa954>] sctp_backlog_rcv+0x5e/0x2a0 net/sctp/input.c:354
     [<00000000d9e249a8>] sk_backlog_rcv include/net/sock.h:950 [inline]
     [<00000000d9e249a8>] __release_sock+0xab/0x110 net/core/sock.c:2418
     [<00000000acae44fa>] release_sock+0x37/0xd0 net/core/sock.c:2934
     [<00000000963cc9ae>] sctp_sendmsg+0x2c0/0x990 net/sctp/socket.c:2122
     [<00000000a7fc7565>] inet_sendmsg+0x64/0x120 net/ipv4/af_inet.c:802
     [<00000000b732cbd3>] sock_sendmsg_nosec net/socket.c:652 [inline]
     [<00000000b732cbd3>] sock_sendmsg+0x54/0x70 net/socket.c:671
     [<00000000274c57ab>] ___sys_sendmsg+0x393/0x3c0 net/socket.c:2292
     [<000000008252aedb>] __sys_sendmsg+0x80/0xf0 net/socket.c:2330
     [<00000000f7bf23d1>] __do_sys_sendmsg net/socket.c:2339 [inline]
     [<00000000f7bf23d1>] __se_sys_sendmsg net/socket.c:2337 [inline]
     [<00000000f7bf23d1>] __x64_sys_sendmsg+0x23/0x30 net/socket.c:2337
     [<00000000a8b4131f>] do_syscall_64+0x76/0x1a0  
arch/x86/entry/common.c:301

BUG: memory leak
unreferenced object 0xffff88810ef68400 (size 1024):
   comm "syz-executor273", pid 7046, jiffies 4294945598 (age 28.840s)
   hex dump (first 32 bytes):
     1d de 28 8d de 0b 1b e3 b5 c2 f9 68 fd 1a 97 25  ..(........h...%
     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
   backtrace:
     [<00000000a02cebbd>] kmemleak_alloc_recursive  
include/linux/kmemleak.h:55 [inline]
     [<00000000a02cebbd>] slab_post_alloc_hook mm/slab.h:439 [inline]
     [<00000000a02cebbd>] slab_alloc mm/slab.c:3326 [inline]
     [<00000000a02cebbd>] __do_kmalloc mm/slab.c:3658 [inline]
     [<00000000a02cebbd>] __kmalloc_track_caller+0x15d/0x2c0 mm/slab.c:3675
     [<000000009e6245e6>] kmemdup+0x27/0x60 mm/util.c:119
     [<00000000dfdc5d2d>] kmemdup include/linux/string.h:432 [inline]
     [<00000000dfdc5d2d>] sctp_process_init+0xa7e/0xc20  
net/sctp/sm_make_chunk.c:2437
     [<00000000b58b62f8>] sctp_cmd_process_init net/sctp/sm_sideeffect.c:682  
[inline]
     [<00000000b58b62f8>] sctp_cmd_interpreter net/sctp/sm_sideeffect.c:1384  
[inline]
     [<00000000b58b62f8>] sctp_side_effects net/sctp/sm_sideeffect.c:1194  
[inline]
     [<00000000b58b62f8>] sctp_do_sm+0xbdc/0x1d60  
net/sctp/sm_sideeffect.c:1165
     [<0000000044e11f96>] sctp_assoc_bh_rcv+0x13c/0x200  
net/sctp/associola.c:1074
     [<00000000ec43804d>] sctp_inq_push+0x7f/0xb0 net/sctp/inqueue.c:95
     [<00000000726aa954>] sctp_backlog_rcv+0x5e/0x2a0 net/sctp/input.c:354
     [<00000000d9e249a8>] sk_backlog_rcv include/net/sock.h:950 [inline]
     [<00000000d9e249a8>] __release_sock+0xab/0x110 net/core/sock.c:2418
     [<00000000acae44fa>] release_sock+0x37/0xd0 net/core/sock.c:2934
     [<00000000963cc9ae>] sctp_sendmsg+0x2c0/0x990 net/sctp/socket.c:2122
     [<00000000a7fc7565>] inet_sendmsg+0x64/0x120 net/ipv4/af_inet.c:802
     [<00000000b732cbd3>] sock_sendmsg_nosec net/socket.c:652 [inline]
     [<00000000b732cbd3>] sock_sendmsg+0x54/0x70 net/socket.c:671
     [<00000000274c57ab>] ___sys_sendmsg+0x393/0x3c0 net/socket.c:2292
     [<000000008252aedb>] __sys_sendmsg+0x80/0xf0 net/socket.c:2330
     [<00000000f7bf23d1>] __do_sys_sendmsg net/socket.c:2339 [inline]
     [<00000000f7bf23d1>] __se_sys_sendmsg net/socket.c:2337 [inline]
     [<00000000f7bf23d1>] __x64_sys_sendmsg+0x23/0x30 net/socket.c:2337
     [<00000000a8b4131f>] do_syscall_64+0x76/0x1a0  
arch/x86/entry/common.c:301

BUG: memory leak
unreferenced object 0xffff88810ef68400 (size 1024):
   comm "syz-executor273", pid 7046, jiffies 4294945598 (age 28.910s)
   hex dump (first 32 bytes):
     1d de 28 8d de 0b 1b e3 b5 c2 f9 68 fd 1a 97 25  ..(........h...%
     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
   backtrace:
     [<00000000a02cebbd>] kmemleak_alloc_recursive  
include/linux/kmemleak.h:55 [inline]
     [<00000000a02cebbd>] slab_post_alloc_hook mm/slab.h:439 [inline]
     [<00000000a02cebbd>] slab_alloc mm/slab.c:3326 [inline]
     [<00000000a02cebbd>] __do_kmalloc mm/slab.c:3658 [inline]
     [<00000000a02cebbd>] __kmalloc_track_caller+0x15d/0x2c0 mm/slab.c:3675
     [<000000009e6245e6>] kmemdup+0x27/0x60 mm/util.c:119
     [<00000000dfdc5d2d>] kmemdup include/linux/string.h:432 [inline]
     [<00000000dfdc5d2d>] sctp_process_init+0xa7e/0xc20  
net/sctp/sm_make_chunk.c:2437
     [<00000000b58b62f8>] sctp_cmd_process_init net/sctp/sm_sideeffect.c:682  
[inline]
     [<00000000b58b62f8>] sctp_cmd_interpreter net/sctp/sm_sideeffect.c:1384  
[inline]
     [<00000000b58b62f8>] sctp_side_effects net/sctp/sm_sideeffect.c:1194  
[inline]
     [<00000000b58b62f8>] sctp_do_sm+0xbdc/0x1d60  
net/sctp/sm_sideeffect.c:1165
     [<0000000044e11f96>] sctp_assoc_bh_rcv+0x13c/0x200  
net/sctp/associola.c:1074
     [<00000000ec43804d>] sctp_inq_push+0x7f/0xb0 net/sctp/inqueue.c:95
     [<00000000726aa954>] sctp_backlog_rcv+0x5e/0x2a0 net/sctp/input.c:354
     [<00000000d9e249a8>] sk_backlog_rcv include/net/sock.h:950 [inline]
     [<00000000d9e249a8>] __release_sock+0xab/0x110 net/core/sock.c:2418
     [<00000000acae44fa>] release_sock+0x37/0xd0 net/core/sock.c:2934
     [<00000000963cc9ae>] sctp_sendmsg+0x2c0/0x990 net/sctp/socket.c:2122
     [<00000000a7fc7565>] inet_sendmsg+0x64/0x120 net/ipv4/af_inet.c:802
     [<00000000b732cbd3>] sock_sendmsg_nosec net/socket.c:652 [inline]
     [<00000000b732cbd3>] sock_sendmsg+0x54/0x70 net/socket.c:671
     [<00000000274c57ab>] ___sys_sendmsg+0x393/0x3c0 net/socket.c:2292
     [<000000008252aedb>] __sys_sendmsg+0x80/0xf0 net/socket.c:2330
     [<00000000f7bf23d1>] __do_sys_sendmsg net/socket.c:2339 [inline]
     [<00000000f7bf23d1>] __se_sys_sendmsg net/socket.c:2337 [inline]
     [<00000000f7bf23d1>] __x64_sys_sendmsg+0x23/0x30 net/socket.c:2337
     [<00000000a8b4131f>] do_syscall_64+0x76/0x1a0  
arch/x86/entry/common.c:301

BUG: memory leak
unreferenced object 0xffff88810ef68400 (size 1024):
   comm "syz-executor273", pid 7046, jiffies 4294945598 (age 28.980s)
   hex dump (first 32 bytes):
     1d de 28 8d de 0b 1b e3 b5 c2 f9 68 fd 1a 97 25  ..(........h...%
     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
   backtrace:
     [<00000000a02cebbd>] kmemleak_alloc_recursive  
include/linux/kmemleak.h:55 [inline]
     [<00000000a02cebbd>] slab_post_alloc_hook mm/slab.h:439 [inline]
     [<00000000a02cebbd>] slab_alloc mm/slab.c:3326 [inline]
     [<00000000a02cebbd>] __do_kmalloc mm/slab.c:3658 [inline]
     [<00000000a02cebbd>] __kmalloc_track_caller+0x15d/0x2c0 mm/slab.c:3675
     [<000000009e6245e6>] kmemdup+0x27/0x60 mm/util.c:119
     [<00000000dfdc5d2d>] kmemdup include/linux/string.h:432 [inline]
     [<00000000dfdc5d2d>] sctp_process_init+0xa7e/0xc20  
net/sctp/sm_make_chunk.c:2437
     [<00000000b58b62f8>] sctp_cmd_process_init net/sctp/sm_sideeffect.c:682  
[inline]
     [<00000000b58b62f8>] sctp_cmd_interpreter net/sctp/sm_sideeffect.c:1384  
[inline]
     [<00000000b58b62f8>] sctp_side_effects net/sctp/sm_sideeffect.c:1194  
[inline]
     [<00000000b58b62f8>] sctp_do_sm+0xbdc/0x1d60  
net/sctp/sm_sideeffect.c:1165
     [<0000000044e11f96>] sctp_assoc_bh_rcv+0x13c/0x200  
net/sctp/associola.c:1074
     [<00000000ec43804d>] sctp_inq_push+0x7f/0xb0 net/sctp/inqueue.c:95
     [<00000000726aa954>] sctp_backlog_rcv+0x5e/0x2a0 net/sctp/input.c:354
     [<00000000d9e249a8>] sk_backlog_rcv include/net/sock.h:950 [inline]
     [<00000000d9e249a8>] __release_sock+0xab/0x110 net/core/sock.c:2418
     [<00000000acae44fa>] release_sock+0x37/0xd0 net/core/sock.c:2934
     [<00000000963cc9ae>] sctp_sendmsg+0x2c0/0x990 net/sctp/socket.c:2122
     [<00000000a7fc7565>] inet_sendmsg+0x64/0x120 net/ipv4/af_inet.c:802
     [<00000000b732cbd3>] sock_sendmsg_nosec net/socket.c:652 [inline]
     [<00000000b732cbd3>] sock_sendmsg+0x54/0x70 net/socket.c:671
     [<00000000274c57ab>] ___sys_sendmsg+0x393/0x3c0 net/socket.c:2292
     [<000000008252aedb>] __sys_sendmsg+0x80/0xf0 net/socket.c:2330
     [<00000000f7bf23d1>] __do_sys_sendmsg net/socket.c:2339 [inline]
     [<00000000f7bf23d1>] __se_sys_sendmsg net/socket.c:2337 [inline]
     [<00000000f7bf23d1>] __x64_sys_sendmsg+0x23/0x30 net/socket.c:2337
     [<00000000a8b4131f>] do_syscall_64+0x76/0x1a0  
arch/x86/entry/common.c:301

BUG: memory leak
unreferenced object 0xffff88810ef68400 (size 1024):
   comm "syz-executor273", pid 7046, jiffies 4294945598 (age 29.050s)
   hex dump (first 32 bytes):
     1d de 28 8d de 0b 1b e3 b5 c2 f9 68 fd 1a 97 25  ..(........h...%
     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
   backtrace:
     [<00000000a02cebbd>] kmemleak_alloc_recursive  
include/linux/kmemleak.h:55 [inline]
     [<00000000a02cebbd>] slab_post_alloc_hook mm/slab.h:439 [inline]
     [<00000000a02cebbd>] slab_alloc mm/slab.c:3326 [inline]
     [<00000000a02cebbd>] __do_kmalloc mm/slab.c:3658 [inline]
     [<00000000a02cebbd>] __kmalloc_track_caller+0x15d/0x2c0 mm/slab.c:3675
     [<000000009e6245e6>] kmemdup+0x27/0x60 mm/util.c:119
     [<00000000dfdc5d2d>] kmemdup include/linux/string.h:432 [inline]
     [<00000000dfdc5d2d>] sctp_process_init+0xa7e/0xc20  
net/sctp/sm_make_chunk.c:2437
     [<00000000b58b62f8>] sctp_cmd_process_init net/sctp/sm_sideeffect.c:682  
[inline]
     [<00000000b58b62f8>] sctp_cmd_interpreter net/sctp/sm_sideeffect.c:1384  
[inline]
     [<00000000b58b62f8>] sctp_side_effects net/sctp/sm_sideeffect.c:1194  
[inline]
     [<00000000b58b62f8>] sctp_do_sm+0xbdc/0x1d60  
net/sctp/sm_sideeffect.c:1165
     [<0000000044e11f96>] sctp_assoc_bh_rcv+0x13c/0x200  
net/sctp/associola.c:1074
     [<00000000ec43804d>] sctp_inq_push+0x7f/0xb0 net/sctp/inqueue.c:95
     [<00000000726aa954>] sctp_backlog_rcv+0x5e/0x2a0 net/sctp/input.c:354
     [<00000000d9e249a8>] sk_backlog_rcv include/net/sock.h:950 [inline]
     [<00000000d9e249a8>] __release_sock+0xab/0x110 net/core/sock.c:2418
     [<00000000acae44fa>] release_sock+0x37/0xd0 net/core/sock.c:2934
     [<00000000963cc9ae>] sctp_sendmsg+0x2c0/0x990 net/sctp/socket.c:2122
     [<00000000a7fc7565>] inet_sendmsg+0x64/0x120 net/ipv4/af_inet.c:802
     [<00000000b732cbd3>] sock_sendmsg_nosec net/socket.c:652 [inline]
     [<00000000b732cbd3>] sock_sendmsg+0x54/0x70 net/socket.c:671
     [<00000000274c57ab>] ___sys_sendmsg+0x393/0x3c0 net/socket.c:2292
     [<000000008252aedb>] __sys_sendmsg+0x80/0xf0 net/socket.c:2330
     [<00000000f7bf23d1>] __do_sys_sendmsg net/socket.c:2339 [inline]
     [<00000000f7bf23d1>] __se_sys_sendmsg net/socket.c:2337 [inline]
     [<00000000f7bf23d1>] __x64_sys_sendmsg+0x23/0x30 net/socket.c:2337
     [<00000000a8b4131f>] do_syscall_64+0x76/0x1a0  
arch/x86/entry/common.c:301

BUG: memory leak
unreferenced object 0xffff88810ef68400 (size 1024):
   comm "syz-executor273", pid 7046, jiffies 4294945598 (age 29.120s)
   hex dump (first 32 bytes):
     1d de 28 8d de 0b 1b e3 b5 c2 f9 68 fd 1a 97 25  ..(........h...%
     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
   backtrace:
     [<00000000a02cebbd>] kmemleak_alloc_recursive  
include/linux/kmemleak.h:55 [inline]
     [<00000000a02cebbd>] slab_post_alloc_hook mm/slab.h:439 [inline]
     [<00000000a02cebbd>] slab_alloc mm/slab.c:3326 [inline]
     [<00000000a02cebbd>] __do_kmalloc mm/slab.c:3658 [inline]
     [<00000000a02cebbd>] __kmalloc_track_caller+0x15d/0x2c0 mm/slab.c:3675
     [<000000009e6245e6>] kmemdup+0x27/0x60 mm/util.c:119
     [<00000000dfdc5d2d>] kmemdup include/linux/string.h:432 [inline]
     [<00000000dfdc5d2d>] sctp_process_init+0xa7e/0xc20  
net/sctp/sm_make_chunk.c:2437
     [<00000000b58b62f8>] sctp_cmd_process_init net/sctp/sm_sideeffect.c:682  
[inline]
     [<00000000b58b62f8>] sctp_cmd_interpreter net/sctp/sm_sideeffect.c:1384  
[inline]
     [<00000000b58b62f8>] sctp_side_effects net/sctp/sm_sideeffect.c:1194  
[inline]
     [<00000000b58b62f8>] sctp_do_sm+0xbdc/0x1d60  
net/sctp/sm_sideeffect.c:1165
     [<0000000044e11f96>] sctp_assoc_bh_rcv+0x13c/0x200  
net/sctp/associola.c:1074
     [<00000000ec43804d>] sctp_inq_push+0x7f/0xb0 net/sctp/inqueue.c:95
     [<00000000726aa954>] sctp_backlog_rcv+0x5e/0x2a0 net/sctp/input.c:354
     [<00000000d9e249a8>] sk_backlog_rcv include/net/sock.h:950 [inline]
     [<00000000d9e249a8>] __release_sock+0xab/0x110 net/core/sock.c:2418
     [<00000000acae44fa>] release_sock+0x37/0xd0 net/core/sock.c:2934
     [<00000000963cc9ae>] sctp_sendmsg+0x2c0/0x990 net/sctp/socket.c:2122
     [<00000000a7fc7565>] inet_sendmsg+0x64/0x120 net/ipv4/af_inet.c:802
     [<00000000b732cbd3>] sock_sendmsg_nosec net/socket.c:652 [inline]
     [<00000000b732cbd3>] sock_sendmsg+0x54/0x70 net/socket.c:671
     [<00000000274c57ab>] ___sys_sendmsg+0x393/0x3c0 net/socket.c:2292
     [<000000008252aedb>] __sys_sendmsg+0x80/0xf0 net/socket.c:2330
     [<00000000f7bf23d1>] __do_sys_sendmsg net/socket.c:2339 [inline]
     [<00000000f7bf23d1>] __se_sys_sendmsg net/socket.c:2337 [inline]
     [<00000000f7bf23d1>] __x64_sys_sendmsg+0x23/0x30 net/socket.c:2337
     [<00000000a8b4131f>] do_syscall_64+0x76/0x1a0  
arch/x86/entry/common.c:301

BUG: memory leak
unreferenced object 0xffff88810ef68400 (size 1024):
   comm "syz-executor273", pid 7046, jiffies 4294945598 (age 29.190s)
   hex dump (first 32 bytes):
     1d de 28 8d de 0b 1b e3 b5 c2 f9 68 fd 1a 97 25  ..(........h...%
     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
   backtrace:
     [<00000000a02cebbd>] kmemleak_alloc_recursive  
include/linux/kmemleak.h:55 [inline]
     [<00000000a02cebbd>] slab_post_alloc_hook mm/slab.h:439 [inline]
     [<00000000a02cebbd>] slab_alloc mm/slab.c:3326 [inline]
     [<00000000a02cebbd>] __do_kmalloc mm/slab.c:3658 [inline]
     [<00000000a02cebbd>] __kmalloc_track_caller+0x15d/0x2c0 mm/slab.c:3675
     [<000000009e6245e6>] kmemdup+0x27/0x60 mm/util.c:119
     [<00000000dfdc5d2d>] kmemdup include/linux/string.h:432 [inline]
     [<00000000dfdc5d2d>] sctp_process_init+0xa7e/0xc20  
net/sctp/sm_make_chunk.c:2437
     [<00000000b58b62f8>] sctp_cmd_process_init net/sctp/sm_sideeffect.c:682  
[inline]
     [<00000000b58b62f8>] sctp_cmd_interpreter net/sctp/sm_sideeffect.c:1384  
[inline]
     [<00000000b58b62f8>] sctp_side_effects net/sctp/sm_sideeffect.c:1194  
[inline]
     [<00000000b58b62f8>] sctp_do_sm+0xbdc/0x1d60  
net/sctp/sm_sideeffect.c:1165
     [<0000000044e11f96>] sctp_assoc_bh_rcv+0x13c/0x200  
net/sctp/associola.c:1074
     [<00000000ec43804d>] sctp_inq_push+0x7f/0xb0 net/sctp/inqueue.c:95
     [<00000000726aa954>] sctp_backlog_rcv+0x5e/0x2a0 net/sctp/input.c:354
     [<00000000d9e249a8>] sk_backlog_rcv include/net/sock.h:950 [inline]
     [<00000000d9e249a8>] __release_sock+0xab/0x110 net/core/sock.c:2418
     [<00000000acae44fa>] release_sock+0x37/0xd0 net/core/sock.c:2934
     [<00000000963cc9ae>] sctp_sendmsg+0x2c0/0x990 net/sctp/socket.c:2122
     [<00000000a7fc7565>] inet_sendmsg+0x64/0x120 net/ipv4/af_inet.c:802
     [<00000000b732cbd3>] sock_sendmsg_nosec net/socket.c:652 [inline]
     [<00000000b732cbd3>] sock_sendmsg+0x54/0x70 net/socket.c:671
     [<00000000274c57ab>] ___sys_sendmsg+0x393/0x3c0 net/socket.c:2292
     [<000000008252aedb>] __sys_sendmsg+0x80/0xf0 net/socket.c:2330
     [<00000000f7bf23d1>] __do_sys_sendmsg net/socket.c:2339 [inline]
     [<00000000f7bf23d1>] __se_sys_sendmsg net/socket.c:2337 [inline]
     [<00000000f7bf23d1>] __x64_sys_sendmsg+0x23/0x30 net/socket.c:2337
     [<00000000a8b4131f>] do_syscall_64+0x76/0x1a0  
arch/x86/entry/common.c:301

BUG: memory leak
unreferenced object 0xffff88810ef68400 (size 1024):
   comm "syz-executor273", pid 7046, jiffies 4294945598 (age 29.260s)
   hex dump (first 32 bytes):
     1d de 28 8d de 0b 1b e3 b5 c2 f9 68 fd 1a 97 25  ..(........h...%
     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
   backtrace:
     [<00000000a02cebbd>] kmemleak_alloc_recursive  
include/linux/kmemleak.h:55 [inline]
     [<00000000a02cebbd>] slab_post_alloc_hook mm/slab.h:439 [inline]
     [<00000000a02cebbd>] slab_alloc mm/slab.c:3326 [inline]
     [<00000000a02cebbd>] __do_kmalloc mm/slab.c:3658 [inline]
     [<00000000a02cebbd>] __kmalloc_track_caller+0x15d/0x2c0 mm/slab.c:3675
     [<000000009e6245e6>] kmemdup+0x27/0x60 mm/util.c:119
     [<00000000dfdc5d2d>] kmemdup include/linux/string.h:432 [inline]
     [<00000000dfdc5d2d>] sctp_process_init+0xa7e/0xc20  
net/sctp/sm_make_chunk.c:2437
     [<00000000b58b62f8>] sctp_cmd_process_init net/sctp/sm_sideeffect.c:682  
[inline]
     [<00000000b58b62f8>] sctp_cmd_interpreter net/sctp/sm_sideeffect.c:1384  
[inline]
     [<00000000b58b62f8>] sctp_side_effects net/sctp/sm_sideeffect.c:1194  
[inline]
     [<00000000b58b62f8>] sctp_do_sm+0xbdc/0x1d60  
net/sctp/sm_sideeffect.c:1165
     [<0000000044e11f96>] sctp_assoc_bh_rcv+0x13c/0x200  
net/sctp/associola.c:1074
     [<00000000ec43804d>] sctp_inq_push+0x7f/0xb0 net/sctp/inqueue.c:95
     [<00000000726aa954>] sctp_backlog_rcv+0x5e/0x2a0 net/sctp/input.c:354
     [<00000000d9e249a8>] sk_backlog_rcv include/net/sock.h:950 [inline]
     [<00000000d9e249a8>] __release_sock+0xab/0x110 net/core/sock.c:2418
     [<00000000acae44fa>] release_sock+0x37/0xd0 net/core/sock.c:2934
     [<00000000963cc9ae>] sctp_sendmsg+0x2c0/0x990 net/sctp/socket.c:2122
     [<00000000a7fc7565>] inet_sendmsg+0x64/0x120 net/ipv4/af_inet.c:802
     [<00000000b732cbd3>] sock_sendmsg_nosec net/socket.c:652 [inline]
     [<00000000b732cbd3>] sock_sendmsg+0x54/0x70 net/socket.c:671
     [<00000000274c57ab>] ___sys_sendmsg+0x393/0x3c0 net/socket.c:2292
     [<000000008252aedb>] __sys_sendmsg+0x80/0xf0 net/socket.c:2330
     [<00000000f7bf23d1>] __do_sys_sendmsg net/socket.c:2339 [inline]
     [<00000000f7bf23d1>] __se_sys_sendmsg net/socket.c:2337 [inline]
     [<00000000f7bf23d1>] __x64_sys_sendmsg+0x23/0x30 net/socket.c:2337
     [<00000000a8b4131f>] do_syscall_64+0x76/0x1a0  
arch/x86/entry/common.c:301



---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches

Re: memory leak in sctp_process_init

[Marcelo Ricardo Leitner]

On Mon, May 27, 2019 at 05:48:06PM -0700, syzbot wrote:
> Hello,
> 
> syzbot found the following crash on:
> 
> HEAD commit:    9c7db500 Merge tag 'selinux-pr-20190521' of git://git.kern..
> git tree:       upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=10388530a00000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=61dd9e15a761691d
> dashboard link: https://syzkaller.appspot.com/bug?extid=f7e9153b037eac9b1df8
> compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=10e32f8ca00000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=177fa530a00000
> 
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+f7e9153b037eac9b1df8@syzkaller.appspotmail.com
> 
>  0 to HW filter on device batadv0
> executing program
> executing program
> executing program
> BUG: memory leak
> unreferenced object 0xffff88810ef68400 (size 1024):
>   comm "syz-executor273", pid 7046, jiffies 4294945598 (age 28.770s)
>   hex dump (first 32 bytes):
>     1d de 28 8d de 0b 1b e3 b5 c2 f9 68 fd 1a 97 25  ..(........h...%
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   backtrace:
>     [<00000000a02cebbd>] kmemleak_alloc_recursive
> include/linux/kmemleak.h:55 [inline]
>     [<00000000a02cebbd>] slab_post_alloc_hook mm/slab.h:439 [inline]
>     [<00000000a02cebbd>] slab_alloc mm/slab.c:3326 [inline]
>     [<00000000a02cebbd>] __do_kmalloc mm/slab.c:3658 [inline]
>     [<00000000a02cebbd>] __kmalloc_track_caller+0x15d/0x2c0 mm/slab.c:3675
>     [<000000009e6245e6>] kmemdup+0x27/0x60 mm/util.c:119
>     [<00000000dfdc5d2d>] kmemdup include/linux/string.h:432 [inline]
>     [<00000000dfdc5d2d>] sctp_process_init+0xa7e/0xc20
> net/sctp/sm_make_chunk.c:2437
>     [<00000000b58b62f8>] sctp_cmd_process_init net/sctp/sm_sideeffect.c:682
> [inline]
>     [<00000000b58b62f8>] sctp_cmd_interpreter net/sctp/sm_sideeffect.c:1384
> [inline]
>     [<00000000b58b62f8>] sctp_side_effects net/sctp/sm_sideeffect.c:1194
> [inline]
>     [<00000000b58b62f8>] sctp_do_sm+0xbdc/0x1d60
> net/sctp/sm_sideeffect.c:1165

Note that this is on the client side. It was handling the INIT_ACK
chunk, from sctp_sf_do_5_1C_ack().

I'm not seeing anything else other than sctp_association_free()
releasing this memory. This means 2 things:
- Every time the cookie is retransmitted, it leaks. As shown by the
  repetitive leaks here.
- The cookie remains allocated throughout the association, which is
  also not good as that's a 1k that we could have released back to the
  system right after the handshake.

  Marcelo

Re: memory leak in sctp_process_init

[Neil Horman]

On Mon, May 27, 2019 at 10:36:00PM -0300, Marcelo Ricardo Leitner wrote:
> On Mon, May 27, 2019 at 05:48:06PM -0700, syzbot wrote:
> > Hello,
> > 
> > syzbot found the following crash on:
> > 
> > HEAD commit:    9c7db500 Merge tag 'selinux-pr-20190521' of git://git.kern..
> > git tree:       upstream
> > console output: https://syzkaller.appspot.com/x/log.txt?x=10388530a00000
> > kernel config:  https://syzkaller.appspot.com/x/.config?x=61dd9e15a761691d
> > dashboard link: https://syzkaller.appspot.com/bug?extid=f7e9153b037eac9b1df8
> > compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
> > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=10e32f8ca00000
> > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=177fa530a00000
> > 
> > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > Reported-by: syzbot+f7e9153b037eac9b1df8@syzkaller.appspotmail.com
> > 
> >  0 to HW filter on device batadv0
> > executing program
> > executing program
> > executing program
> > BUG: memory leak
> > unreferenced object 0xffff88810ef68400 (size 1024):
> >   comm "syz-executor273", pid 7046, jiffies 4294945598 (age 28.770s)
> >   hex dump (first 32 bytes):
> >     1d de 28 8d de 0b 1b e3 b5 c2 f9 68 fd 1a 97 25  ..(........h...%
> >     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
> >   backtrace:
> >     [<00000000a02cebbd>] kmemleak_alloc_recursive
> > include/linux/kmemleak.h:55 [inline]
> >     [<00000000a02cebbd>] slab_post_alloc_hook mm/slab.h:439 [inline]
> >     [<00000000a02cebbd>] slab_alloc mm/slab.c:3326 [inline]
> >     [<00000000a02cebbd>] __do_kmalloc mm/slab.c:3658 [inline]
> >     [<00000000a02cebbd>] __kmalloc_track_caller+0x15d/0x2c0 mm/slab.c:3675
> >     [<000000009e6245e6>] kmemdup+0x27/0x60 mm/util.c:119
> >     [<00000000dfdc5d2d>] kmemdup include/linux/string.h:432 [inline]
> >     [<00000000dfdc5d2d>] sctp_process_init+0xa7e/0xc20
> > net/sctp/sm_make_chunk.c:2437
> >     [<00000000b58b62f8>] sctp_cmd_process_init net/sctp/sm_sideeffect.c:682
> > [inline]
> >     [<00000000b58b62f8>] sctp_cmd_interpreter net/sctp/sm_sideeffect.c:1384
> > [inline]
> >     [<00000000b58b62f8>] sctp_side_effects net/sctp/sm_sideeffect.c:1194
> > [inline]
> >     [<00000000b58b62f8>] sctp_do_sm+0xbdc/0x1d60
> > net/sctp/sm_sideeffect.c:1165
> 
> Note that this is on the client side. It was handling the INIT_ACK
> chunk, from sctp_sf_do_5_1C_ack().
> 
> I'm not seeing anything else other than sctp_association_free()
> releasing this memory. This means 2 things:
> - Every time the cookie is retransmitted, it leaks. As shown by the
>   repetitive leaks here.
> - The cookie remains allocated throughout the association, which is
>   also not good as that's a 1k that we could have released back to the
>   system right after the handshake.
> 
>   Marcelo
> 
If we have an INIT chunk bundled with a COOKIE_ECHO chunk in the same packet,
this might occur.  Processing for each chunk (via sctp_cmd_process_init and
sctp_sf_do_5_1D_ce both call sctp_process_init, which would cause a second write
to asoc->peer.cookie, leaving the first write (set via kmemdup), to be orphaned
and leak.  Seems like we should set a flag to determine if we've already cloned
the cookie, and free the old one if its set.  If we wanted to do that on the
cheap, we might be able to get away with checking asoc->stream->[in|out]cnt for
being non-zero as an indicator if we've already cloned the cookie

Neil

Re: memory leak in sctp_process_init

[Neil Horman]

On Tue, May 28, 2019 at 07:15:50AM -0400, Neil Horman wrote:
> On Mon, May 27, 2019 at 10:36:00PM -0300, Marcelo Ricardo Leitner wrote:
> > On Mon, May 27, 2019 at 05:48:06PM -0700, syzbot wrote:
> > > Hello,
> > > 
> > > syzbot found the following crash on:
> > > 
> > > HEAD commit:    9c7db500 Merge tag 'selinux-pr-20190521' of git://git.kern..
> > > git tree:       upstream
> > > console output: https://syzkaller.appspot.com/x/log.txt?x=10388530a00000
> > > kernel config:  https://syzkaller.appspot.com/x/.config?x=61dd9e15a761691d
> > > dashboard link: https://syzkaller.appspot.com/bug?extid=f7e9153b037eac9b1df8
> > > compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
> > > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=10e32f8ca00000
> > > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=177fa530a00000
> > > 
> > > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > > Reported-by: syzbot+f7e9153b037eac9b1df8@syzkaller.appspotmail.com
> > > 
> > >  0 to HW filter on device batadv0
> > > executing program
> > > executing program
> > > executing program
> > > BUG: memory leak
> > > unreferenced object 0xffff88810ef68400 (size 1024):
> > >   comm "syz-executor273", pid 7046, jiffies 4294945598 (age 28.770s)
> > >   hex dump (first 32 bytes):
> > >     1d de 28 8d de 0b 1b e3 b5 c2 f9 68 fd 1a 97 25  ..(........h...%
> > >     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
> > >   backtrace:
> > >     [<00000000a02cebbd>] kmemleak_alloc_recursive
> > > include/linux/kmemleak.h:55 [inline]
> > >     [<00000000a02cebbd>] slab_post_alloc_hook mm/slab.h:439 [inline]
> > >     [<00000000a02cebbd>] slab_alloc mm/slab.c:3326 [inline]
> > >     [<00000000a02cebbd>] __do_kmalloc mm/slab.c:3658 [inline]
> > >     [<00000000a02cebbd>] __kmalloc_track_caller+0x15d/0x2c0 mm/slab.c:3675
> > >     [<000000009e6245e6>] kmemdup+0x27/0x60 mm/util.c:119
> > >     [<00000000dfdc5d2d>] kmemdup include/linux/string.h:432 [inline]
> > >     [<00000000dfdc5d2d>] sctp_process_init+0xa7e/0xc20
> > > net/sctp/sm_make_chunk.c:2437
> > >     [<00000000b58b62f8>] sctp_cmd_process_init net/sctp/sm_sideeffect.c:682
> > > [inline]
> > >     [<00000000b58b62f8>] sctp_cmd_interpreter net/sctp/sm_sideeffect.c:1384
> > > [inline]
> > >     [<00000000b58b62f8>] sctp_side_effects net/sctp/sm_sideeffect.c:1194
> > > [inline]
> > >     [<00000000b58b62f8>] sctp_do_sm+0xbdc/0x1d60
> > > net/sctp/sm_sideeffect.c:1165
> > 
> > Note that this is on the client side. It was handling the INIT_ACK
> > chunk, from sctp_sf_do_5_1C_ack().
> > 
> > I'm not seeing anything else other than sctp_association_free()
> > releasing this memory. This means 2 things:
> > - Every time the cookie is retransmitted, it leaks. As shown by the
> >   repetitive leaks here.
> > - The cookie remains allocated throughout the association, which is
> >   also not good as that's a 1k that we could have released back to the
> >   system right after the handshake.
> > 
> >   Marcelo
> > 
> If we have an INIT chunk bundled with a COOKIE_ECHO chunk in the same packet,
> this might occur.  Processing for each chunk (via sctp_cmd_process_init and
> sctp_sf_do_5_1D_ce both call sctp_process_init, which would cause a second write
> to asoc->peer.cookie, leaving the first write (set via kmemdup), to be orphaned
> and leak.  Seems like we should set a flag to determine if we've already cloned
> the cookie, and free the old one if its set.  If we wanted to do that on the
> cheap, we might be able to get away with checking asoc->stream->[in|out]cnt for
> being non-zero as an indicator if we've already cloned the cookie
> 
> Neil
> 
> 

Completely untested, but can you give this patch a shot?


diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 0767701ef362..a5772d72eb87 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -1701,6 +1701,7 @@ struct sctp_association {
 		__u8    sack_needed:1,     /* Do we need to sack the peer? */
 			sack_generation:1,
 			zero_window_announced:1;
+			cookie_allocated:1
 		__u32	sack_cnt;
 
 		__u32   adaptation_ind;	 /* Adaptation Code point. */
diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 1999237ce481..b6e8fd7081b7 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -213,6 +213,7 @@ static struct sctp_association *sctp_association_init(
 	 */
 	asoc->peer.sack_needed = 1;
 	asoc->peer.sack_generation = 1;
+	asoc->cookie_allocated=0;
 
 	/* Assume that the peer will tell us if he recognizes ASCONF
 	 * as part of INIT exchange.
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index 92331e1195c1..e966a3cc78bf 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -2419,9 +2419,12 @@ int sctp_process_init(struct sctp_association *asoc, struct sctp_chunk *chunk,
 	/* Copy cookie in case we need to resend COOKIE-ECHO. */
 	cookie = asoc->peer.cookie;
 	if (cookie) {
+		if (asoc->peer.cookie_allocated)
+			kfree(cookie);
 		asoc->peer.cookie = kmemdup(cookie, asoc->peer.cookie_len, gfp);
 		if (!asoc->peer.cookie)
 			goto clean_up;
+		asoc->peer.cookie_allocated=1;
 	}
 
 	/* RFC 2960 7.2.1 The initial value of ssthresh MAY be arbitrarily

Re: memory leak in sctp_process_init

[Marcelo Ricardo Leitner]

On Wed, May 29, 2019 at 03:07:09PM -0400, Neil Horman wrote:
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -2419,9 +2419,12 @@ int sctp_process_init(struct sctp_association *asoc, struct sctp_chunk *chunk,
>  	/* Copy cookie in case we need to resend COOKIE-ECHO. */
>  	cookie = asoc->peer.cookie;
>  	if (cookie) {
> +		if (asoc->peer.cookie_allocated)
> +			kfree(cookie);
>  		asoc->peer.cookie = kmemdup(cookie, asoc->peer.cookie_len, gfp);
>  		if (!asoc->peer.cookie)
>  			goto clean_up;
> +		asoc->peer.cookie_allocated=1;
>  	}
>  
>  	/* RFC 2960 7.2.1 The initial value of ssthresh MAY be arbitrarily

What if we kmemdup directly at sctp_process_param(), as it's done for
others already? Like SCTP_PARAM_RANDOM and SCTP_PARAM_HMAC_ALGO. I
don't see a reason for SCTP_PARAM_STATE_COOKIE to be different
here. This way it would be always allocated, and ready to be kfreed.

We still need to free it after the handshake, btw.

  Marcelo

Re: memory leak in sctp_process_init

[Neil Horman]

On Wed, May 29, 2019 at 08:37:57PM -0300, Marcelo Ricardo Leitner wrote:
> On Wed, May 29, 2019 at 03:07:09PM -0400, Neil Horman wrote:
> > --- a/net/sctp/sm_make_chunk.c
> > +++ b/net/sctp/sm_make_chunk.c
> > @@ -2419,9 +2419,12 @@ int sctp_process_init(struct sctp_association *asoc, struct sctp_chunk *chunk,
> >  	/* Copy cookie in case we need to resend COOKIE-ECHO. */
> >  	cookie = asoc->peer.cookie;
> >  	if (cookie) {
> > +		if (asoc->peer.cookie_allocated)
> > +			kfree(cookie);
> >  		asoc->peer.cookie = kmemdup(cookie, asoc->peer.cookie_len, gfp);
> >  		if (!asoc->peer.cookie)
> >  			goto clean_up;
> > +		asoc->peer.cookie_allocated=1;
> >  	}
> >  
> >  	/* RFC 2960 7.2.1 The initial value of ssthresh MAY be arbitrarily
> 
> What if we kmemdup directly at sctp_process_param(), as it's done for
> others already? Like SCTP_PARAM_RANDOM and SCTP_PARAM_HMAC_ALGO. I
> don't see a reason for SCTP_PARAM_STATE_COOKIE to be different
> here. This way it would be always allocated, and ready to be kfreed.
> 
> We still need to free it after the handshake, btw.
> 
Yeah, that makes sense, I'll give that a shot.
Neil

>   Marcelo
> 

Re: memory leak in sctp_process_init

[Neil Horman]

On Wed, May 29, 2019 at 08:37:57PM -0300, Marcelo Ricardo Leitner wrote:
> On Wed, May 29, 2019 at 03:07:09PM -0400, Neil Horman wrote:
> > --- a/net/sctp/sm_make_chunk.c
> > +++ b/net/sctp/sm_make_chunk.c
> > @@ -2419,9 +2419,12 @@ int sctp_process_init(struct sctp_association *asoc, struct sctp_chunk *chunk,
> >  	/* Copy cookie in case we need to resend COOKIE-ECHO. */
> >  	cookie = asoc->peer.cookie;
> >  	if (cookie) {
> > +		if (asoc->peer.cookie_allocated)
> > +			kfree(cookie);
> >  		asoc->peer.cookie = kmemdup(cookie, asoc->peer.cookie_len, gfp);
> >  		if (!asoc->peer.cookie)
> >  			goto clean_up;
> > +		asoc->peer.cookie_allocated=1;
> >  	}
> >  
> >  	/* RFC 2960 7.2.1 The initial value of ssthresh MAY be arbitrarily
> 
> What if we kmemdup directly at sctp_process_param(), as it's done for
> others already? Like SCTP_PARAM_RANDOM and SCTP_PARAM_HMAC_ALGO. I
> don't see a reason for SCTP_PARAM_STATE_COOKIE to be different
> here. This way it would be always allocated, and ready to be kfreed.
> 
> We still need to free it after the handshake, btw.
> 
>   Marcelo
> 

Still untested, but something like this?


diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index d2c7d0d2abc1..718b9917844e 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -393,6 +393,7 @@ void sctp_association_free(struct sctp_association *asoc)
 	kfree(asoc->peer.peer_random);
 	kfree(asoc->peer.peer_chunks);
 	kfree(asoc->peer.peer_hmacs);
+	kfree(asoc->peer.cookie);
 
 	/* Release the transport structures. */
 	list_for_each_safe(pos, temp, &asoc->peer.transport_addr_list) {
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index 72e74503f9fc..ff365f22a3c1 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -2431,14 +2431,6 @@ int sctp_process_init(struct sctp_association *asoc, struct sctp_chunk *chunk,
 	/* Peer Rwnd   : Current calculated value of the peer's rwnd.  */
 	asoc->peer.rwnd = asoc->peer.i.a_rwnd;
 
-	/* Copy cookie in case we need to resend COOKIE-ECHO. */
-	cookie = asoc->peer.cookie;
-	if (cookie) {
-		asoc->peer.cookie = kmemdup(cookie, asoc->peer.cookie_len, gfp);
-		if (!asoc->peer.cookie)
-			goto clean_up;
-	}
-
 	/* RFC 2960 7.2.1 The initial value of ssthresh MAY be arbitrarily
 	 * high (for example, implementations MAY use the size of the receiver
 	 * advertised window).
@@ -2607,7 +2599,9 @@ static int sctp_process_param(struct sctp_association *asoc,
 	case SCTP_PARAM_STATE_COOKIE:
 		asoc->peer.cookie_len =
 			ntohs(param.p->length) - sizeof(struct sctp_paramhdr);
-		asoc->peer.cookie = param.cookie->body;
+		asoc->peer.cookie = kmemdup(param.cookie->body, asoc->peer.cookie_len, gfp);
+		if (!asoc->peer.cookie)
+			retval = 0;
 		break;
 
 	case SCTP_PARAM_HEARTBEAT_INFO:

Re: memory leak in sctp_process_init

[Marcelo Ricardo Leitner]

On Thu, May 30, 2019 at 10:20:11AM -0400, Neil Horman wrote:
> On Wed, May 29, 2019 at 08:37:57PM -0300, Marcelo Ricardo Leitner wrote:
> > On Wed, May 29, 2019 at 03:07:09PM -0400, Neil Horman wrote:
> > > --- a/net/sctp/sm_make_chunk.c
> > > +++ b/net/sctp/sm_make_chunk.c
> > > @@ -2419,9 +2419,12 @@ int sctp_process_init(struct sctp_association *asoc, struct sctp_chunk *chunk,
> > >  	/* Copy cookie in case we need to resend COOKIE-ECHO. */
> > >  	cookie = asoc->peer.cookie;
> > >  	if (cookie) {
> > > +		if (asoc->peer.cookie_allocated)
> > > +			kfree(cookie);
> > >  		asoc->peer.cookie = kmemdup(cookie, asoc->peer.cookie_len, gfp);
> > >  		if (!asoc->peer.cookie)
> > >  			goto clean_up;
> > > +		asoc->peer.cookie_allocated=1;
> > >  	}
> > >  
> > >  	/* RFC 2960 7.2.1 The initial value of ssthresh MAY be arbitrarily
> > 
> > What if we kmemdup directly at sctp_process_param(), as it's done for
> > others already? Like SCTP_PARAM_RANDOM and SCTP_PARAM_HMAC_ALGO. I
> > don't see a reason for SCTP_PARAM_STATE_COOKIE to be different
> > here. This way it would be always allocated, and ready to be kfreed.
> > 
> > We still need to free it after the handshake, btw.
> > 
> >   Marcelo
> > 
> 
> Still untested, but something like this?
> 

Yes, just..

> 
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index d2c7d0d2abc1..718b9917844e 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -393,6 +393,7 @@ void sctp_association_free(struct sctp_association *asoc)
>  	kfree(asoc->peer.peer_random);
>  	kfree(asoc->peer.peer_chunks);
>  	kfree(asoc->peer.peer_hmacs);
> +	kfree(asoc->peer.cookie);

this chunk is not needed because it is freed right above the first
kfree() in the context here.

>  
>  	/* Release the transport structures. */
>  	list_for_each_safe(pos, temp, &asoc->peer.transport_addr_list) {
> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index 72e74503f9fc..ff365f22a3c1 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -2431,14 +2431,6 @@ int sctp_process_init(struct sctp_association *asoc, struct sctp_chunk *chunk,
>  	/* Peer Rwnd   : Current calculated value of the peer's rwnd.  */
>  	asoc->peer.rwnd = asoc->peer.i.a_rwnd;
>  
> -	/* Copy cookie in case we need to resend COOKIE-ECHO. */
> -	cookie = asoc->peer.cookie;
> -	if (cookie) {
> -		asoc->peer.cookie = kmemdup(cookie, asoc->peer.cookie_len, gfp);
> -		if (!asoc->peer.cookie)
> -			goto clean_up;
> -	}
> -
>  	/* RFC 2960 7.2.1 The initial value of ssthresh MAY be arbitrarily
>  	 * high (for example, implementations MAY use the size of the receiver
>  	 * advertised window).
> @@ -2607,7 +2599,9 @@ static int sctp_process_param(struct sctp_association *asoc,
>  	case SCTP_PARAM_STATE_COOKIE:
>  		asoc->peer.cookie_len =
>  			ntohs(param.p->length) - sizeof(struct sctp_paramhdr);
> -		asoc->peer.cookie = param.cookie->body;
> +		asoc->peer.cookie = kmemdup(param.cookie->body, asoc->peer.cookie_len, gfp);
> +		if (!asoc->peer.cookie)
> +			retval = 0;
>  		break;
>  
>  	case SCTP_PARAM_HEARTBEAT_INFO:

Plus:

--- a/net/sctp/sm_sideeffect.c
+++ b/net/sctp/sm_sideeffect.c
@@ -898,6 +898,11 @@ static void sctp_cmd_new_state(struct sctp_cmd_seq *cmds,
 						asoc->rto_initial;
 	}
 
+	if (sctp_state(asoc, ESTABLISHED)) {
+		kfree(asoc->peer.cookie);
+		asoc->peer.cookie = NULL;
+	}
+
 	if (sctp_state(asoc, ESTABLISHED) ||
 	    sctp_state(asoc, CLOSED) ||
 	    sctp_state(asoc, SHUTDOWN_RECEIVED)) {

Also untested, just sharing the idea.

  Marcelo

Re: memory leak in sctp_process_init

[Neil Horman]

On Thu, May 30, 2019 at 12:17:05PM -0300, Marcelo Ricardo Leitner wrote:
> On Thu, May 30, 2019 at 10:20:11AM -0400, Neil Horman wrote:
> > On Wed, May 29, 2019 at 08:37:57PM -0300, Marcelo Ricardo Leitner wrote:
> > > On Wed, May 29, 2019 at 03:07:09PM -0400, Neil Horman wrote:
> > > > --- a/net/sctp/sm_make_chunk.c
> > > > +++ b/net/sctp/sm_make_chunk.c
> > > > @@ -2419,9 +2419,12 @@ int sctp_process_init(struct sctp_association *asoc, struct sctp_chunk *chunk,
> > > >  	/* Copy cookie in case we need to resend COOKIE-ECHO. */
> > > >  	cookie = asoc->peer.cookie;
> > > >  	if (cookie) {
> > > > +		if (asoc->peer.cookie_allocated)
> > > > +			kfree(cookie);
> > > >  		asoc->peer.cookie = kmemdup(cookie, asoc->peer.cookie_len, gfp);
> > > >  		if (!asoc->peer.cookie)
> > > >  			goto clean_up;
> > > > +		asoc->peer.cookie_allocated=1;
> > > >  	}
> > > >  
> > > >  	/* RFC 2960 7.2.1 The initial value of ssthresh MAY be arbitrarily
> > > 
> > > What if we kmemdup directly at sctp_process_param(), as it's done for
> > > others already? Like SCTP_PARAM_RANDOM and SCTP_PARAM_HMAC_ALGO. I
> > > don't see a reason for SCTP_PARAM_STATE_COOKIE to be different
> > > here. This way it would be always allocated, and ready to be kfreed.
> > > 
> > > We still need to free it after the handshake, btw.
> > > 
> > >   Marcelo
> > > 
> > 
> > Still untested, but something like this?
> > 
> 
> Yes, just..
> 
> > 
> > diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> > index d2c7d0d2abc1..718b9917844e 100644
> > --- a/net/sctp/associola.c
> > +++ b/net/sctp/associola.c
> > @@ -393,6 +393,7 @@ void sctp_association_free(struct sctp_association *asoc)
> >  	kfree(asoc->peer.peer_random);
> >  	kfree(asoc->peer.peer_chunks);
> >  	kfree(asoc->peer.peer_hmacs);
> > +	kfree(asoc->peer.cookie);
> 
> this chunk is not needed because it is freed right above the first
> kfree() in the context here.
> 
Ah, thanks, missed that.

> >  
> >  	/* Release the transport structures. */
> >  	list_for_each_safe(pos, temp, &asoc->peer.transport_addr_list) {
> > diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> > index 72e74503f9fc..ff365f22a3c1 100644
> > --- a/net/sctp/sm_make_chunk.c
> > +++ b/net/sctp/sm_make_chunk.c
> > @@ -2431,14 +2431,6 @@ int sctp_process_init(struct sctp_association *asoc, struct sctp_chunk *chunk,
> >  	/* Peer Rwnd   : Current calculated value of the peer's rwnd.  */
> >  	asoc->peer.rwnd = asoc->peer.i.a_rwnd;
> >  
> > -	/* Copy cookie in case we need to resend COOKIE-ECHO. */
> > -	cookie = asoc->peer.cookie;
> > -	if (cookie) {
> > -		asoc->peer.cookie = kmemdup(cookie, asoc->peer.cookie_len, gfp);
> > -		if (!asoc->peer.cookie)
> > -			goto clean_up;
> > -	}
> > -
> >  	/* RFC 2960 7.2.1 The initial value of ssthresh MAY be arbitrarily
> >  	 * high (for example, implementations MAY use the size of the receiver
> >  	 * advertised window).
> > @@ -2607,7 +2599,9 @@ static int sctp_process_param(struct sctp_association *asoc,
> >  	case SCTP_PARAM_STATE_COOKIE:
> >  		asoc->peer.cookie_len =
> >  			ntohs(param.p->length) - sizeof(struct sctp_paramhdr);
> > -		asoc->peer.cookie = param.cookie->body;
> > +		asoc->peer.cookie = kmemdup(param.cookie->body, asoc->peer.cookie_len, gfp);
> > +		if (!asoc->peer.cookie)
> > +			retval = 0;
> >  		break;
> >  
> >  	case SCTP_PARAM_HEARTBEAT_INFO:
> 
> Plus:
> 
> --- a/net/sctp/sm_sideeffect.c
> +++ b/net/sctp/sm_sideeffect.c
> @@ -898,6 +898,11 @@ static void sctp_cmd_new_state(struct sctp_cmd_seq *cmds,
>  						asoc->rto_initial;
>  	}
>  
> +	if (sctp_state(asoc, ESTABLISHED)) {
> +		kfree(asoc->peer.cookie);
> +		asoc->peer.cookie = NULL;
> +	}
> +
Not sure I follow why this is needed.  It doesn't hurt anything of course, but
if we're freeing in sctp_association_free, we don't need to duplicate the
operation here, do we?
>  	if (sctp_state(asoc, ESTABLISHED) ||
>  	    sctp_state(asoc, CLOSED) ||
>  	    sctp_state(asoc, SHUTDOWN_RECEIVED)) {
> 
> Also untested, just sharing the idea.
> 
>   Marcelo
> 

Re: memory leak in sctp_process_init

[Marcelo Ricardo Leitner]

On Thu, May 30, 2019 at 03:56:34PM -0400, Neil Horman wrote:
> On Thu, May 30, 2019 at 12:17:05PM -0300, Marcelo Ricardo Leitner wrote:
...
> > --- a/net/sctp/sm_sideeffect.c
> > +++ b/net/sctp/sm_sideeffect.c
> > @@ -898,6 +898,11 @@ static void sctp_cmd_new_state(struct sctp_cmd_seq *cmds,
> >  						asoc->rto_initial;
> >  	}
> >  
> > +	if (sctp_state(asoc, ESTABLISHED)) {
> > +		kfree(asoc->peer.cookie);
> > +		asoc->peer.cookie = NULL;
> > +	}
> > +
> Not sure I follow why this is needed.  It doesn't hurt anything of course, but
> if we're freeing in sctp_association_free, we don't need to duplicate the
> operation here, do we?

This one would be to avoid storing the cookie throughout the entire
association lifetime, as the cookie is only needed during the
handshake.
While the free in sctp_association_free will handle the freeing in
case the association never enters established state.

> >  	if (sctp_state(asoc, ESTABLISHED) ||
> >  	    sctp_state(asoc, CLOSED) ||
> >  	    sctp_state(asoc, SHUTDOWN_RECEIVED)) {
> > 
> > Also untested, just sharing the idea.
> > 
> >   Marcelo
> > 

Re: memory leak in sctp_process_init

[Neil Horman]

On Fri, May 31, 2019 at 09:42:42AM -0300, Marcelo Ricardo Leitner wrote:
> On Thu, May 30, 2019 at 03:56:34PM -0400, Neil Horman wrote:
> > On Thu, May 30, 2019 at 12:17:05PM -0300, Marcelo Ricardo Leitner wrote:
> ...
> > > --- a/net/sctp/sm_sideeffect.c
> > > +++ b/net/sctp/sm_sideeffect.c
> > > @@ -898,6 +898,11 @@ static void sctp_cmd_new_state(struct sctp_cmd_seq *cmds,
> > >  						asoc->rto_initial;
> > >  	}
> > >  
> > > +	if (sctp_state(asoc, ESTABLISHED)) {
> > > +		kfree(asoc->peer.cookie);
> > > +		asoc->peer.cookie = NULL;
> > > +	}
> > > +
> > Not sure I follow why this is needed.  It doesn't hurt anything of course, but
> > if we're freeing in sctp_association_free, we don't need to duplicate the
> > operation here, do we?
> 
> This one would be to avoid storing the cookie throughout the entire
> association lifetime, as the cookie is only needed during the
> handshake.
> While the free in sctp_association_free will handle the freeing in
> case the association never enters established state.
> 

Ok, I see we do that with the peer_random and other allocated values as well
when they are no longer needed, but ew, I hate freeing in multiple places like
that.  I'll fix this up on monday, but I wonder if we can't consolidate that
somehow

Neil

> > >  	if (sctp_state(asoc, ESTABLISHED) ||
> > >  	    sctp_state(asoc, CLOSED) ||
> > >  	    sctp_state(asoc, SHUTDOWN_RECEIVED)) {
> > > 
> > > Also untested, just sharing the idea.
> > > 
> > >   Marcelo
> > > 
> 

[PATCH V2] Fix memory leak in sctp_process_init

[Neil Horman]

syzbot found the following leak in sctp_process_init
BUG: memory leak
unreferenced object 0xffff88810ef68400 (size 1024):
  comm "syz-executor273", pid 7046, jiffies 4294945598 (age 28.770s)
  hex dump (first 32 bytes):
    1d de 28 8d de 0b 1b e3 b5 c2 f9 68 fd 1a 97 25  ..(........h...%
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<00000000a02cebbd>] kmemleak_alloc_recursive include/linux/kmemleak.h:55
[inline]
    [<00000000a02cebbd>] slab_post_alloc_hook mm/slab.h:439 [inline]
    [<00000000a02cebbd>] slab_alloc mm/slab.c:3326 [inline]
    [<00000000a02cebbd>] __do_kmalloc mm/slab.c:3658 [inline]
    [<00000000a02cebbd>] __kmalloc_track_caller+0x15d/0x2c0 mm/slab.c:3675
    [<000000009e6245e6>] kmemdup+0x27/0x60 mm/util.c:119
    [<00000000dfdc5d2d>] kmemdup include/linux/string.h:432 [inline]
    [<00000000dfdc5d2d>] sctp_process_init+0xa7e/0xc20
net/sctp/sm_make_chunk.c:2437
    [<00000000b58b62f8>] sctp_cmd_process_init net/sctp/sm_sideeffect.c:682
[inline]
    [<00000000b58b62f8>] sctp_cmd_interpreter net/sctp/sm_sideeffect.c:1384
[inline]
    [<00000000b58b62f8>] sctp_side_effects net/sctp/sm_sideeffect.c:1194
[inline]
    [<00000000b58b62f8>] sctp_do_sm+0xbdc/0x1d60 net/sctp/sm_sideeffect.c:1165
    [<0000000044e11f96>] sctp_assoc_bh_rcv+0x13c/0x200
net/sctp/associola.c:1074
    [<00000000ec43804d>] sctp_inq_push+0x7f/0xb0 net/sctp/inqueue.c:95
    [<00000000726aa954>] sctp_backlog_rcv+0x5e/0x2a0 net/sctp/input.c:354
    [<00000000d9e249a8>] sk_backlog_rcv include/net/sock.h:950 [inline]
    [<00000000d9e249a8>] __release_sock+0xab/0x110 net/core/sock.c:2418
    [<00000000acae44fa>] release_sock+0x37/0xd0 net/core/sock.c:2934
    [<00000000963cc9ae>] sctp_sendmsg+0x2c0/0x990 net/sctp/socket.c:2122
    [<00000000a7fc7565>] inet_sendmsg+0x64/0x120 net/ipv4/af_inet.c:802
    [<00000000b732cbd3>] sock_sendmsg_nosec net/socket.c:652 [inline]
    [<00000000b732cbd3>] sock_sendmsg+0x54/0x70 net/socket.c:671
    [<00000000274c57ab>] ___sys_sendmsg+0x393/0x3c0 net/socket.c:2292
    [<000000008252aedb>] __sys_sendmsg+0x80/0xf0 net/socket.c:2330
    [<00000000f7bf23d1>] __do_sys_sendmsg net/socket.c:2339 [inline]
    [<00000000f7bf23d1>] __se_sys_sendmsg net/socket.c:2337 [inline]
    [<00000000f7bf23d1>] __x64_sys_sendmsg+0x23/0x30 net/socket.c:2337
    [<00000000a8b4131f>] do_syscall_64+0x76/0x1a0 arch/x86/entry/common.c:3

The problem was that the peer.cookie value points to an skb allocated
area on the first pass through this function, at which point it is
overwritten with a heap allocated value, but in certain cases, where a
COOKIE_ECHO chunk is included in the packet, a second pass through
sctp_process_init is made, where the cookie value is re-allocated,
leaking the first allocation.

Fix is to always allocate the cookie value, and free it when we are done
using it.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
Reported-by: syzbot+f7e9153b037eac9b1df8@syzkaller.appspotmail.com
CC: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
CC: "David S. Miller" <davem@davemloft.net>
CC: netdev@vger.kernel.org

---
Change notes

V1->V2:
  * Removed an accidental double free I let slip in in
sctp_association_free
  * Removed now unused cookie variable
---
 net/sctp/sm_make_chunk.c | 13 +++----------
 net/sctp/sm_sideeffect.c |  5 +++++
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index 72e74503f9fc..8e12e19fe42d 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -2327,7 +2327,6 @@ int sctp_process_init(struct sctp_association *asoc, struct sctp_chunk *chunk,
 	union sctp_addr addr;
 	struct sctp_af *af;
 	int src_match = 0;
-	char *cookie;
 
 	/* We must include the address that the INIT packet came from.
 	 * This is the only address that matters for an INIT packet.
@@ -2431,14 +2430,6 @@ int sctp_process_init(struct sctp_association *asoc, struct sctp_chunk *chunk,
 	/* Peer Rwnd   : Current calculated value of the peer's rwnd.  */
 	asoc->peer.rwnd = asoc->peer.i.a_rwnd;
 
-	/* Copy cookie in case we need to resend COOKIE-ECHO. */
-	cookie = asoc->peer.cookie;
-	if (cookie) {
-		asoc->peer.cookie = kmemdup(cookie, asoc->peer.cookie_len, gfp);
-		if (!asoc->peer.cookie)
-			goto clean_up;
-	}
-
 	/* RFC 2960 7.2.1 The initial value of ssthresh MAY be arbitrarily
 	 * high (for example, implementations MAY use the size of the receiver
 	 * advertised window).
@@ -2607,7 +2598,9 @@ static int sctp_process_param(struct sctp_association *asoc,
 	case SCTP_PARAM_STATE_COOKIE:
 		asoc->peer.cookie_len =
 			ntohs(param.p->length) - sizeof(struct sctp_paramhdr);
-		asoc->peer.cookie = param.cookie->body;
+		asoc->peer.cookie = kmemdup(param.cookie->body, asoc->peer.cookie_len, gfp);
+		if (!asoc->peer.cookie)
+			retval = 0;
 		break;
 
 	case SCTP_PARAM_HEARTBEAT_INFO:
diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
index 4aa03588f87b..27ddf2d8f001 100644
--- a/net/sctp/sm_sideeffect.c
+++ b/net/sctp/sm_sideeffect.c
@@ -898,6 +898,11 @@ static void sctp_cmd_new_state(struct sctp_cmd_seq *cmds,
 						asoc->rto_initial;
 	}
 
+	if (sctp_state(asoc, ESTABLISHED)) {
+		kfree(asoc->peer.cookie);
+		asoc->peer.cookie = NULL;
+	}
+
 	if (sctp_state(asoc, ESTABLISHED) ||
 	    sctp_state(asoc, CLOSED) ||
 	    sctp_state(asoc, SHUTDOWN_RECEIVED)) {
-- 
2.20.1

Re: [PATCH V2] Fix memory leak in sctp_process_init

[Marcelo Ricardo Leitner]

On Mon, Jun 03, 2019 at 04:32:59PM -0400, Neil Horman wrote:
> syzbot found the following leak in sctp_process_init
> BUG: memory leak
> unreferenced object 0xffff88810ef68400 (size 1024):
>   comm "syz-executor273", pid 7046, jiffies 4294945598 (age 28.770s)
>   hex dump (first 32 bytes):
>     1d de 28 8d de 0b 1b e3 b5 c2 f9 68 fd 1a 97 25  ..(........h...%
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   backtrace:
>     [<00000000a02cebbd>] kmemleak_alloc_recursive include/linux/kmemleak.h:55
> [inline]
>     [<00000000a02cebbd>] slab_post_alloc_hook mm/slab.h:439 [inline]
>     [<00000000a02cebbd>] slab_alloc mm/slab.c:3326 [inline]
>     [<00000000a02cebbd>] __do_kmalloc mm/slab.c:3658 [inline]
>     [<00000000a02cebbd>] __kmalloc_track_caller+0x15d/0x2c0 mm/slab.c:3675
>     [<000000009e6245e6>] kmemdup+0x27/0x60 mm/util.c:119
>     [<00000000dfdc5d2d>] kmemdup include/linux/string.h:432 [inline]
>     [<00000000dfdc5d2d>] sctp_process_init+0xa7e/0xc20
> net/sctp/sm_make_chunk.c:2437
>     [<00000000b58b62f8>] sctp_cmd_process_init net/sctp/sm_sideeffect.c:682
> [inline]
>     [<00000000b58b62f8>] sctp_cmd_interpreter net/sctp/sm_sideeffect.c:1384
> [inline]
>     [<00000000b58b62f8>] sctp_side_effects net/sctp/sm_sideeffect.c:1194
> [inline]
>     [<00000000b58b62f8>] sctp_do_sm+0xbdc/0x1d60 net/sctp/sm_sideeffect.c:1165
>     [<0000000044e11f96>] sctp_assoc_bh_rcv+0x13c/0x200
> net/sctp/associola.c:1074
>     [<00000000ec43804d>] sctp_inq_push+0x7f/0xb0 net/sctp/inqueue.c:95
>     [<00000000726aa954>] sctp_backlog_rcv+0x5e/0x2a0 net/sctp/input.c:354
>     [<00000000d9e249a8>] sk_backlog_rcv include/net/sock.h:950 [inline]
>     [<00000000d9e249a8>] __release_sock+0xab/0x110 net/core/sock.c:2418
>     [<00000000acae44fa>] release_sock+0x37/0xd0 net/core/sock.c:2934
>     [<00000000963cc9ae>] sctp_sendmsg+0x2c0/0x990 net/sctp/socket.c:2122
>     [<00000000a7fc7565>] inet_sendmsg+0x64/0x120 net/ipv4/af_inet.c:802
>     [<00000000b732cbd3>] sock_sendmsg_nosec net/socket.c:652 [inline]
>     [<00000000b732cbd3>] sock_sendmsg+0x54/0x70 net/socket.c:671
>     [<00000000274c57ab>] ___sys_sendmsg+0x393/0x3c0 net/socket.c:2292
>     [<000000008252aedb>] __sys_sendmsg+0x80/0xf0 net/socket.c:2330
>     [<00000000f7bf23d1>] __do_sys_sendmsg net/socket.c:2339 [inline]
>     [<00000000f7bf23d1>] __se_sys_sendmsg net/socket.c:2337 [inline]
>     [<00000000f7bf23d1>] __x64_sys_sendmsg+0x23/0x30 net/socket.c:2337
>     [<00000000a8b4131f>] do_syscall_64+0x76/0x1a0 arch/x86/entry/common.c:3
> 
> The problem was that the peer.cookie value points to an skb allocated
> area on the first pass through this function, at which point it is
> overwritten with a heap allocated value, but in certain cases, where a
> COOKIE_ECHO chunk is included in the packet, a second pass through
> sctp_process_init is made, where the cookie value is re-allocated,
> leaking the first allocation.
> 
> Fix is to always allocate the cookie value, and free it when we are done
> using it.
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> Reported-by: syzbot+f7e9153b037eac9b1df8@syzkaller.appspotmail.com
> CC: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> CC: "David S. Miller" <davem@davemloft.net>
> CC: netdev@vger.kernel.org

Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

> 
> ---
> Change notes
> 
> V1->V2:
>   * Removed an accidental double free I let slip in in
> sctp_association_free
>   * Removed now unused cookie variable
> ---
>  net/sctp/sm_make_chunk.c | 13 +++----------
>  net/sctp/sm_sideeffect.c |  5 +++++
>  2 files changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index 72e74503f9fc..8e12e19fe42d 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -2327,7 +2327,6 @@ int sctp_process_init(struct sctp_association *asoc, struct sctp_chunk *chunk,
>  	union sctp_addr addr;
>  	struct sctp_af *af;
>  	int src_match = 0;
> -	char *cookie;
>  
>  	/* We must include the address that the INIT packet came from.
>  	 * This is the only address that matters for an INIT packet.
> @@ -2431,14 +2430,6 @@ int sctp_process_init(struct sctp_association *asoc, struct sctp_chunk *chunk,
>  	/* Peer Rwnd   : Current calculated value of the peer's rwnd.  */
>  	asoc->peer.rwnd = asoc->peer.i.a_rwnd;
>  
> -	/* Copy cookie in case we need to resend COOKIE-ECHO. */
> -	cookie = asoc->peer.cookie;
> -	if (cookie) {
> -		asoc->peer.cookie = kmemdup(cookie, asoc->peer.cookie_len, gfp);
> -		if (!asoc->peer.cookie)
> -			goto clean_up;
> -	}
> -
>  	/* RFC 2960 7.2.1 The initial value of ssthresh MAY be arbitrarily
>  	 * high (for example, implementations MAY use the size of the receiver
>  	 * advertised window).
> @@ -2607,7 +2598,9 @@ static int sctp_process_param(struct sctp_association *asoc,
>  	case SCTP_PARAM_STATE_COOKIE:
>  		asoc->peer.cookie_len =
>  			ntohs(param.p->length) - sizeof(struct sctp_paramhdr);
> -		asoc->peer.cookie = param.cookie->body;
> +		asoc->peer.cookie = kmemdup(param.cookie->body, asoc->peer.cookie_len, gfp);
> +		if (!asoc->peer.cookie)
> +			retval = 0;
>  		break;
>  
>  	case SCTP_PARAM_HEARTBEAT_INFO:
> diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
> index 4aa03588f87b..27ddf2d8f001 100644
> --- a/net/sctp/sm_sideeffect.c
> +++ b/net/sctp/sm_sideeffect.c
> @@ -898,6 +898,11 @@ static void sctp_cmd_new_state(struct sctp_cmd_seq *cmds,
>  						asoc->rto_initial;
>  	}
>  
> +	if (sctp_state(asoc, ESTABLISHED)) {
> +		kfree(asoc->peer.cookie);
> +		asoc->peer.cookie = NULL;
> +	}
> +
>  	if (sctp_state(asoc, ESTABLISHED) ||
>  	    sctp_state(asoc, CLOSED) ||
>  	    sctp_state(asoc, SHUTDOWN_RECEIVED)) {
> -- 
> 2.20.1
> 

Re: [PATCH V2] Fix memory leak in sctp_process_init

[Xin Long]

On Tue, Jun 4, 2019 at 4:34 AM Neil Horman <nhorman@tuxdriver.com> wrote:
>
> syzbot found the following leak in sctp_process_init
> BUG: memory leak
> unreferenced object 0xffff88810ef68400 (size 1024):
>   comm "syz-executor273", pid 7046, jiffies 4294945598 (age 28.770s)
>   hex dump (first 32 bytes):
>     1d de 28 8d de 0b 1b e3 b5 c2 f9 68 fd 1a 97 25  ..(........h...%
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   backtrace:
>     [<00000000a02cebbd>] kmemleak_alloc_recursive include/linux/kmemleak.h:55
> [inline]
>     [<00000000a02cebbd>] slab_post_alloc_hook mm/slab.h:439 [inline]
>     [<00000000a02cebbd>] slab_alloc mm/slab.c:3326 [inline]
>     [<00000000a02cebbd>] __do_kmalloc mm/slab.c:3658 [inline]
>     [<00000000a02cebbd>] __kmalloc_track_caller+0x15d/0x2c0 mm/slab.c:3675
>     [<000000009e6245e6>] kmemdup+0x27/0x60 mm/util.c:119
>     [<00000000dfdc5d2d>] kmemdup include/linux/string.h:432 [inline]
>     [<00000000dfdc5d2d>] sctp_process_init+0xa7e/0xc20
> net/sctp/sm_make_chunk.c:2437
>     [<00000000b58b62f8>] sctp_cmd_process_init net/sctp/sm_sideeffect.c:682
> [inline]
>     [<00000000b58b62f8>] sctp_cmd_interpreter net/sctp/sm_sideeffect.c:1384
> [inline]
>     [<00000000b58b62f8>] sctp_side_effects net/sctp/sm_sideeffect.c:1194
> [inline]
>     [<00000000b58b62f8>] sctp_do_sm+0xbdc/0x1d60 net/sctp/sm_sideeffect.c:1165
>     [<0000000044e11f96>] sctp_assoc_bh_rcv+0x13c/0x200
> net/sctp/associola.c:1074
>     [<00000000ec43804d>] sctp_inq_push+0x7f/0xb0 net/sctp/inqueue.c:95
>     [<00000000726aa954>] sctp_backlog_rcv+0x5e/0x2a0 net/sctp/input.c:354
>     [<00000000d9e249a8>] sk_backlog_rcv include/net/sock.h:950 [inline]
>     [<00000000d9e249a8>] __release_sock+0xab/0x110 net/core/sock.c:2418
>     [<00000000acae44fa>] release_sock+0x37/0xd0 net/core/sock.c:2934
>     [<00000000963cc9ae>] sctp_sendmsg+0x2c0/0x990 net/sctp/socket.c:2122
>     [<00000000a7fc7565>] inet_sendmsg+0x64/0x120 net/ipv4/af_inet.c:802
>     [<00000000b732cbd3>] sock_sendmsg_nosec net/socket.c:652 [inline]
>     [<00000000b732cbd3>] sock_sendmsg+0x54/0x70 net/socket.c:671
>     [<00000000274c57ab>] ___sys_sendmsg+0x393/0x3c0 net/socket.c:2292
>     [<000000008252aedb>] __sys_sendmsg+0x80/0xf0 net/socket.c:2330
>     [<00000000f7bf23d1>] __do_sys_sendmsg net/socket.c:2339 [inline]
>     [<00000000f7bf23d1>] __se_sys_sendmsg net/socket.c:2337 [inline]
>     [<00000000f7bf23d1>] __x64_sys_sendmsg+0x23/0x30 net/socket.c:2337
>     [<00000000a8b4131f>] do_syscall_64+0x76/0x1a0 arch/x86/entry/common.c:3
>
> The problem was that the peer.cookie value points to an skb allocated
> area on the first pass through this function, at which point it is
> overwritten with a heap allocated value, but in certain cases, where a
> COOKIE_ECHO chunk is included in the packet, a second pass through
> sctp_process_init is made, where the cookie value is re-allocated,
> leaking the first allocation.
This's not gonna happen, as after processing INIT, the temp asoc will be
deleted on the server side. Besides, from the reproducer:

  https://syzkaller.appspot.com/x/repro.syz?x=10e32f8ca00000

Packet(INIT|COOKIE_ECHO) can't be made in here.

The call trace says the leak happened when processing INIT_ACK on the
client side, as Marcelo noticed.  Then it can be triggered by:

1. sctp_sf_do_5_1C_ack() -> SCTP_CMD_PEER_INIT -> sctp_process_init():
   where it "goto clean_up" after sctp_process_param(), but in 'cleanup'
   path, it doesn't do any freeup for the memdups of sctp_process_param().
   then the client T1 retrans INIT, and later get INIT_ACK again from the
   peer, and go to sctp_process_init() to memdup().

2. sctp_sf_cookie_echoed_err() -> sctp_sf_do_5_2_6_stale():
   where the asoc state will go from COOKIE_ECHOED back to COOKIE_WAIT,
   and T1 starts to retrans INIT, and later it will get INIT_ACK again
   to sctp_process_init() and memdup().

As on either above, asoc's never been to ESTABLISHED state,
asoc->peer.cookie can be not freed, and this patch won't work.
But yes, it's nice to have this patch, just not to fix this memleak.

I tracked the code, this memleak was triggered by case 2, so I think
you also need to add something like:

@@ -881,6 +893,18 @@ static void sctp_cmd_new_state(struct sctp_cmd_seq *cmds,
                                                asoc->rto_initial;
                asoc->timeouts[SCTP_EVENT_TIMEOUT_T1_COOKIE] =
                                                asoc->rto_initial;
+
+               if (asoc->peer.cookie) {
+                       kfree(asoc->peer.cookie);
+                       kfree(asoc->peer.peer_random);
+                       kfree(asoc->peer.peer_chunks);
+                       kfree(asoc->peer.peer_hmacs);
+
+                       asoc->peer.cookie = NULL;
+                       asoc->peer.peer_random = NULL;
+                       asoc->peer.peer_random = NULL;
+                       asoc->peer.peer_hmacs = NULL;
+               }
        }

and the same thing may also be needed in sctp_cmd_process_init() on the
err path for case 1.

>
> Fix is to always allocate the cookie value, and free it when we are done
> using it.
>
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> Reported-by: syzbot+f7e9153b037eac9b1df8@syzkaller.appspotmail.com
> CC: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> CC: "David S. Miller" <davem@davemloft.net>
> CC: netdev@vger.kernel.org
>
> ---
> Change notes
>
> V1->V2:
>   * Removed an accidental double free I let slip in in
> sctp_association_free
>   * Removed now unused cookie variable
> ---
>  net/sctp/sm_make_chunk.c | 13 +++----------
>  net/sctp/sm_sideeffect.c |  5 +++++
>  2 files changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index 72e74503f9fc..8e12e19fe42d 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -2327,7 +2327,6 @@ int sctp_process_init(struct sctp_association *asoc, struct sctp_chunk *chunk,
>         union sctp_addr addr;
>         struct sctp_af *af;
>         int src_match = 0;
> -       char *cookie;
>
>         /* We must include the address that the INIT packet came from.
>          * This is the only address that matters for an INIT packet.
> @@ -2431,14 +2430,6 @@ int sctp_process_init(struct sctp_association *asoc, struct sctp_chunk *chunk,
>         /* Peer Rwnd   : Current calculated value of the peer's rwnd.  */
>         asoc->peer.rwnd = asoc->peer.i.a_rwnd;
>
> -       /* Copy cookie in case we need to resend COOKIE-ECHO. */
> -       cookie = asoc->peer.cookie;
> -       if (cookie) {
> -               asoc->peer.cookie = kmemdup(cookie, asoc->peer.cookie_len, gfp);
> -               if (!asoc->peer.cookie)
> -                       goto clean_up;
> -       }
> -
>         /* RFC 2960 7.2.1 The initial value of ssthresh MAY be arbitrarily
>          * high (for example, implementations MAY use the size of the receiver
>          * advertised window).
> @@ -2607,7 +2598,9 @@ static int sctp_process_param(struct sctp_association *asoc,
>         case SCTP_PARAM_STATE_COOKIE:
>                 asoc->peer.cookie_len =
>                         ntohs(param.p->length) - sizeof(struct sctp_paramhdr);
> -               asoc->peer.cookie = param.cookie->body;
> +               asoc->peer.cookie = kmemdup(param.cookie->body, asoc->peer.cookie_len, gfp);
> +               if (!asoc->peer.cookie)
> +                       retval = 0;
>                 break;
>
>         case SCTP_PARAM_HEARTBEAT_INFO:
> diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
> index 4aa03588f87b..27ddf2d8f001 100644
> --- a/net/sctp/sm_sideeffect.c
> +++ b/net/sctp/sm_sideeffect.c
> @@ -898,6 +898,11 @@ static void sctp_cmd_new_state(struct sctp_cmd_seq *cmds,
>                                                 asoc->rto_initial;
>         }
>
> +       if (sctp_state(asoc, ESTABLISHED)) {
> +               kfree(asoc->peer.cookie);
> +               asoc->peer.cookie = NULL;
> +       }
> +
>         if (sctp_state(asoc, ESTABLISHED) ||
>             sctp_state(asoc, CLOSED) ||
>             sctp_state(asoc, SHUTDOWN_RECEIVED)) {
> --
> 2.20.1
>

Re: [PATCH V2] Fix memory leak in sctp_process_init

[Marcelo Ricardo Leitner]

On Wed, Jun 05, 2019 at 04:16:24AM +0800, Xin Long wrote:
> On Tue, Jun 4, 2019 at 4:34 AM Neil Horman <nhorman@tuxdriver.com> wrote:
> >
> > syzbot found the following leak in sctp_process_init
> > BUG: memory leak
> > unreferenced object 0xffff88810ef68400 (size 1024):
> >   comm "syz-executor273", pid 7046, jiffies 4294945598 (age 28.770s)
> >   hex dump (first 32 bytes):
> >     1d de 28 8d de 0b 1b e3 b5 c2 f9 68 fd 1a 97 25  ..(........h...%
> >     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
> >   backtrace:
> >     [<00000000a02cebbd>] kmemleak_alloc_recursive include/linux/kmemleak.h:55
> > [inline]
> >     [<00000000a02cebbd>] slab_post_alloc_hook mm/slab.h:439 [inline]
> >     [<00000000a02cebbd>] slab_alloc mm/slab.c:3326 [inline]
> >     [<00000000a02cebbd>] __do_kmalloc mm/slab.c:3658 [inline]
> >     [<00000000a02cebbd>] __kmalloc_track_caller+0x15d/0x2c0 mm/slab.c:3675
> >     [<000000009e6245e6>] kmemdup+0x27/0x60 mm/util.c:119
> >     [<00000000dfdc5d2d>] kmemdup include/linux/string.h:432 [inline]
> >     [<00000000dfdc5d2d>] sctp_process_init+0xa7e/0xc20
> > net/sctp/sm_make_chunk.c:2437
> >     [<00000000b58b62f8>] sctp_cmd_process_init net/sctp/sm_sideeffect.c:682
> > [inline]
> >     [<00000000b58b62f8>] sctp_cmd_interpreter net/sctp/sm_sideeffect.c:1384
> > [inline]
> >     [<00000000b58b62f8>] sctp_side_effects net/sctp/sm_sideeffect.c:1194
> > [inline]
> >     [<00000000b58b62f8>] sctp_do_sm+0xbdc/0x1d60 net/sctp/sm_sideeffect.c:1165
> >     [<0000000044e11f96>] sctp_assoc_bh_rcv+0x13c/0x200
> > net/sctp/associola.c:1074
> >     [<00000000ec43804d>] sctp_inq_push+0x7f/0xb0 net/sctp/inqueue.c:95
> >     [<00000000726aa954>] sctp_backlog_rcv+0x5e/0x2a0 net/sctp/input.c:354
> >     [<00000000d9e249a8>] sk_backlog_rcv include/net/sock.h:950 [inline]
> >     [<00000000d9e249a8>] __release_sock+0xab/0x110 net/core/sock.c:2418
> >     [<00000000acae44fa>] release_sock+0x37/0xd0 net/core/sock.c:2934
> >     [<00000000963cc9ae>] sctp_sendmsg+0x2c0/0x990 net/sctp/socket.c:2122
> >     [<00000000a7fc7565>] inet_sendmsg+0x64/0x120 net/ipv4/af_inet.c:802
> >     [<00000000b732cbd3>] sock_sendmsg_nosec net/socket.c:652 [inline]
> >     [<00000000b732cbd3>] sock_sendmsg+0x54/0x70 net/socket.c:671
> >     [<00000000274c57ab>] ___sys_sendmsg+0x393/0x3c0 net/socket.c:2292
> >     [<000000008252aedb>] __sys_sendmsg+0x80/0xf0 net/socket.c:2330
> >     [<00000000f7bf23d1>] __do_sys_sendmsg net/socket.c:2339 [inline]
> >     [<00000000f7bf23d1>] __se_sys_sendmsg net/socket.c:2337 [inline]
> >     [<00000000f7bf23d1>] __x64_sys_sendmsg+0x23/0x30 net/socket.c:2337
> >     [<00000000a8b4131f>] do_syscall_64+0x76/0x1a0 arch/x86/entry/common.c:3
> >
> > The problem was that the peer.cookie value points to an skb allocated
> > area on the first pass through this function, at which point it is
> > overwritten with a heap allocated value, but in certain cases, where a
> > COOKIE_ECHO chunk is included in the packet, a second pass through
> > sctp_process_init is made, where the cookie value is re-allocated,
> > leaking the first allocation.
> This's not gonna happen, as after processing INIT, the temp asoc will be
> deleted on the server side. Besides, from the reproducer:
> 
>   https://syzkaller.appspot.com/x/repro.syz?x=10e32f8ca00000
> 
> Packet(INIT|COOKIE_ECHO) can't be made in here.
> 
> The call trace says the leak happened when processing INIT_ACK on the
> client side, as Marcelo noticed.  Then it can be triggered by:
> 
> 1. sctp_sf_do_5_1C_ack() -> SCTP_CMD_PEER_INIT -> sctp_process_init():
>    where it "goto clean_up" after sctp_process_param(), but in 'cleanup'
>    path, it doesn't do any freeup for the memdups of sctp_process_param().
>    then the client T1 retrans INIT, and later get INIT_ACK again from the
>    peer, and go to sctp_process_init() to memdup().
> 
> 2. sctp_sf_cookie_echoed_err() -> sctp_sf_do_5_2_6_stale():
>    where the asoc state will go from COOKIE_ECHOED back to COOKIE_WAIT,
>    and T1 starts to retrans INIT, and later it will get INIT_ACK again
>    to sctp_process_init() and memdup().
> 
> As on either above, asoc's never been to ESTABLISHED state,
> asoc->peer.cookie can be not freed, and this patch won't work.
> But yes, it's nice to have this patch, just not to fix this memleak.
> 
> I tracked the code, this memleak was triggered by case 2, so I think
> you also need to add something like:

Interesting, because as part of writing some scripts here to more
easily work with syzkaller reports, I actually had the patch Neil
posted being tested on Friday and with it applied the reproducer
didn't catch any leaks anymore, while with it, it takes a while but
the leaks were happening.

(Will read the details you posted later, I'm just sharing this for
now, sorry for the rushiness)

> 
> @@ -881,6 +893,18 @@ static void sctp_cmd_new_state(struct sctp_cmd_seq *cmds,
>                                                 asoc->rto_initial;
>                 asoc->timeouts[SCTP_EVENT_TIMEOUT_T1_COOKIE] =
>                                                 asoc->rto_initial;
> +
> +               if (asoc->peer.cookie) {
> +                       kfree(asoc->peer.cookie);
> +                       kfree(asoc->peer.peer_random);
> +                       kfree(asoc->peer.peer_chunks);
> +                       kfree(asoc->peer.peer_hmacs);
> +
> +                       asoc->peer.cookie = NULL;
> +                       asoc->peer.peer_random = NULL;
> +                       asoc->peer.peer_random = NULL;
> +                       asoc->peer.peer_hmacs = NULL;
> +               }
>         }
> 
> and the same thing may also be needed in sctp_cmd_process_init() on the
> err path for case 1.
> 
> >
> > Fix is to always allocate the cookie value, and free it when we are done
> > using it.
> >
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > Reported-by: syzbot+f7e9153b037eac9b1df8@syzkaller.appspotmail.com
> > CC: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> > CC: "David S. Miller" <davem@davemloft.net>
> > CC: netdev@vger.kernel.org
> >
> > ---
> > Change notes
> >
> > V1->V2:
> >   * Removed an accidental double free I let slip in in
> > sctp_association_free
> >   * Removed now unused cookie variable
> > ---
> >  net/sctp/sm_make_chunk.c | 13 +++----------
> >  net/sctp/sm_sideeffect.c |  5 +++++
> >  2 files changed, 8 insertions(+), 10 deletions(-)
> >
> > diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> > index 72e74503f9fc..8e12e19fe42d 100644
> > --- a/net/sctp/sm_make_chunk.c
> > +++ b/net/sctp/sm_make_chunk.c
> > @@ -2327,7 +2327,6 @@ int sctp_process_init(struct sctp_association *asoc, struct sctp_chunk *chunk,
> >         union sctp_addr addr;
> >         struct sctp_af *af;
> >         int src_match = 0;
> > -       char *cookie;
> >
> >         /* We must include the address that the INIT packet came from.
> >          * This is the only address that matters for an INIT packet.
> > @@ -2431,14 +2430,6 @@ int sctp_process_init(struct sctp_association *asoc, struct sctp_chunk *chunk,
> >         /* Peer Rwnd   : Current calculated value of the peer's rwnd.  */
> >         asoc->peer.rwnd = asoc->peer.i.a_rwnd;
> >
> > -       /* Copy cookie in case we need to resend COOKIE-ECHO. */
> > -       cookie = asoc->peer.cookie;
> > -       if (cookie) {
> > -               asoc->peer.cookie = kmemdup(cookie, asoc->peer.cookie_len, gfp);
> > -               if (!asoc->peer.cookie)
> > -                       goto clean_up;
> > -       }
> > -
> >         /* RFC 2960 7.2.1 The initial value of ssthresh MAY be arbitrarily
> >          * high (for example, implementations MAY use the size of the receiver
> >          * advertised window).
> > @@ -2607,7 +2598,9 @@ static int sctp_process_param(struct sctp_association *asoc,
> >         case SCTP_PARAM_STATE_COOKIE:
> >                 asoc->peer.cookie_len =
> >                         ntohs(param.p->length) - sizeof(struct sctp_paramhdr);
> > -               asoc->peer.cookie = param.cookie->body;
> > +               asoc->peer.cookie = kmemdup(param.cookie->body, asoc->peer.cookie_len, gfp);
> > +               if (!asoc->peer.cookie)
> > +                       retval = 0;
> >                 break;
> >
> >         case SCTP_PARAM_HEARTBEAT_INFO:
> > diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
> > index 4aa03588f87b..27ddf2d8f001 100644
> > --- a/net/sctp/sm_sideeffect.c
> > +++ b/net/sctp/sm_sideeffect.c
> > @@ -898,6 +898,11 @@ static void sctp_cmd_new_state(struct sctp_cmd_seq *cmds,
> >                                                 asoc->rto_initial;
> >         }
> >
> > +       if (sctp_state(asoc, ESTABLISHED)) {
> > +               kfree(asoc->peer.cookie);
> > +               asoc->peer.cookie = NULL;
> > +       }
> > +
> >         if (sctp_state(asoc, ESTABLISHED) ||
> >             sctp_state(asoc, CLOSED) ||
> >             sctp_state(asoc, SHUTDOWN_RECEIVED)) {
> > --
> > 2.20.1
> >
> 

Re: [PATCH V2] Fix memory leak in sctp_process_init

[Neil Horman]

On Wed, Jun 05, 2019 at 04:16:24AM +0800, Xin Long wrote:
> On Tue, Jun 4, 2019 at 4:34 AM Neil Horman <nhorman@tuxdriver.com> wrote:
> >
> > syzbot found the following leak in sctp_process_init
> > BUG: memory leak
> > unreferenced object 0xffff88810ef68400 (size 1024):
> >   comm "syz-executor273", pid 7046, jiffies 4294945598 (age 28.770s)
> >   hex dump (first 32 bytes):
> >     1d de 28 8d de 0b 1b e3 b5 c2 f9 68 fd 1a 97 25  ..(........h...%
> >     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
> >   backtrace:
> >     [<00000000a02cebbd>] kmemleak_alloc_recursive include/linux/kmemleak.h:55
> > [inline]
> >     [<00000000a02cebbd>] slab_post_alloc_hook mm/slab.h:439 [inline]
> >     [<00000000a02cebbd>] slab_alloc mm/slab.c:3326 [inline]
> >     [<00000000a02cebbd>] __do_kmalloc mm/slab.c:3658 [inline]
> >     [<00000000a02cebbd>] __kmalloc_track_caller+0x15d/0x2c0 mm/slab.c:3675
> >     [<000000009e6245e6>] kmemdup+0x27/0x60 mm/util.c:119
> >     [<00000000dfdc5d2d>] kmemdup include/linux/string.h:432 [inline]
> >     [<00000000dfdc5d2d>] sctp_process_init+0xa7e/0xc20
> > net/sctp/sm_make_chunk.c:2437
> >     [<00000000b58b62f8>] sctp_cmd_process_init net/sctp/sm_sideeffect.c:682
> > [inline]
> >     [<00000000b58b62f8>] sctp_cmd_interpreter net/sctp/sm_sideeffect.c:1384
> > [inline]
> >     [<00000000b58b62f8>] sctp_side_effects net/sctp/sm_sideeffect.c:1194
> > [inline]
> >     [<00000000b58b62f8>] sctp_do_sm+0xbdc/0x1d60 net/sctp/sm_sideeffect.c:1165
> >     [<0000000044e11f96>] sctp_assoc_bh_rcv+0x13c/0x200
> > net/sctp/associola.c:1074
> >     [<00000000ec43804d>] sctp_inq_push+0x7f/0xb0 net/sctp/inqueue.c:95
> >     [<00000000726aa954>] sctp_backlog_rcv+0x5e/0x2a0 net/sctp/input.c:354
> >     [<00000000d9e249a8>] sk_backlog_rcv include/net/sock.h:950 [inline]
> >     [<00000000d9e249a8>] __release_sock+0xab/0x110 net/core/sock.c:2418
> >     [<00000000acae44fa>] release_sock+0x37/0xd0 net/core/sock.c:2934
> >     [<00000000963cc9ae>] sctp_sendmsg+0x2c0/0x990 net/sctp/socket.c:2122
> >     [<00000000a7fc7565>] inet_sendmsg+0x64/0x120 net/ipv4/af_inet.c:802
> >     [<00000000b732cbd3>] sock_sendmsg_nosec net/socket.c:652 [inline]
> >     [<00000000b732cbd3>] sock_sendmsg+0x54/0x70 net/socket.c:671
> >     [<00000000274c57ab>] ___sys_sendmsg+0x393/0x3c0 net/socket.c:2292
> >     [<000000008252aedb>] __sys_sendmsg+0x80/0xf0 net/socket.c:2330
> >     [<00000000f7bf23d1>] __do_sys_sendmsg net/socket.c:2339 [inline]
> >     [<00000000f7bf23d1>] __se_sys_sendmsg net/socket.c:2337 [inline]
> >     [<00000000f7bf23d1>] __x64_sys_sendmsg+0x23/0x30 net/socket.c:2337
> >     [<00000000a8b4131f>] do_syscall_64+0x76/0x1a0 arch/x86/entry/common.c:3
> >
> > The problem was that the peer.cookie value points to an skb allocated
> > area on the first pass through this function, at which point it is
> > overwritten with a heap allocated value, but in certain cases, where a
> > COOKIE_ECHO chunk is included in the packet, a second pass through
> > sctp_process_init is made, where the cookie value is re-allocated,
> > leaking the first allocation.
> This's not gonna happen, as after processing INIT, the temp asoc will be
> deleted on the server side. Besides, from the reproducer:
> 
>   https://syzkaller.appspot.com/x/repro.syz?x=10e32f8ca00000
> 
> Packet(INIT|COOKIE_ECHO) can't be made in here.
> 
> The call trace says the leak happened when processing INIT_ACK on the
> client side, as Marcelo noticed.  Then it can be triggered by:
> 
> 1. sctp_sf_do_5_1C_ack() -> SCTP_CMD_PEER_INIT -> sctp_process_init():
>    where it "goto clean_up" after sctp_process_param(), but in 'cleanup'
>    path, it doesn't do any freeup for the memdups of sctp_process_param().
>    then the client T1 retrans INIT, and later get INIT_ACK again from the
>    peer, and go to sctp_process_init() to memdup().
> 
> 2. sctp_sf_cookie_echoed_err() -> sctp_sf_do_5_2_6_stale():
>    where the asoc state will go from COOKIE_ECHOED back to COOKIE_WAIT,
>    and T1 starts to retrans INIT, and later it will get INIT_ACK again
>    to sctp_process_init() and memdup().
> 
ok, you may well be right as to the path that we take to get here, but the root
cause is the same, multiple passes through sctp_process_init without freeing the
previously memduped memory.  Thats why I would think my patch is fixing the
issue, because now we're always duping the cookie memory and always freeing it
when we transition to the ESTABLISHED state.

As for the other variables (peer_[random|chunks|hmacs]), I'm not sure why we're
not seeing leak reports of those variables.

Neil

> As on either above, asoc's never been to ESTABLISHED state,
> asoc->peer.cookie can be not freed, and this patch won't work.
> But yes, it's nice to have this patch, just not to fix this memleak.
> 
> I tracked the code, this memleak was triggered by case 2, so I think
> you also need to add something like:
> 
> @@ -881,6 +893,18 @@ static void sctp_cmd_new_state(struct sctp_cmd_seq *cmds,
>                                                 asoc->rto_initial;
>                 asoc->timeouts[SCTP_EVENT_TIMEOUT_T1_COOKIE] =
>                                                 asoc->rto_initial;
> +
> +               if (asoc->peer.cookie) {
> +                       kfree(asoc->peer.cookie);
> +                       kfree(asoc->peer.peer_random);
> +                       kfree(asoc->peer.peer_chunks);
> +                       kfree(asoc->peer.peer_hmacs);
> +
> +                       asoc->peer.cookie = NULL;
> +                       asoc->peer.peer_random = NULL;
> +                       asoc->peer.peer_random = NULL;
> +                       asoc->peer.peer_hmacs = NULL;
> +               }
>         }
> 
> and the same thing may also be needed in sctp_cmd_process_init() on the
> err path for case 1.
> 
> >
> > Fix is to always allocate the cookie value, and free it when we are done
> > using it.
> >
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > Reported-by: syzbot+f7e9153b037eac9b1df8@syzkaller.appspotmail.com
> > CC: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> > CC: "David S. Miller" <davem@davemloft.net>
> > CC: netdev@vger.kernel.org
> >
> > ---
> > Change notes
> >
> > V1->V2:
> >   * Removed an accidental double free I let slip in in
> > sctp_association_free
> >   * Removed now unused cookie variable
> > ---
> >  net/sctp/sm_make_chunk.c | 13 +++----------
> >  net/sctp/sm_sideeffect.c |  5 +++++
> >  2 files changed, 8 insertions(+), 10 deletions(-)
> >
> > diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> > index 72e74503f9fc..8e12e19fe42d 100644
> > --- a/net/sctp/sm_make_chunk.c
> > +++ b/net/sctp/sm_make_chunk.c
> > @@ -2327,7 +2327,6 @@ int sctp_process_init(struct sctp_association *asoc, struct sctp_chunk *chunk,
> >         union sctp_addr addr;
> >         struct sctp_af *af;
> >         int src_match = 0;
> > -       char *cookie;
> >
> >         /* We must include the address that the INIT packet came from.
> >          * This is the only address that matters for an INIT packet.
> > @@ -2431,14 +2430,6 @@ int sctp_process_init(struct sctp_association *asoc, struct sctp_chunk *chunk,
> >         /* Peer Rwnd   : Current calculated value of the peer's rwnd.  */
> >         asoc->peer.rwnd = asoc->peer.i.a_rwnd;
> >
> > -       /* Copy cookie in case we need to resend COOKIE-ECHO. */
> > -       cookie = asoc->peer.cookie;
> > -       if (cookie) {
> > -               asoc->peer.cookie = kmemdup(cookie, asoc->peer.cookie_len, gfp);
> > -               if (!asoc->peer.cookie)
> > -                       goto clean_up;
> > -       }
> > -
> >         /* RFC 2960 7.2.1 The initial value of ssthresh MAY be arbitrarily
> >          * high (for example, implementations MAY use the size of the receiver
> >          * advertised window).
> > @@ -2607,7 +2598,9 @@ static int sctp_process_param(struct sctp_association *asoc,
> >         case SCTP_PARAM_STATE_COOKIE:
> >                 asoc->peer.cookie_len =
> >                         ntohs(param.p->length) - sizeof(struct sctp_paramhdr);
> > -               asoc->peer.cookie = param.cookie->body;
> > +               asoc->peer.cookie = kmemdup(param.cookie->body, asoc->peer.cookie_len, gfp);
> > +               if (!asoc->peer.cookie)
> > +                       retval = 0;
> >                 break;
> >
> >         case SCTP_PARAM_HEARTBEAT_INFO:
> > diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
> > index 4aa03588f87b..27ddf2d8f001 100644
> > --- a/net/sctp/sm_sideeffect.c
> > +++ b/net/sctp/sm_sideeffect.c
> > @@ -898,6 +898,11 @@ static void sctp_cmd_new_state(struct sctp_cmd_seq *cmds,
> >                                                 asoc->rto_initial;
> >         }
> >
> > +       if (sctp_state(asoc, ESTABLISHED)) {
> > +               kfree(asoc->peer.cookie);
> > +               asoc->peer.cookie = NULL;
> > +       }
> > +
> >         if (sctp_state(asoc, ESTABLISHED) ||
> >             sctp_state(asoc, CLOSED) ||
> >             sctp_state(asoc, SHUTDOWN_RECEIVED)) {
> > --
> > 2.20.1
> >
> 

Re: [PATCH V2] Fix memory leak in sctp_process_init

[Marcelo Ricardo Leitner]

On Wed, Jun 05, 2019 at 07:20:10AM -0400, Neil Horman wrote:
> On Wed, Jun 05, 2019 at 04:16:24AM +0800, Xin Long wrote:
> > On Tue, Jun 4, 2019 at 4:34 AM Neil Horman <nhorman@tuxdriver.com> wrote:
> > >
> > > syzbot found the following leak in sctp_process_init
> > > BUG: memory leak
> > > unreferenced object 0xffff88810ef68400 (size 1024):
> > >   comm "syz-executor273", pid 7046, jiffies 4294945598 (age 28.770s)
> > >   hex dump (first 32 bytes):
> > >     1d de 28 8d de 0b 1b e3 b5 c2 f9 68 fd 1a 97 25  ..(........h...%
> > >     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
> > >   backtrace:
> > >     [<00000000a02cebbd>] kmemleak_alloc_recursive include/linux/kmemleak.h:55
> > > [inline]
> > >     [<00000000a02cebbd>] slab_post_alloc_hook mm/slab.h:439 [inline]
> > >     [<00000000a02cebbd>] slab_alloc mm/slab.c:3326 [inline]
> > >     [<00000000a02cebbd>] __do_kmalloc mm/slab.c:3658 [inline]
> > >     [<00000000a02cebbd>] __kmalloc_track_caller+0x15d/0x2c0 mm/slab.c:3675
> > >     [<000000009e6245e6>] kmemdup+0x27/0x60 mm/util.c:119
> > >     [<00000000dfdc5d2d>] kmemdup include/linux/string.h:432 [inline]
> > >     [<00000000dfdc5d2d>] sctp_process_init+0xa7e/0xc20
> > > net/sctp/sm_make_chunk.c:2437
> > >     [<00000000b58b62f8>] sctp_cmd_process_init net/sctp/sm_sideeffect.c:682
> > > [inline]
> > >     [<00000000b58b62f8>] sctp_cmd_interpreter net/sctp/sm_sideeffect.c:1384
> > > [inline]
> > >     [<00000000b58b62f8>] sctp_side_effects net/sctp/sm_sideeffect.c:1194
> > > [inline]
> > >     [<00000000b58b62f8>] sctp_do_sm+0xbdc/0x1d60 net/sctp/sm_sideeffect.c:1165
> > >     [<0000000044e11f96>] sctp_assoc_bh_rcv+0x13c/0x200
> > > net/sctp/associola.c:1074
> > >     [<00000000ec43804d>] sctp_inq_push+0x7f/0xb0 net/sctp/inqueue.c:95
> > >     [<00000000726aa954>] sctp_backlog_rcv+0x5e/0x2a0 net/sctp/input.c:354
> > >     [<00000000d9e249a8>] sk_backlog_rcv include/net/sock.h:950 [inline]
> > >     [<00000000d9e249a8>] __release_sock+0xab/0x110 net/core/sock.c:2418
> > >     [<00000000acae44fa>] release_sock+0x37/0xd0 net/core/sock.c:2934
> > >     [<00000000963cc9ae>] sctp_sendmsg+0x2c0/0x990 net/sctp/socket.c:2122
> > >     [<00000000a7fc7565>] inet_sendmsg+0x64/0x120 net/ipv4/af_inet.c:802
> > >     [<00000000b732cbd3>] sock_sendmsg_nosec net/socket.c:652 [inline]
> > >     [<00000000b732cbd3>] sock_sendmsg+0x54/0x70 net/socket.c:671
> > >     [<00000000274c57ab>] ___sys_sendmsg+0x393/0x3c0 net/socket.c:2292
> > >     [<000000008252aedb>] __sys_sendmsg+0x80/0xf0 net/socket.c:2330
> > >     [<00000000f7bf23d1>] __do_sys_sendmsg net/socket.c:2339 [inline]
> > >     [<00000000f7bf23d1>] __se_sys_sendmsg net/socket.c:2337 [inline]
> > >     [<00000000f7bf23d1>] __x64_sys_sendmsg+0x23/0x30 net/socket.c:2337
> > >     [<00000000a8b4131f>] do_syscall_64+0x76/0x1a0 arch/x86/entry/common.c:3
> > >
> > > The problem was that the peer.cookie value points to an skb allocated
> > > area on the first pass through this function, at which point it is
> > > overwritten with a heap allocated value, but in certain cases, where a
> > > COOKIE_ECHO chunk is included in the packet, a second pass through
> > > sctp_process_init is made, where the cookie value is re-allocated,
> > > leaking the first allocation.
> > This's not gonna happen, as after processing INIT, the temp asoc will be
> > deleted on the server side. Besides, from the reproducer:
> > 
> >   https://syzkaller.appspot.com/x/repro.syz?x=10e32f8ca00000
> > 
> > Packet(INIT|COOKIE_ECHO) can't be made in here.
> > 
> > The call trace says the leak happened when processing INIT_ACK on the
> > client side, as Marcelo noticed.  Then it can be triggered by:
> > 
> > 1. sctp_sf_do_5_1C_ack() -> SCTP_CMD_PEER_INIT -> sctp_process_init():
> >    where it "goto clean_up" after sctp_process_param(), but in 'cleanup'
> >    path, it doesn't do any freeup for the memdups of sctp_process_param().
> >    then the client T1 retrans INIT, and later get INIT_ACK again from the
> >    peer, and go to sctp_process_init() to memdup().
> > 
> > 2. sctp_sf_cookie_echoed_err() -> sctp_sf_do_5_2_6_stale():
> >    where the asoc state will go from COOKIE_ECHOED back to COOKIE_WAIT,
> >    and T1 starts to retrans INIT, and later it will get INIT_ACK again
> >    to sctp_process_init() and memdup().
> > 
> ok, you may well be right as to the path that we take to get here, but the root
> cause is the same, multiple passes through sctp_process_init without freeing the
> previously memduped memory.  Thats why I would think my patch is fixing the
> issue, because now we're always duping the cookie memory and always freeing it
> when we transition to the ESTABLISHED state.
> 
> As for the other variables (peer_[random|chunks|hmacs]), I'm not sure why we're
> not seeing leak reports of those variables.

This actually also works for peer.cookie as well, as now they are
allocated and freed in the same places.

I talked with Xin and I agree that this patch didn't really fix the
leak. It moved the allocation from sctp_process_init() to
sctp_process_param() which is called by sctp_process_init and there is
nothing avoiding multiple allocations on these vars once INIT_ACK is
retransmited.

The reproducer works by having the client to re-request cookies by
setting a short lifetime. So
Client sends INIT
                       Server replies INIT_ACK with cookie
Client echoes the cookie
                       Server rejects because the cookie expired
Client sends INIT again, to ask for another cookie
  (sctp_sf_do_5_2_6_stale)
                       Server sends INIT_ACK again with a new cookie
Client calls sctp_process_init again, from
  -> sctp_sf_do_5_1C_ack()
   -> sctp_cmd_process_init()
    -> sctp_process_init()
     -> sctp_process_param()
        ^-- leak happens
	               Server may or may not accept the new cookie,
		       leading to further leaks if not
(this is his 2nd case above)

Yet somehow the patch changed the dynamics and made the test pass (4x
10mins each run) here. Uff.

  Marcelo

> 
> Neil
> 
> > As on either above, asoc's never been to ESTABLISHED state,
> > asoc->peer.cookie can be not freed, and this patch won't work.
> > But yes, it's nice to have this patch, just not to fix this memleak.

+1 (read: won't hurt -stable and no need to revert)

> > 
> > I tracked the code, this memleak was triggered by case 2, so I think
> > you also need to add something like:
> > 
> > @@ -881,6 +893,18 @@ static void sctp_cmd_new_state(struct sctp_cmd_seq *cmds,
> >                                                 asoc->rto_initial;
> >                 asoc->timeouts[SCTP_EVENT_TIMEOUT_T1_COOKIE] =
> >                                                 asoc->rto_initial;
> > +
> > +               if (asoc->peer.cookie) {
> > +                       kfree(asoc->peer.cookie);
> > +                       kfree(asoc->peer.peer_random);
> > +                       kfree(asoc->peer.peer_chunks);
> > +                       kfree(asoc->peer.peer_hmacs);
> > +
> > +                       asoc->peer.cookie = NULL;
> > +                       asoc->peer.peer_random = NULL;
> > +                       asoc->peer.peer_random = NULL;
> > +                       asoc->peer.peer_hmacs = NULL;
> > +               }
> >         }
> > 
> > and the same thing may also be needed in sctp_cmd_process_init() on the
> > err path for case 1.
> > 
> > >
> > > Fix is to always allocate the cookie value, and free it when we are done
> > > using it.
> > >
> > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > > Reported-by: syzbot+f7e9153b037eac9b1df8@syzkaller.appspotmail.com
> > > CC: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> > > CC: "David S. Miller" <davem@davemloft.net>
> > > CC: netdev@vger.kernel.org
> > >
> > > ---
> > > Change notes
> > >
> > > V1->V2:
> > >   * Removed an accidental double free I let slip in in
> > > sctp_association_free
> > >   * Removed now unused cookie variable
> > > ---
> > >  net/sctp/sm_make_chunk.c | 13 +++----------
> > >  net/sctp/sm_sideeffect.c |  5 +++++
> > >  2 files changed, 8 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> > > index 72e74503f9fc..8e12e19fe42d 100644
> > > --- a/net/sctp/sm_make_chunk.c
> > > +++ b/net/sctp/sm_make_chunk.c
> > > @@ -2327,7 +2327,6 @@ int sctp_process_init(struct sctp_association *asoc, struct sctp_chunk *chunk,
> > >         union sctp_addr addr;
> > >         struct sctp_af *af;
> > >         int src_match = 0;
> > > -       char *cookie;
> > >
> > >         /* We must include the address that the INIT packet came from.
> > >          * This is the only address that matters for an INIT packet.
> > > @@ -2431,14 +2430,6 @@ int sctp_process_init(struct sctp_association *asoc, struct sctp_chunk *chunk,
> > >         /* Peer Rwnd   : Current calculated value of the peer's rwnd.  */
> > >         asoc->peer.rwnd = asoc->peer.i.a_rwnd;
> > >
> > > -       /* Copy cookie in case we need to resend COOKIE-ECHO. */
> > > -       cookie = asoc->peer.cookie;
> > > -       if (cookie) {
> > > -               asoc->peer.cookie = kmemdup(cookie, asoc->peer.cookie_len, gfp);
> > > -               if (!asoc->peer.cookie)
> > > -                       goto clean_up;
> > > -       }
> > > -
> > >         /* RFC 2960 7.2.1 The initial value of ssthresh MAY be arbitrarily
> > >          * high (for example, implementations MAY use the size of the receiver
> > >          * advertised window).
> > > @@ -2607,7 +2598,9 @@ static int sctp_process_param(struct sctp_association *asoc,
> > >         case SCTP_PARAM_STATE_COOKIE:
> > >                 asoc->peer.cookie_len =
> > >                         ntohs(param.p->length) - sizeof(struct sctp_paramhdr);
> > > -               asoc->peer.cookie = param.cookie->body;
> > > +               asoc->peer.cookie = kmemdup(param.cookie->body, asoc->peer.cookie_len, gfp);
> > > +               if (!asoc->peer.cookie)
> > > +                       retval = 0;
> > >                 break;
> > >
> > >         case SCTP_PARAM_HEARTBEAT_INFO:
> > > diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
> > > index 4aa03588f87b..27ddf2d8f001 100644
> > > --- a/net/sctp/sm_sideeffect.c
> > > +++ b/net/sctp/sm_sideeffect.c
> > > @@ -898,6 +898,11 @@ static void sctp_cmd_new_state(struct sctp_cmd_seq *cmds,
> > >                                                 asoc->rto_initial;
> > >         }
> > >
> > > +       if (sctp_state(asoc, ESTABLISHED)) {
> > > +               kfree(asoc->peer.cookie);
> > > +               asoc->peer.cookie = NULL;
> > > +       }
> > > +
> > >         if (sctp_state(asoc, ESTABLISHED) ||
> > >             sctp_state(asoc, CLOSED) ||
> > >             sctp_state(asoc, SHUTDOWN_RECEIVED)) {
> > > --
> > > 2.20.1
> > >
> > 
> 

Re: [PATCH V2] Fix memory leak in sctp_process_init

[Neil Horman]

On Thu, Jun 06, 2019 at 12:47:55PM -0300, Marcelo Ricardo Leitner wrote:
> On Wed, Jun 05, 2019 at 07:20:10AM -0400, Neil Horman wrote:
> > On Wed, Jun 05, 2019 at 04:16:24AM +0800, Xin Long wrote:
> > > On Tue, Jun 4, 2019 at 4:34 AM Neil Horman <nhorman@tuxdriver.com> wrote:
> > > >
> > > > syzbot found the following leak in sctp_process_init
> > > > BUG: memory leak
> > > > unreferenced object 0xffff88810ef68400 (size 1024):
> > > >   comm "syz-executor273", pid 7046, jiffies 4294945598 (age 28.770s)
> > > >   hex dump (first 32 bytes):
> > > >     1d de 28 8d de 0b 1b e3 b5 c2 f9 68 fd 1a 97 25  ..(........h...%
> > > >     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
> > > >   backtrace:
> > > >     [<00000000a02cebbd>] kmemleak_alloc_recursive include/linux/kmemleak.h:55
> > > > [inline]
> > > >     [<00000000a02cebbd>] slab_post_alloc_hook mm/slab.h:439 [inline]
> > > >     [<00000000a02cebbd>] slab_alloc mm/slab.c:3326 [inline]
> > > >     [<00000000a02cebbd>] __do_kmalloc mm/slab.c:3658 [inline]
> > > >     [<00000000a02cebbd>] __kmalloc_track_caller+0x15d/0x2c0 mm/slab.c:3675
> > > >     [<000000009e6245e6>] kmemdup+0x27/0x60 mm/util.c:119
> > > >     [<00000000dfdc5d2d>] kmemdup include/linux/string.h:432 [inline]
> > > >     [<00000000dfdc5d2d>] sctp_process_init+0xa7e/0xc20
> > > > net/sctp/sm_make_chunk.c:2437
> > > >     [<00000000b58b62f8>] sctp_cmd_process_init net/sctp/sm_sideeffect.c:682
> > > > [inline]
> > > >     [<00000000b58b62f8>] sctp_cmd_interpreter net/sctp/sm_sideeffect.c:1384
> > > > [inline]
> > > >     [<00000000b58b62f8>] sctp_side_effects net/sctp/sm_sideeffect.c:1194
> > > > [inline]
> > > >     [<00000000b58b62f8>] sctp_do_sm+0xbdc/0x1d60 net/sctp/sm_sideeffect.c:1165
> > > >     [<0000000044e11f96>] sctp_assoc_bh_rcv+0x13c/0x200
> > > > net/sctp/associola.c:1074
> > > >     [<00000000ec43804d>] sctp_inq_push+0x7f/0xb0 net/sctp/inqueue.c:95
> > > >     [<00000000726aa954>] sctp_backlog_rcv+0x5e/0x2a0 net/sctp/input.c:354
> > > >     [<00000000d9e249a8>] sk_backlog_rcv include/net/sock.h:950 [inline]
> > > >     [<00000000d9e249a8>] __release_sock+0xab/0x110 net/core/sock.c:2418
> > > >     [<00000000acae44fa>] release_sock+0x37/0xd0 net/core/sock.c:2934
> > > >     [<00000000963cc9ae>] sctp_sendmsg+0x2c0/0x990 net/sctp/socket.c:2122
> > > >     [<00000000a7fc7565>] inet_sendmsg+0x64/0x120 net/ipv4/af_inet.c:802
> > > >     [<00000000b732cbd3>] sock_sendmsg_nosec net/socket.c:652 [inline]
> > > >     [<00000000b732cbd3>] sock_sendmsg+0x54/0x70 net/socket.c:671
> > > >     [<00000000274c57ab>] ___sys_sendmsg+0x393/0x3c0 net/socket.c:2292
> > > >     [<000000008252aedb>] __sys_sendmsg+0x80/0xf0 net/socket.c:2330
> > > >     [<00000000f7bf23d1>] __do_sys_sendmsg net/socket.c:2339 [inline]
> > > >     [<00000000f7bf23d1>] __se_sys_sendmsg net/socket.c:2337 [inline]
> > > >     [<00000000f7bf23d1>] __x64_sys_sendmsg+0x23/0x30 net/socket.c:2337
> > > >     [<00000000a8b4131f>] do_syscall_64+0x76/0x1a0 arch/x86/entry/common.c:3
> > > >
> > > > The problem was that the peer.cookie value points to an skb allocated
> > > > area on the first pass through this function, at which point it is
> > > > overwritten with a heap allocated value, but in certain cases, where a
> > > > COOKIE_ECHO chunk is included in the packet, a second pass through
> > > > sctp_process_init is made, where the cookie value is re-allocated,
> > > > leaking the first allocation.
> > > This's not gonna happen, as after processing INIT, the temp asoc will be
> > > deleted on the server side. Besides, from the reproducer:
> > > 
> > >   https://syzkaller.appspot.com/x/repro.syz?x=10e32f8ca00000
> > > 
> > > Packet(INIT|COOKIE_ECHO) can't be made in here.
> > > 
> > > The call trace says the leak happened when processing INIT_ACK on the
> > > client side, as Marcelo noticed.  Then it can be triggered by:
> > > 
> > > 1. sctp_sf_do_5_1C_ack() -> SCTP_CMD_PEER_INIT -> sctp_process_init():
> > >    where it "goto clean_up" after sctp_process_param(), but in 'cleanup'
> > >    path, it doesn't do any freeup for the memdups of sctp_process_param().
> > >    then the client T1 retrans INIT, and later get INIT_ACK again from the
> > >    peer, and go to sctp_process_init() to memdup().
> > > 
> > > 2. sctp_sf_cookie_echoed_err() -> sctp_sf_do_5_2_6_stale():
> > >    where the asoc state will go from COOKIE_ECHOED back to COOKIE_WAIT,
> > >    and T1 starts to retrans INIT, and later it will get INIT_ACK again
> > >    to sctp_process_init() and memdup().
> > > 
> > ok, you may well be right as to the path that we take to get here, but the root
> > cause is the same, multiple passes through sctp_process_init without freeing the
> > previously memduped memory.  Thats why I would think my patch is fixing the
> > issue, because now we're always duping the cookie memory and always freeing it
> > when we transition to the ESTABLISHED state.
> > 
> > As for the other variables (peer_[random|chunks|hmacs]), I'm not sure why we're
> > not seeing leak reports of those variables.
> 
> This actually also works for peer.cookie as well, as now they are
> allocated and freed in the same places.
> 
> I talked with Xin and I agree that this patch didn't really fix the
> leak. It moved the allocation from sctp_process_init() to
> sctp_process_param() which is called by sctp_process_init and there is
> nothing avoiding multiple allocations on these vars once INIT_ACK is
> retransmited.
> 
> The reproducer works by having the client to re-request cookies by
> setting a short lifetime. So
> Client sends INIT
>                        Server replies INIT_ACK with cookie
> Client echoes the cookie
>                        Server rejects because the cookie expired
> Client sends INIT again, to ask for another cookie
>   (sctp_sf_do_5_2_6_stale)
>                        Server sends INIT_ACK again with a new cookie
> Client calls sctp_process_init again, from
>   -> sctp_sf_do_5_1C_ack()
>    -> sctp_cmd_process_init()
>     -> sctp_process_init()
>      -> sctp_process_param()
>         ^-- leak happens
> 	               Server may or may not accept the new cookie,
> 		       leading to further leaks if not
If this is the case, it seems like the most reasonable fix then is to just
check to see if peer.cookie is non-null prior to calling kmemdup, and freeing
the old cookie first, no?

Neil

> (this is his 2nd case above)
> 
> Yet somehow the patch changed the dynamics and made the test pass (4x
> 10mins each run) here. Uff.
> 
>   Marcelo
> 
> > 
> > Neil
> > 
> > > As on either above, asoc's never been to ESTABLISHED state,
> > > asoc->peer.cookie can be not freed, and this patch won't work.
> > > But yes, it's nice to have this patch, just not to fix this memleak.
> 
> +1 (read: won't hurt -stable and no need to revert)
> 
> > > 
> > > I tracked the code, this memleak was triggered by case 2, so I think
> > > you also need to add something like:
> > > 
> > > @@ -881,6 +893,18 @@ static void sctp_cmd_new_state(struct sctp_cmd_seq *cmds,
> > >                                                 asoc->rto_initial;
> > >                 asoc->timeouts[SCTP_EVENT_TIMEOUT_T1_COOKIE] =
> > >                                                 asoc->rto_initial;
> > > +
> > > +               if (asoc->peer.cookie) {
> > > +                       kfree(asoc->peer.cookie);
> > > +                       kfree(asoc->peer.peer_random);
> > > +                       kfree(asoc->peer.peer_chunks);
> > > +                       kfree(asoc->peer.peer_hmacs);
> > > +
> > > +                       asoc->peer.cookie = NULL;
> > > +                       asoc->peer.peer_random = NULL;
> > > +                       asoc->peer.peer_random = NULL;
> > > +                       asoc->peer.peer_hmacs = NULL;
> > > +               }
> > >         }
> > > 
> > > and the same thing may also be needed in sctp_cmd_process_init() on the
> > > err path for case 1.
> > > 
> > > >
> > > > Fix is to always allocate the cookie value, and free it when we are done
> > > > using it.
> > > >
> > > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > > > Reported-by: syzbot+f7e9153b037eac9b1df8@syzkaller.appspotmail.com
> > > > CC: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> > > > CC: "David S. Miller" <davem@davemloft.net>
> > > > CC: netdev@vger.kernel.org
> > > >
> > > > ---
> > > > Change notes
> > > >
> > > > V1->V2:
> > > >   * Removed an accidental double free I let slip in in
> > > > sctp_association_free
> > > >   * Removed now unused cookie variable
> > > > ---
> > > >  net/sctp/sm_make_chunk.c | 13 +++----------
> > > >  net/sctp/sm_sideeffect.c |  5 +++++
> > > >  2 files changed, 8 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> > > > index 72e74503f9fc..8e12e19fe42d 100644
> > > > --- a/net/sctp/sm_make_chunk.c
> > > > +++ b/net/sctp/sm_make_chunk.c
> > > > @@ -2327,7 +2327,6 @@ int sctp_process_init(struct sctp_association *asoc, struct sctp_chunk *chunk,
> > > >         union sctp_addr addr;
> > > >         struct sctp_af *af;
> > > >         int src_match = 0;
> > > > -       char *cookie;
> > > >
> > > >         /* We must include the address that the INIT packet came from.
> > > >          * This is the only address that matters for an INIT packet.
> > > > @@ -2431,14 +2430,6 @@ int sctp_process_init(struct sctp_association *asoc, struct sctp_chunk *chunk,
> > > >         /* Peer Rwnd   : Current calculated value of the peer's rwnd.  */
> > > >         asoc->peer.rwnd = asoc->peer.i.a_rwnd;
> > > >
> > > > -       /* Copy cookie in case we need to resend COOKIE-ECHO. */
> > > > -       cookie = asoc->peer.cookie;
> > > > -       if (cookie) {
> > > > -               asoc->peer.cookie = kmemdup(cookie, asoc->peer.cookie_len, gfp);
> > > > -               if (!asoc->peer.cookie)
> > > > -                       goto clean_up;
> > > > -       }
> > > > -
> > > >         /* RFC 2960 7.2.1 The initial value of ssthresh MAY be arbitrarily
> > > >          * high (for example, implementations MAY use the size of the receiver
> > > >          * advertised window).
> > > > @@ -2607,7 +2598,9 @@ static int sctp_process_param(struct sctp_association *asoc,
> > > >         case SCTP_PARAM_STATE_COOKIE:
> > > >                 asoc->peer.cookie_len =
> > > >                         ntohs(param.p->length) - sizeof(struct sctp_paramhdr);
> > > > -               asoc->peer.cookie = param.cookie->body;
> > > > +               asoc->peer.cookie = kmemdup(param.cookie->body, asoc->peer.cookie_len, gfp);
> > > > +               if (!asoc->peer.cookie)
> > > > +                       retval = 0;
> > > >                 break;
> > > >
> > > >         case SCTP_PARAM_HEARTBEAT_INFO:
> > > > diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
> > > > index 4aa03588f87b..27ddf2d8f001 100644
> > > > --- a/net/sctp/sm_sideeffect.c
> > > > +++ b/net/sctp/sm_sideeffect.c
> > > > @@ -898,6 +898,11 @@ static void sctp_cmd_new_state(struct sctp_cmd_seq *cmds,
> > > >                                                 asoc->rto_initial;
> > > >         }
> > > >
> > > > +       if (sctp_state(asoc, ESTABLISHED)) {
> > > > +               kfree(asoc->peer.cookie);
> > > > +               asoc->peer.cookie = NULL;
> > > > +       }
> > > > +
> > > >         if (sctp_state(asoc, ESTABLISHED) ||
> > > >             sctp_state(asoc, CLOSED) ||
> > > >             sctp_state(asoc, SHUTDOWN_RECEIVED)) {
> > > > --
> > > > 2.20.1
> > > >
> > > 
> > 
> 

Re: [PATCH V2] Fix memory leak in sctp_process_init

[Marcelo Ricardo Leitner]

On Fri, Jun 07, 2019 at 06:56:39AM -0400, Neil Horman wrote:
> On Thu, Jun 06, 2019 at 12:47:55PM -0300, Marcelo Ricardo Leitner wrote:
> > On Wed, Jun 05, 2019 at 07:20:10AM -0400, Neil Horman wrote:
> > > On Wed, Jun 05, 2019 at 04:16:24AM +0800, Xin Long wrote:
> > > > On Tue, Jun 4, 2019 at 4:34 AM Neil Horman <nhorman@tuxdriver.com> wrote:
> > > > >
> > > > > syzbot found the following leak in sctp_process_init
> > > > > BUG: memory leak
> > > > > unreferenced object 0xffff88810ef68400 (size 1024):
> > > > >   comm "syz-executor273", pid 7046, jiffies 4294945598 (age 28.770s)
> > > > >   hex dump (first 32 bytes):
> > > > >     1d de 28 8d de 0b 1b e3 b5 c2 f9 68 fd 1a 97 25  ..(........h...%
> > > > >     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
> > > > >   backtrace:
> > > > >     [<00000000a02cebbd>] kmemleak_alloc_recursive include/linux/kmemleak.h:55
> > > > > [inline]
> > > > >     [<00000000a02cebbd>] slab_post_alloc_hook mm/slab.h:439 [inline]
> > > > >     [<00000000a02cebbd>] slab_alloc mm/slab.c:3326 [inline]
> > > > >     [<00000000a02cebbd>] __do_kmalloc mm/slab.c:3658 [inline]
> > > > >     [<00000000a02cebbd>] __kmalloc_track_caller+0x15d/0x2c0 mm/slab.c:3675
> > > > >     [<000000009e6245e6>] kmemdup+0x27/0x60 mm/util.c:119
> > > > >     [<00000000dfdc5d2d>] kmemdup include/linux/string.h:432 [inline]
> > > > >     [<00000000dfdc5d2d>] sctp_process_init+0xa7e/0xc20
> > > > > net/sctp/sm_make_chunk.c:2437
> > > > >     [<00000000b58b62f8>] sctp_cmd_process_init net/sctp/sm_sideeffect.c:682
> > > > > [inline]
> > > > >     [<00000000b58b62f8>] sctp_cmd_interpreter net/sctp/sm_sideeffect.c:1384
> > > > > [inline]
> > > > >     [<00000000b58b62f8>] sctp_side_effects net/sctp/sm_sideeffect.c:1194
> > > > > [inline]
> > > > >     [<00000000b58b62f8>] sctp_do_sm+0xbdc/0x1d60 net/sctp/sm_sideeffect.c:1165
> > > > >     [<0000000044e11f96>] sctp_assoc_bh_rcv+0x13c/0x200
> > > > > net/sctp/associola.c:1074
> > > > >     [<00000000ec43804d>] sctp_inq_push+0x7f/0xb0 net/sctp/inqueue.c:95
> > > > >     [<00000000726aa954>] sctp_backlog_rcv+0x5e/0x2a0 net/sctp/input.c:354
> > > > >     [<00000000d9e249a8>] sk_backlog_rcv include/net/sock.h:950 [inline]
> > > > >     [<00000000d9e249a8>] __release_sock+0xab/0x110 net/core/sock.c:2418
> > > > >     [<00000000acae44fa>] release_sock+0x37/0xd0 net/core/sock.c:2934
> > > > >     [<00000000963cc9ae>] sctp_sendmsg+0x2c0/0x990 net/sctp/socket.c:2122
> > > > >     [<00000000a7fc7565>] inet_sendmsg+0x64/0x120 net/ipv4/af_inet.c:802
> > > > >     [<00000000b732cbd3>] sock_sendmsg_nosec net/socket.c:652 [inline]
> > > > >     [<00000000b732cbd3>] sock_sendmsg+0x54/0x70 net/socket.c:671
> > > > >     [<00000000274c57ab>] ___sys_sendmsg+0x393/0x3c0 net/socket.c:2292
> > > > >     [<000000008252aedb>] __sys_sendmsg+0x80/0xf0 net/socket.c:2330
> > > > >     [<00000000f7bf23d1>] __do_sys_sendmsg net/socket.c:2339 [inline]
> > > > >     [<00000000f7bf23d1>] __se_sys_sendmsg net/socket.c:2337 [inline]
> > > > >     [<00000000f7bf23d1>] __x64_sys_sendmsg+0x23/0x30 net/socket.c:2337
> > > > >     [<00000000a8b4131f>] do_syscall_64+0x76/0x1a0 arch/x86/entry/common.c:3
> > > > >
> > > > > The problem was that the peer.cookie value points to an skb allocated
> > > > > area on the first pass through this function, at which point it is
> > > > > overwritten with a heap allocated value, but in certain cases, where a
> > > > > COOKIE_ECHO chunk is included in the packet, a second pass through
> > > > > sctp_process_init is made, where the cookie value is re-allocated,
> > > > > leaking the first allocation.
> > > > This's not gonna happen, as after processing INIT, the temp asoc will be
> > > > deleted on the server side. Besides, from the reproducer:
> > > > 
> > > >   https://syzkaller.appspot.com/x/repro.syz?x=10e32f8ca00000
> > > > 
> > > > Packet(INIT|COOKIE_ECHO) can't be made in here.
> > > > 
> > > > The call trace says the leak happened when processing INIT_ACK on the
> > > > client side, as Marcelo noticed.  Then it can be triggered by:
> > > > 
> > > > 1. sctp_sf_do_5_1C_ack() -> SCTP_CMD_PEER_INIT -> sctp_process_init():
> > > >    where it "goto clean_up" after sctp_process_param(), but in 'cleanup'
> > > >    path, it doesn't do any freeup for the memdups of sctp_process_param().
> > > >    then the client T1 retrans INIT, and later get INIT_ACK again from the
> > > >    peer, and go to sctp_process_init() to memdup().
> > > > 
> > > > 2. sctp_sf_cookie_echoed_err() -> sctp_sf_do_5_2_6_stale():
> > > >    where the asoc state will go from COOKIE_ECHOED back to COOKIE_WAIT,
> > > >    and T1 starts to retrans INIT, and later it will get INIT_ACK again
> > > >    to sctp_process_init() and memdup().
> > > > 
> > > ok, you may well be right as to the path that we take to get here, but the root
> > > cause is the same, multiple passes through sctp_process_init without freeing the
> > > previously memduped memory.  Thats why I would think my patch is fixing the
> > > issue, because now we're always duping the cookie memory and always freeing it
> > > when we transition to the ESTABLISHED state.
> > > 
> > > As for the other variables (peer_[random|chunks|hmacs]), I'm not sure why we're
> > > not seeing leak reports of those variables.
> > 
> > This actually also works for peer.cookie as well, as now they are
> > allocated and freed in the same places.
> > 
> > I talked with Xin and I agree that this patch didn't really fix the
> > leak. It moved the allocation from sctp_process_init() to
> > sctp_process_param() which is called by sctp_process_init and there is
> > nothing avoiding multiple allocations on these vars once INIT_ACK is
> > retransmited.
> > 
> > The reproducer works by having the client to re-request cookies by
> > setting a short lifetime. So
> > Client sends INIT
> >                        Server replies INIT_ACK with cookie
> > Client echoes the cookie
> >                        Server rejects because the cookie expired
> > Client sends INIT again, to ask for another cookie
> >   (sctp_sf_do_5_2_6_stale)
> >                        Server sends INIT_ACK again with a new cookie
> > Client calls sctp_process_init again, from
> >   -> sctp_sf_do_5_1C_ack()
> >    -> sctp_cmd_process_init()
> >     -> sctp_process_init()
> >      -> sctp_process_param()
> >         ^-- leak happens
> > 	               Server may or may not accept the new cookie,
> > 		       leading to further leaks if not
> If this is the case, it seems like the most reasonable fix then is to just
> check to see if peer.cookie is non-null prior to calling kmemdup, and freeing
> the old cookie first, no?

That probably would be clearer and easier to maintain later, yes.
Xin?

  Marcelo

> 
> Neil
> 
> > (this is his 2nd case above)
> > 
> > Yet somehow the patch changed the dynamics and made the test pass (4x
> > 10mins each run) here. Uff.
> > 
> >   Marcelo
> > 
> > > 
> > > Neil
> > > 
> > > > As on either above, asoc's never been to ESTABLISHED state,
> > > > asoc->peer.cookie can be not freed, and this patch won't work.
> > > > But yes, it's nice to have this patch, just not to fix this memleak.
> > 
> > +1 (read: won't hurt -stable and no need to revert)
> > 
> > > > 
> > > > I tracked the code, this memleak was triggered by case 2, so I think
> > > > you also need to add something like:
> > > > 
> > > > @@ -881,6 +893,18 @@ static void sctp_cmd_new_state(struct sctp_cmd_seq *cmds,
> > > >                                                 asoc->rto_initial;
> > > >                 asoc->timeouts[SCTP_EVENT_TIMEOUT_T1_COOKIE] =
> > > >                                                 asoc->rto_initial;
> > > > +
> > > > +               if (asoc->peer.cookie) {
> > > > +                       kfree(asoc->peer.cookie);
> > > > +                       kfree(asoc->peer.peer_random);
> > > > +                       kfree(asoc->peer.peer_chunks);
> > > > +                       kfree(asoc->peer.peer_hmacs);
> > > > +
> > > > +                       asoc->peer.cookie = NULL;
> > > > +                       asoc->peer.peer_random = NULL;
> > > > +                       asoc->peer.peer_random = NULL;
> > > > +                       asoc->peer.peer_hmacs = NULL;
> > > > +               }
> > > >         }
> > > > 
> > > > and the same thing may also be needed in sctp_cmd_process_init() on the
> > > > err path for case 1.
> > > > 
> > > > >
> > > > > Fix is to always allocate the cookie value, and free it when we are done
> > > > > using it.
> > > > >
> > > > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > > > > Reported-by: syzbot+f7e9153b037eac9b1df8@syzkaller.appspotmail.com
> > > > > CC: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> > > > > CC: "David S. Miller" <davem@davemloft.net>
> > > > > CC: netdev@vger.kernel.org
> > > > >
> > > > > ---
> > > > > Change notes
> > > > >
> > > > > V1->V2:
> > > > >   * Removed an accidental double free I let slip in in
> > > > > sctp_association_free
> > > > >   * Removed now unused cookie variable
> > > > > ---
> > > > >  net/sctp/sm_make_chunk.c | 13 +++----------
> > > > >  net/sctp/sm_sideeffect.c |  5 +++++
> > > > >  2 files changed, 8 insertions(+), 10 deletions(-)
> > > > >
> > > > > diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> > > > > index 72e74503f9fc..8e12e19fe42d 100644
> > > > > --- a/net/sctp/sm_make_chunk.c
> > > > > +++ b/net/sctp/sm_make_chunk.c
> > > > > @@ -2327,7 +2327,6 @@ int sctp_process_init(struct sctp_association *asoc, struct sctp_chunk *chunk,
> > > > >         union sctp_addr addr;
> > > > >         struct sctp_af *af;
> > > > >         int src_match = 0;
> > > > > -       char *cookie;
> > > > >
> > > > >         /* We must include the address that the INIT packet came from.
> > > > >          * This is the only address that matters for an INIT packet.
> > > > > @@ -2431,14 +2430,6 @@ int sctp_process_init(struct sctp_association *asoc, struct sctp_chunk *chunk,
> > > > >         /* Peer Rwnd   : Current calculated value of the peer's rwnd.  */
> > > > >         asoc->peer.rwnd = asoc->peer.i.a_rwnd;
> > > > >
> > > > > -       /* Copy cookie in case we need to resend COOKIE-ECHO. */
> > > > > -       cookie = asoc->peer.cookie;
> > > > > -       if (cookie) {
> > > > > -               asoc->peer.cookie = kmemdup(cookie, asoc->peer.cookie_len, gfp);
> > > > > -               if (!asoc->peer.cookie)
> > > > > -                       goto clean_up;
> > > > > -       }
> > > > > -
> > > > >         /* RFC 2960 7.2.1 The initial value of ssthresh MAY be arbitrarily
> > > > >          * high (for example, implementations MAY use the size of the receiver
> > > > >          * advertised window).
> > > > > @@ -2607,7 +2598,9 @@ static int sctp_process_param(struct sctp_association *asoc,
> > > > >         case SCTP_PARAM_STATE_COOKIE:
> > > > >                 asoc->peer.cookie_len =
> > > > >                         ntohs(param.p->length) - sizeof(struct sctp_paramhdr);
> > > > > -               asoc->peer.cookie = param.cookie->body;
> > > > > +               asoc->peer.cookie = kmemdup(param.cookie->body, asoc->peer.cookie_len, gfp);
> > > > > +               if (!asoc->peer.cookie)
> > > > > +                       retval = 0;
> > > > >                 break;
> > > > >
> > > > >         case SCTP_PARAM_HEARTBEAT_INFO:
> > > > > diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
> > > > > index 4aa03588f87b..27ddf2d8f001 100644
> > > > > --- a/net/sctp/sm_sideeffect.c
> > > > > +++ b/net/sctp/sm_sideeffect.c
> > > > > @@ -898,6 +898,11 @@ static void sctp_cmd_new_state(struct sctp_cmd_seq *cmds,
> > > > >                                                 asoc->rto_initial;
> > > > >         }
> > > > >
> > > > > +       if (sctp_state(asoc, ESTABLISHED)) {
> > > > > +               kfree(asoc->peer.cookie);
> > > > > +               asoc->peer.cookie = NULL;
> > > > > +       }
> > > > > +
> > > > >         if (sctp_state(asoc, ESTABLISHED) ||
> > > > >             sctp_state(asoc, CLOSED) ||
> > > > >             sctp_state(asoc, SHUTDOWN_RECEIVED)) {
> > > > > --
> > > > > 2.20.1
> > > > >
> > > > 
> > > 
> > 

Re: [PATCH V2] Fix memory leak in sctp_process_init

[Xin Long]

On Fri, Jun 7, 2019 at 8:48 PM Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
>
> On Fri, Jun 07, 2019 at 06:56:39AM -0400, Neil Horman wrote:
> > On Thu, Jun 06, 2019 at 12:47:55PM -0300, Marcelo Ricardo Leitner wrote:
> > > On Wed, Jun 05, 2019 at 07:20:10AM -0400, Neil Horman wrote:
> > > > On Wed, Jun 05, 2019 at 04:16:24AM +0800, Xin Long wrote:
> > > > > On Tue, Jun 4, 2019 at 4:34 AM Neil Horman <nhorman@tuxdriver.com> wrote:
> > > > > >
> > > > > > syzbot found the following leak in sctp_process_init
> > > > > > BUG: memory leak
> > > > > > unreferenced object 0xffff88810ef68400 (size 1024):
> > > > > >   comm "syz-executor273", pid 7046, jiffies 4294945598 (age 28.770s)
> > > > > >   hex dump (first 32 bytes):
> > > > > >     1d de 28 8d de 0b 1b e3 b5 c2 f9 68 fd 1a 97 25  ..(........h...%
> > > > > >     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
> > > > > >   backtrace:
> > > > > >     [<00000000a02cebbd>] kmemleak_alloc_recursive include/linux/kmemleak.h:55
> > > > > > [inline]
> > > > > >     [<00000000a02cebbd>] slab_post_alloc_hook mm/slab.h:439 [inline]
> > > > > >     [<00000000a02cebbd>] slab_alloc mm/slab.c:3326 [inline]
> > > > > >     [<00000000a02cebbd>] __do_kmalloc mm/slab.c:3658 [inline]
> > > > > >     [<00000000a02cebbd>] __kmalloc_track_caller+0x15d/0x2c0 mm/slab.c:3675
> > > > > >     [<000000009e6245e6>] kmemdup+0x27/0x60 mm/util.c:119
> > > > > >     [<00000000dfdc5d2d>] kmemdup include/linux/string.h:432 [inline]
> > > > > >     [<00000000dfdc5d2d>] sctp_process_init+0xa7e/0xc20
> > > > > > net/sctp/sm_make_chunk.c:2437
> > > > > >     [<00000000b58b62f8>] sctp_cmd_process_init net/sctp/sm_sideeffect.c:682
> > > > > > [inline]
> > > > > >     [<00000000b58b62f8>] sctp_cmd_interpreter net/sctp/sm_sideeffect.c:1384
> > > > > > [inline]
> > > > > >     [<00000000b58b62f8>] sctp_side_effects net/sctp/sm_sideeffect.c:1194
> > > > > > [inline]
> > > > > >     [<00000000b58b62f8>] sctp_do_sm+0xbdc/0x1d60 net/sctp/sm_sideeffect.c:1165
> > > > > >     [<0000000044e11f96>] sctp_assoc_bh_rcv+0x13c/0x200
> > > > > > net/sctp/associola.c:1074
> > > > > >     [<00000000ec43804d>] sctp_inq_push+0x7f/0xb0 net/sctp/inqueue.c:95
> > > > > >     [<00000000726aa954>] sctp_backlog_rcv+0x5e/0x2a0 net/sctp/input.c:354
> > > > > >     [<00000000d9e249a8>] sk_backlog_rcv include/net/sock.h:950 [inline]
> > > > > >     [<00000000d9e249a8>] __release_sock+0xab/0x110 net/core/sock.c:2418
> > > > > >     [<00000000acae44fa>] release_sock+0x37/0xd0 net/core/sock.c:2934
> > > > > >     [<00000000963cc9ae>] sctp_sendmsg+0x2c0/0x990 net/sctp/socket.c:2122
> > > > > >     [<00000000a7fc7565>] inet_sendmsg+0x64/0x120 net/ipv4/af_inet.c:802
> > > > > >     [<00000000b732cbd3>] sock_sendmsg_nosec net/socket.c:652 [inline]
> > > > > >     [<00000000b732cbd3>] sock_sendmsg+0x54/0x70 net/socket.c:671
> > > > > >     [<00000000274c57ab>] ___sys_sendmsg+0x393/0x3c0 net/socket.c:2292
> > > > > >     [<000000008252aedb>] __sys_sendmsg+0x80/0xf0 net/socket.c:2330
> > > > > >     [<00000000f7bf23d1>] __do_sys_sendmsg net/socket.c:2339 [inline]
> > > > > >     [<00000000f7bf23d1>] __se_sys_sendmsg net/socket.c:2337 [inline]
> > > > > >     [<00000000f7bf23d1>] __x64_sys_sendmsg+0x23/0x30 net/socket.c:2337
> > > > > >     [<00000000a8b4131f>] do_syscall_64+0x76/0x1a0 arch/x86/entry/common.c:3
> > > > > >
> > > > > > The problem was that the peer.cookie value points to an skb allocated
> > > > > > area on the first pass through this function, at which point it is
> > > > > > overwritten with a heap allocated value, but in certain cases, where a
> > > > > > COOKIE_ECHO chunk is included in the packet, a second pass through
> > > > > > sctp_process_init is made, where the cookie value is re-allocated,
> > > > > > leaking the first allocation.
> > > > > This's not gonna happen, as after processing INIT, the temp asoc will be
> > > > > deleted on the server side. Besides, from the reproducer:
> > > > >
> > > > >   https://syzkaller.appspot.com/x/repro.syz?x=10e32f8ca00000
> > > > >
> > > > > Packet(INIT|COOKIE_ECHO) can't be made in here.
> > > > >
> > > > > The call trace says the leak happened when processing INIT_ACK on the
> > > > > client side, as Marcelo noticed.  Then it can be triggered by:
> > > > >
> > > > > 1. sctp_sf_do_5_1C_ack() -> SCTP_CMD_PEER_INIT -> sctp_process_init():
> > > > >    where it "goto clean_up" after sctp_process_param(), but in 'cleanup'
> > > > >    path, it doesn't do any freeup for the memdups of sctp_process_param().
> > > > >    then the client T1 retrans INIT, and later get INIT_ACK again from the
> > > > >    peer, and go to sctp_process_init() to memdup().
> > > > >
> > > > > 2. sctp_sf_cookie_echoed_err() -> sctp_sf_do_5_2_6_stale():
> > > > >    where the asoc state will go from COOKIE_ECHOED back to COOKIE_WAIT,
> > > > >    and T1 starts to retrans INIT, and later it will get INIT_ACK again
> > > > >    to sctp_process_init() and memdup().
> > > > >
> > > > ok, you may well be right as to the path that we take to get here, but the root
> > > > cause is the same, multiple passes through sctp_process_init without freeing the
> > > > previously memduped memory.  Thats why I would think my patch is fixing the
> > > > issue, because now we're always duping the cookie memory and always freeing it
> > > > when we transition to the ESTABLISHED state.
> > > >
> > > > As for the other variables (peer_[random|chunks|hmacs]), I'm not sure why we're
> > > > not seeing leak reports of those variables.
> > >
> > > This actually also works for peer.cookie as well, as now they are
> > > allocated and freed in the same places.
> > >
> > > I talked with Xin and I agree that this patch didn't really fix the
> > > leak. It moved the allocation from sctp_process_init() to
> > > sctp_process_param() which is called by sctp_process_init and there is
> > > nothing avoiding multiple allocations on these vars once INIT_ACK is
> > > retransmited.
> > >
> > > The reproducer works by having the client to re-request cookies by
> > > setting a short lifetime. So
> > > Client sends INIT
> > >                        Server replies INIT_ACK with cookie
> > > Client echoes the cookie
> > >                        Server rejects because the cookie expired
> > > Client sends INIT again, to ask for another cookie
> > >   (sctp_sf_do_5_2_6_stale)
> > >                        Server sends INIT_ACK again with a new cookie
> > > Client calls sctp_process_init again, from
> > >   -> sctp_sf_do_5_1C_ack()
> > >    -> sctp_cmd_process_init()
> > >     -> sctp_process_init()
> > >      -> sctp_process_param()
> > >         ^-- leak happens
> > >                    Server may or may not accept the new cookie,
> > >                    leading to further leaks if not
> > If this is the case, it seems like the most reasonable fix then is to just
> > check to see if peer.cookie is non-null prior to calling kmemdup, and freeing
> > the old cookie first, no?
>
> That probably would be clearer and easier to maintain later, yes.
> Xin?
Sure, looks good to me. :)

>
>   Marcelo
>
> >
> > Neil
> >
> > > (this is his 2nd case above)
> > >
> > > Yet somehow the patch changed the dynamics and made the test pass (4x
> > > 10mins each run) here. Uff.
> > >
> > >   Marcelo
> > >
> > > >
> > > > Neil
> > > >
> > > > > As on either above, asoc's never been to ESTABLISHED state,
> > > > > asoc->peer.cookie can be not freed, and this patch won't work.
> > > > > But yes, it's nice to have this patch, just not to fix this memleak.
> > >
> > > +1 (read: won't hurt -stable and no need to revert)
> > >
> > > > >
> > > > > I tracked the code, this memleak was triggered by case 2, so I think
> > > > > you also need to add something like:
> > > > >
> > > > > @@ -881,6 +893,18 @@ static void sctp_cmd_new_state(struct sctp_cmd_seq *cmds,
> > > > >                                                 asoc->rto_initial;
> > > > >                 asoc->timeouts[SCTP_EVENT_TIMEOUT_T1_COOKIE] =
> > > > >                                                 asoc->rto_initial;
> > > > > +
> > > > > +               if (asoc->peer.cookie) {
> > > > > +                       kfree(asoc->peer.cookie);
> > > > > +                       kfree(asoc->peer.peer_random);
> > > > > +                       kfree(asoc->peer.peer_chunks);
> > > > > +                       kfree(asoc->peer.peer_hmacs);
> > > > > +
> > > > > +                       asoc->peer.cookie = NULL;
> > > > > +                       asoc->peer.peer_random = NULL;
> > > > > +                       asoc->peer.peer_random = NULL;
> > > > > +                       asoc->peer.peer_hmacs = NULL;
> > > > > +               }
> > > > >         }
> > > > >
> > > > > and the same thing may also be needed in sctp_cmd_process_init() on the
> > > > > err path for case 1.
> > > > >
> > > > > >
> > > > > > Fix is to always allocate the cookie value, and free it when we are done
> > > > > > using it.
> > > > > >
> > > > > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > > > > > Reported-by: syzbot+f7e9153b037eac9b1df8@syzkaller.appspotmail.com
> > > > > > CC: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> > > > > > CC: "David S. Miller" <davem@davemloft.net>
> > > > > > CC: netdev@vger.kernel.org
> > > > > >
> > > > > > ---
> > > > > > Change notes
> > > > > >
> > > > > > V1->V2:
> > > > > >   * Removed an accidental double free I let slip in in
> > > > > > sctp_association_free
> > > > > >   * Removed now unused cookie variable
> > > > > > ---
> > > > > >  net/sctp/sm_make_chunk.c | 13 +++----------
> > > > > >  net/sctp/sm_sideeffect.c |  5 +++++
> > > > > >  2 files changed, 8 insertions(+), 10 deletions(-)
> > > > > >
> > > > > > diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> > > > > > index 72e74503f9fc..8e12e19fe42d 100644
> > > > > > --- a/net/sctp/sm_make_chunk.c
> > > > > > +++ b/net/sctp/sm_make_chunk.c
> > > > > > @@ -2327,7 +2327,6 @@ int sctp_process_init(struct sctp_association *asoc, struct sctp_chunk *chunk,
> > > > > >         union sctp_addr addr;
> > > > > >         struct sctp_af *af;
> > > > > >         int src_match = 0;
> > > > > > -       char *cookie;
> > > > > >
> > > > > >         /* We must include the address that the INIT packet came from.
> > > > > >          * This is the only address that matters for an INIT packet.
> > > > > > @@ -2431,14 +2430,6 @@ int sctp_process_init(struct sctp_association *asoc, struct sctp_chunk *chunk,
> > > > > >         /* Peer Rwnd   : Current calculated value of the peer's rwnd.  */
> > > > > >         asoc->peer.rwnd = asoc->peer.i.a_rwnd;
> > > > > >
> > > > > > -       /* Copy cookie in case we need to resend COOKIE-ECHO. */
> > > > > > -       cookie = asoc->peer.cookie;
> > > > > > -       if (cookie) {
> > > > > > -               asoc->peer.cookie = kmemdup(cookie, asoc->peer.cookie_len, gfp);
> > > > > > -               if (!asoc->peer.cookie)
> > > > > > -                       goto clean_up;
> > > > > > -       }
> > > > > > -
> > > > > >         /* RFC 2960 7.2.1 The initial value of ssthresh MAY be arbitrarily
> > > > > >          * high (for example, implementations MAY use the size of the receiver
> > > > > >          * advertised window).
> > > > > > @@ -2607,7 +2598,9 @@ static int sctp_process_param(struct sctp_association *asoc,
> > > > > >         case SCTP_PARAM_STATE_COOKIE:
> > > > > >                 asoc->peer.cookie_len =
> > > > > >                         ntohs(param.p->length) - sizeof(struct sctp_paramhdr);
> > > > > > -               asoc->peer.cookie = param.cookie->body;
> > > > > > +               asoc->peer.cookie = kmemdup(param.cookie->body, asoc->peer.cookie_len, gfp);
> > > > > > +               if (!asoc->peer.cookie)
> > > > > > +                       retval = 0;
> > > > > >                 break;
> > > > > >
> > > > > >         case SCTP_PARAM_HEARTBEAT_INFO:
> > > > > > diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
> > > > > > index 4aa03588f87b..27ddf2d8f001 100644
> > > > > > --- a/net/sctp/sm_sideeffect.c
> > > > > > +++ b/net/sctp/sm_sideeffect.c
> > > > > > @@ -898,6 +898,11 @@ static void sctp_cmd_new_state(struct sctp_cmd_seq *cmds,
> > > > > >                                                 asoc->rto_initial;
> > > > > >         }
> > > > > >
> > > > > > +       if (sctp_state(asoc, ESTABLISHED)) {
> > > > > > +               kfree(asoc->peer.cookie);
> > > > > > +               asoc->peer.cookie = NULL;
> > > > > > +       }
> > > > > > +
> > > > > >         if (sctp_state(asoc, ESTABLISHED) ||
> > > > > >             sctp_state(asoc, CLOSED) ||
> > > > > >             sctp_state(asoc, SHUTDOWN_RECEIVED)) {
> > > > > > --
> > > > > > 2.20.1
> > > > > >
> > > > >
> > > >
> > >

Re: [PATCH V2] Fix memory leak in sctp_process_init

[David Miller]

From: Neil Horman <nhorman@tuxdriver.com>
Date: Mon,  3 Jun 2019 16:32:59 -0400

> syzbot found the following leak in sctp_process_init
> BUG: memory leak
> unreferenced object 0xffff88810ef68400 (size 1024):
 ...
> The problem was that the peer.cookie value points to an skb allocated
> area on the first pass through this function, at which point it is
> overwritten with a heap allocated value, but in certain cases, where a
> COOKIE_ECHO chunk is included in the packet, a second pass through
> sctp_process_init is made, where the cookie value is re-allocated,
> leaking the first allocation.
> 
> Fix is to always allocate the cookie value, and free it when we are done
> using it.
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> Reported-by: syzbot+f7e9153b037eac9b1df8@syzkaller.appspotmail.com

Applied and queued up for -stable.