net/sctp: out-of-bounds access in sctp_add_bind_addr

15 messages, 4 authors, 2016-03-07 · open the first message on its own page

net/sctp: out-of-bounds access in sctp_add_bind_addr

From: Dmitry Vyukov <dvyukov@google.com>
Date: 2016-01-25 14:03:06 

Hello,

I've git the following error report while running syzkaller fuzzer:

==================================================================
BUG: KASAN: slab-out-of-bounds in memcpy+0x1d/0x40 at addr ffff88006c6361e8
Read of size 28 by task syz-executor/12551
=============================================================================
BUG kmalloc-16 (Not tainted): kasan: bad access detected
-----------------------------------------------------------------------------

INFO: Allocated in sctp_setsockopt_bindx+0xd2/0x3e0 age=12 cpu=2 pid=12551
[<     inline     >] kmalloc include/linux/slab.h:468
[<      none      >] sctp_setsockopt_bindx+0xd2/0x3e0 net/sctp/socket.c:975
[<      none      >] sctp_setsockopt+0x1493/0x3630 net/sctp/socket.c:3711
[<      none      >] sock_common_setsockopt+0x97/0xd0 net/core/sock.c:2620
[<     inline     >] SYSC_setsockopt net/socket.c:1752
[<      none      >] SyS_setsockopt+0x15b/0x250 net/socket.c:1731
[<      none      >] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185

INFO: Slab 0xffffea0001b18d80 objects=16 used=4 fp=0xffff88006c6376e0
flags=0x5fffc0000004080
INFO: Object 0xffff88006c6361e8 @offset=488 fp=0x0000000000000002
Bytes b4 ffff88006c6361d8: 00 00 00 00 00 00 00 00 2f 98 34 88 ff ff
ff ff  ......../.4.....
Object ffff88006c6361e8: 02 00 00 00 00 00 00 00 02 00 ab 07 7f 00 00
01  ................
CPU: 2 PID: 12551 Comm: syz-executor Tainted: G    B           4.5.0-rc1+ #278
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
 00000000ffffffff ffff880036397928 ffffffff8299a02d ffff88003e807900
 ffff88006c6361e8 ffff88006c636000 ffff880036397958 ffffffff81752814
 ffff88003e807900 ffffea0001b18d80 ffff88006c6361e8 ffff88006c6361e8

Call Trace:
 [<ffffffff8175ad54>] __asan_loadN+0x124/0x1a0 mm/kasan/kasan.c:512
 [<ffffffff8175b2dd>] memcpy+0x1d/0x40 mm/kasan/kasan.c:297
 [<ffffffff85dcb249>] sctp_add_bind_addr+0xa9/0x270 net/sctp/bind_addr.c:162
 [<ffffffff85dcfd66>] sctp_do_bind+0x336/0x580 net/sctp/socket.c:389
 [<ffffffff85dd16ec>] sctp_bindx_add+0xac/0x1a0 net/sctp/socket.c:471
 [<ffffffff85dd5cc8>] sctp_setsockopt_bindx+0x2f8/0x3e0 net/sctp/socket.c:1010
 [<ffffffff85dde283>] sctp_setsockopt+0x1493/0x3630 net/sctp/socket.c:3711
 [<ffffffff851f5ae7>] sock_common_setsockopt+0x97/0xd0 net/core/sock.c:2620
 [<     inline     >] SYSC_setsockopt net/socket.c:1752
 [<ffffffff851f2c3b>] SyS_setsockopt+0x15b/0x250 net/socket.c:1731
 [<ffffffff863595f6>] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185

Memory state around the buggy address:
 ffff88006c636080: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
 ffff88006c636100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
ffff88006c636180: fc fc fc fc fc fc fc fc fc fc fc fc fc 00 00 fc
                                                                ^
 ffff88006c636200: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
 ffff88006c636280: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
==================================================================


sctp_setsockopt_bindx verifies that the user-passed address has valid
len for the specified family, but then sctp_add_bind_addr copies whole
sctp_addr from there. This causes heap out-of-bounds access and can
crash kernel. Not sure if it is possible to copy out the trailing
garbage to user-space later.

On commit 92e963f50fc74041b5e9e744c330dca48e04f08d (Jan 25).

Re: net/sctp: out-of-bounds access in sctp_add_bind_addr

From: Neil Horman <nhorman@tuxdriver.com>
Date: 2016-01-25 14:32:08 

On Mon, Jan 25, 2016 at 03:02:38PM +0100, Dmitry Vyukov wrote:
Hello,

I've git the following error report while running syzkaller fuzzer:

==================================================================
BUG: KASAN: slab-out-of-bounds in memcpy+0x1d/0x40 at addr ffff88006c6361e8
Read of size 28 by task syz-executor/12551
=============================================================================
BUG kmalloc-16 (Not tainted): kasan: bad access detected
-----------------------------------------------------------------------------

INFO: Allocated in sctp_setsockopt_bindx+0xd2/0x3e0 age=12 cpu=2 pid=12551
[<     inline     >] kmalloc include/linux/slab.h:468
[<      none      >] sctp_setsockopt_bindx+0xd2/0x3e0 net/sctp/socket.c:975
[<      none      >] sctp_setsockopt+0x1493/0x3630 net/sctp/socket.c:3711
[<      none      >] sock_common_setsockopt+0x97/0xd0 net/core/sock.c:2620
[<     inline     >] SYSC_setsockopt net/socket.c:1752
[<      none      >] SyS_setsockopt+0x15b/0x250 net/socket.c:1731
[<      none      >] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185

INFO: Slab 0xffffea0001b18d80 objects=16 used=4 fp=0xffff88006c6376e0
flags=0x5fffc0000004080
INFO: Object 0xffff88006c6361e8 @offset=488 fp=0x0000000000000002
Bytes b4 ffff88006c6361d8: 00 00 00 00 00 00 00 00 2f 98 34 88 ff ff
ff ff  ......../.4.....
Object ffff88006c6361e8: 02 00 00 00 00 00 00 00 02 00 ab 07 7f 00 00
01  ................
CPU: 2 PID: 12551 Comm: syz-executor Tainted: G    B           4.5.0-rc1+ #278
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
 00000000ffffffff ffff880036397928 ffffffff8299a02d ffff88003e807900
 ffff88006c6361e8 ffff88006c636000 ffff880036397958 ffffffff81752814
 ffff88003e807900 ffffea0001b18d80 ffff88006c6361e8 ffff88006c6361e8

Call Trace:
 [<ffffffff8175ad54>] __asan_loadN+0x124/0x1a0 mm/kasan/kasan.c:512
 [<ffffffff8175b2dd>] memcpy+0x1d/0x40 mm/kasan/kasan.c:297
 [<ffffffff85dcb249>] sctp_add_bind_addr+0xa9/0x270 net/sctp/bind_addr.c:162
 [<ffffffff85dcfd66>] sctp_do_bind+0x336/0x580 net/sctp/socket.c:389
 [<ffffffff85dd16ec>] sctp_bindx_add+0xac/0x1a0 net/sctp/socket.c:471
 [<ffffffff85dd5cc8>] sctp_setsockopt_bindx+0x2f8/0x3e0 net/sctp/socket.c:1010
 [<ffffffff85dde283>] sctp_setsockopt+0x1493/0x3630 net/sctp/socket.c:3711
 [<ffffffff851f5ae7>] sock_common_setsockopt+0x97/0xd0 net/core/sock.c:2620
 [<     inline     >] SYSC_setsockopt net/socket.c:1752
 [<ffffffff851f2c3b>] SyS_setsockopt+0x15b/0x250 net/socket.c:1731
 [<ffffffff863595f6>] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185

Memory state around the buggy address:
 ffff88006c636080: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
 ffff88006c636100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
quoted
ffff88006c636180: fc fc fc fc fc fc fc fc fc fc fc fc fc 00 00 fc
                                                                ^
 ffff88006c636200: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
 ffff88006c636280: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
==================================================================


sctp_setsockopt_bindx verifies that the user-passed address has valid
len for the specified family, but then sctp_add_bind_addr copies whole
sctp_addr from there. This causes heap out-of-bounds access and can
crash kernel. Not sure if it is possible to copy out the trailing
garbage to user-space later.
It does more than that though.  sctp_setsockopt_bindx checks the following:
1) That passed addr_size is greater than zero
2) that the entire range of memory between addrs and addrs+addr_size is readable
3) That at least one address structure worth of data is available (implicit in
the while (walk_size < addr_size) loop).

Could one of the sockaddr_len fields in one of the addresses have been mangled
so that it appeared shorter in the the while loop from (3), so that a copy of
sizeof(sctp_addr in sctp_add_bind_addr overrun the allocated memory?

Neil

On commit 92e963f50fc74041b5e9e744c330dca48e04f08d (Jan 25).
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: net/sctp: out-of-bounds access in sctp_add_bind_addr

From: Dmitry Vyukov <dvyukov@google.com>
Date: 2016-01-25 14:42:38 

On Mon, Jan 25, 2016 at 3:31 PM, Neil Horman [off-list ref] wrote:
On Mon, Jan 25, 2016 at 03:02:38PM +0100, Dmitry Vyukov wrote:
quoted
Hello,

I've git the following error report while running syzkaller fuzzer:

==================================================================
BUG: KASAN: slab-out-of-bounds in memcpy+0x1d/0x40 at addr ffff88006c6361e8
Read of size 28 by task syz-executor/12551
=============================================================================
BUG kmalloc-16 (Not tainted): kasan: bad access detected
-----------------------------------------------------------------------------

INFO: Allocated in sctp_setsockopt_bindx+0xd2/0x3e0 age=12 cpu=2 pid=12551
[<     inline     >] kmalloc include/linux/slab.h:468
[<      none      >] sctp_setsockopt_bindx+0xd2/0x3e0 net/sctp/socket.c:975
[<      none      >] sctp_setsockopt+0x1493/0x3630 net/sctp/socket.c:3711
[<      none      >] sock_common_setsockopt+0x97/0xd0 net/core/sock.c:2620
[<     inline     >] SYSC_setsockopt net/socket.c:1752
[<      none      >] SyS_setsockopt+0x15b/0x250 net/socket.c:1731
[<      none      >] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185

INFO: Slab 0xffffea0001b18d80 objects=16 used=4 fp=0xffff88006c6376e0
flags=0x5fffc0000004080
INFO: Object 0xffff88006c6361e8 @offset=488 fp=0x0000000000000002
Bytes b4 ffff88006c6361d8: 00 00 00 00 00 00 00 00 2f 98 34 88 ff ff
ff ff  ......../.4.....
Object ffff88006c6361e8: 02 00 00 00 00 00 00 00 02 00 ab 07 7f 00 00
01  ................
CPU: 2 PID: 12551 Comm: syz-executor Tainted: G    B           4.5.0-rc1+ #278
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
 00000000ffffffff ffff880036397928 ffffffff8299a02d ffff88003e807900
 ffff88006c6361e8 ffff88006c636000 ffff880036397958 ffffffff81752814
 ffff88003e807900 ffffea0001b18d80 ffff88006c6361e8 ffff88006c6361e8

Call Trace:
 [<ffffffff8175ad54>] __asan_loadN+0x124/0x1a0 mm/kasan/kasan.c:512
 [<ffffffff8175b2dd>] memcpy+0x1d/0x40 mm/kasan/kasan.c:297
 [<ffffffff85dcb249>] sctp_add_bind_addr+0xa9/0x270 net/sctp/bind_addr.c:162
 [<ffffffff85dcfd66>] sctp_do_bind+0x336/0x580 net/sctp/socket.c:389
 [<ffffffff85dd16ec>] sctp_bindx_add+0xac/0x1a0 net/sctp/socket.c:471
 [<ffffffff85dd5cc8>] sctp_setsockopt_bindx+0x2f8/0x3e0 net/sctp/socket.c:1010
 [<ffffffff85dde283>] sctp_setsockopt+0x1493/0x3630 net/sctp/socket.c:3711
 [<ffffffff851f5ae7>] sock_common_setsockopt+0x97/0xd0 net/core/sock.c:2620
 [<     inline     >] SYSC_setsockopt net/socket.c:1752
 [<ffffffff851f2c3b>] SyS_setsockopt+0x15b/0x250 net/socket.c:1731
 [<ffffffff863595f6>] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185

Memory state around the buggy address:
 ffff88006c636080: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
 ffff88006c636100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
quoted
ffff88006c636180: fc fc fc fc fc fc fc fc fc fc fc fc fc 00 00 fc
                                                                ^
 ffff88006c636200: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
 ffff88006c636280: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
==================================================================


sctp_setsockopt_bindx verifies that the user-passed address has valid
len for the specified family, but then sctp_add_bind_addr copies whole
sctp_addr from there. This causes heap out-of-bounds access and can
crash kernel. Not sure if it is possible to copy out the trailing
garbage to user-space later.
It does more than that though.  sctp_setsockopt_bindx checks the following:
1) That passed addr_size is greater than zero
2) that the entire range of memory between addrs and addrs+addr_size is readable
3) That at least one address structure worth of data is available (implicit in
the while (walk_size < addr_size) loop).

Could one of the sockaddr_len fields in one of the addresses have been mangled
so that it appeared shorter in the the while loop from (3), so that a copy of
sizeof(sctp_addr in sctp_add_bind_addr overrun the allocated memory?
I may be missing something, but what I see is:

1. we check that there is at least family:
if (walk_size + sizeof(sa_family_t) > addrs_size) {

2. get family descriptor:
af = sctp_get_af_specific(sa_addr->sa_family);

3. check that the address size is enough to hold the declared family:
if (!af || (walk_size + af->sockaddr_len) > addrs_size) {

4. then we do sctp_add_bind_addr, which copies whole sctp_addr from addr:

int sctp_add_bind_addr(struct sctp_bind_addr *bp, union sctp_addr *new,
...
memcpy(&addr->a, new, sizeof(*new));

Now imagine that the addr is ipv4 (16 or so bytes, that's what we
checked) and we copy 28 bytes (ipv6) from addr.

Re: net/sctp: out-of-bounds access in sctp_add_bind_addr

From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date: 2016-01-25 14:48:11 

On Mon, Jan 25, 2016 at 03:42:14PM +0100, Dmitry Vyukov wrote:
On Mon, Jan 25, 2016 at 3:31 PM, Neil Horman [off-list ref] wrote:
quoted
On Mon, Jan 25, 2016 at 03:02:38PM +0100, Dmitry Vyukov wrote:
quoted
Hello,

I've git the following error report while running syzkaller fuzzer:

==================================================================
BUG: KASAN: slab-out-of-bounds in memcpy+0x1d/0x40 at addr ffff88006c6361e8
Read of size 28 by task syz-executor/12551
=============================================================================
BUG kmalloc-16 (Not tainted): kasan: bad access detected
-----------------------------------------------------------------------------

INFO: Allocated in sctp_setsockopt_bindx+0xd2/0x3e0 age=12 cpu=2 pid=12551
[<     inline     >] kmalloc include/linux/slab.h:468
[<      none      >] sctp_setsockopt_bindx+0xd2/0x3e0 net/sctp/socket.c:975
[<      none      >] sctp_setsockopt+0x1493/0x3630 net/sctp/socket.c:3711
[<      none      >] sock_common_setsockopt+0x97/0xd0 net/core/sock.c:2620
[<     inline     >] SYSC_setsockopt net/socket.c:1752
[<      none      >] SyS_setsockopt+0x15b/0x250 net/socket.c:1731
[<      none      >] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185

INFO: Slab 0xffffea0001b18d80 objects=16 used=4 fp=0xffff88006c6376e0
flags=0x5fffc0000004080
INFO: Object 0xffff88006c6361e8 @offset=488 fp=0x0000000000000002
Bytes b4 ffff88006c6361d8: 00 00 00 00 00 00 00 00 2f 98 34 88 ff ff
ff ff  ......../.4.....
Object ffff88006c6361e8: 02 00 00 00 00 00 00 00 02 00 ab 07 7f 00 00
01  ................
CPU: 2 PID: 12551 Comm: syz-executor Tainted: G    B           4.5.0-rc1+ #278
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
 00000000ffffffff ffff880036397928 ffffffff8299a02d ffff88003e807900
 ffff88006c6361e8 ffff88006c636000 ffff880036397958 ffffffff81752814
 ffff88003e807900 ffffea0001b18d80 ffff88006c6361e8 ffff88006c6361e8

Call Trace:
 [<ffffffff8175ad54>] __asan_loadN+0x124/0x1a0 mm/kasan/kasan.c:512
 [<ffffffff8175b2dd>] memcpy+0x1d/0x40 mm/kasan/kasan.c:297
 [<ffffffff85dcb249>] sctp_add_bind_addr+0xa9/0x270 net/sctp/bind_addr.c:162
 [<ffffffff85dcfd66>] sctp_do_bind+0x336/0x580 net/sctp/socket.c:389
 [<ffffffff85dd16ec>] sctp_bindx_add+0xac/0x1a0 net/sctp/socket.c:471
 [<ffffffff85dd5cc8>] sctp_setsockopt_bindx+0x2f8/0x3e0 net/sctp/socket.c:1010
 [<ffffffff85dde283>] sctp_setsockopt+0x1493/0x3630 net/sctp/socket.c:3711
 [<ffffffff851f5ae7>] sock_common_setsockopt+0x97/0xd0 net/core/sock.c:2620
 [<     inline     >] SYSC_setsockopt net/socket.c:1752
 [<ffffffff851f2c3b>] SyS_setsockopt+0x15b/0x250 net/socket.c:1731
 [<ffffffff863595f6>] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185

Memory state around the buggy address:
 ffff88006c636080: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
 ffff88006c636100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
quoted
ffff88006c636180: fc fc fc fc fc fc fc fc fc fc fc fc fc 00 00 fc
                                                                ^
 ffff88006c636200: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
 ffff88006c636280: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
==================================================================


sctp_setsockopt_bindx verifies that the user-passed address has valid
len for the specified family, but then sctp_add_bind_addr copies whole
sctp_addr from there. This causes heap out-of-bounds access and can
crash kernel. Not sure if it is possible to copy out the trailing
garbage to user-space later.
It does more than that though.  sctp_setsockopt_bindx checks the following:
1) That passed addr_size is greater than zero
2) that the entire range of memory between addrs and addrs+addr_size is readable
3) That at least one address structure worth of data is available (implicit in
the while (walk_size < addr_size) loop).

Could one of the sockaddr_len fields in one of the addresses have been mangled
so that it appeared shorter in the the while loop from (3), so that a copy of
sizeof(sctp_addr in sctp_add_bind_addr overrun the allocated memory?
I may be missing something, but what I see is:

1. we check that there is at least family:
if (walk_size + sizeof(sa_family_t) > addrs_size) {

2. get family descriptor:
af = sctp_get_af_specific(sa_addr->sa_family);

3. check that the address size is enough to hold the declared family:
if (!af || (walk_size + af->sockaddr_len) > addrs_size) {

4. then we do sctp_add_bind_addr, which copies whole sctp_addr from addr:

int sctp_add_bind_addr(struct sctp_bind_addr *bp, union sctp_addr *new,
...
memcpy(&addr->a, new, sizeof(*new));

Now imagine that the addr is ipv4 (16 or so bytes, that's what we
checked) and we copy 28 bytes (ipv6) from addr.
Yes, that's pretty much it I think. That memcpy should be limited to
af->sockaddr_len, it's just that af is not readily available in that
function.

  Marcelo

Re: net/sctp: out-of-bounds access in sctp_add_bind_addr

From: Neil Horman <nhorman@tuxdriver.com>
Date: 2016-01-25 16:06:22 

On Mon, Jan 25, 2016 at 12:48:02PM -0200, Marcelo Ricardo Leitner wrote:
On Mon, Jan 25, 2016 at 03:42:14PM +0100, Dmitry Vyukov wrote:
quoted
On Mon, Jan 25, 2016 at 3:31 PM, Neil Horman [off-list ref] wrote:
quoted
On Mon, Jan 25, 2016 at 03:02:38PM +0100, Dmitry Vyukov wrote:
quoted
Hello,

I've git the following error report while running syzkaller fuzzer:

==================================================================
BUG: KASAN: slab-out-of-bounds in memcpy+0x1d/0x40 at addr ffff88006c6361e8
Read of size 28 by task syz-executor/12551
=============================================================================
BUG kmalloc-16 (Not tainted): kasan: bad access detected
-----------------------------------------------------------------------------

INFO: Allocated in sctp_setsockopt_bindx+0xd2/0x3e0 age=12 cpu=2 pid=12551
[<     inline     >] kmalloc include/linux/slab.h:468
[<      none      >] sctp_setsockopt_bindx+0xd2/0x3e0 net/sctp/socket.c:975
[<      none      >] sctp_setsockopt+0x1493/0x3630 net/sctp/socket.c:3711
[<      none      >] sock_common_setsockopt+0x97/0xd0 net/core/sock.c:2620
[<     inline     >] SYSC_setsockopt net/socket.c:1752
[<      none      >] SyS_setsockopt+0x15b/0x250 net/socket.c:1731
[<      none      >] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185

INFO: Slab 0xffffea0001b18d80 objects=16 used=4 fp=0xffff88006c6376e0
flags=0x5fffc0000004080
INFO: Object 0xffff88006c6361e8 @offset=488 fp=0x0000000000000002
Bytes b4 ffff88006c6361d8: 00 00 00 00 00 00 00 00 2f 98 34 88 ff ff
ff ff  ......../.4.....
Object ffff88006c6361e8: 02 00 00 00 00 00 00 00 02 00 ab 07 7f 00 00
01  ................
CPU: 2 PID: 12551 Comm: syz-executor Tainted: G    B           4.5.0-rc1+ #278
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
 00000000ffffffff ffff880036397928 ffffffff8299a02d ffff88003e807900
 ffff88006c6361e8 ffff88006c636000 ffff880036397958 ffffffff81752814
 ffff88003e807900 ffffea0001b18d80 ffff88006c6361e8 ffff88006c6361e8

Call Trace:
 [<ffffffff8175ad54>] __asan_loadN+0x124/0x1a0 mm/kasan/kasan.c:512
 [<ffffffff8175b2dd>] memcpy+0x1d/0x40 mm/kasan/kasan.c:297
 [<ffffffff85dcb249>] sctp_add_bind_addr+0xa9/0x270 net/sctp/bind_addr.c:162
 [<ffffffff85dcfd66>] sctp_do_bind+0x336/0x580 net/sctp/socket.c:389
 [<ffffffff85dd16ec>] sctp_bindx_add+0xac/0x1a0 net/sctp/socket.c:471
 [<ffffffff85dd5cc8>] sctp_setsockopt_bindx+0x2f8/0x3e0 net/sctp/socket.c:1010
 [<ffffffff85dde283>] sctp_setsockopt+0x1493/0x3630 net/sctp/socket.c:3711
 [<ffffffff851f5ae7>] sock_common_setsockopt+0x97/0xd0 net/core/sock.c:2620
 [<     inline     >] SYSC_setsockopt net/socket.c:1752
 [<ffffffff851f2c3b>] SyS_setsockopt+0x15b/0x250 net/socket.c:1731
 [<ffffffff863595f6>] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185

Memory state around the buggy address:
 ffff88006c636080: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
 ffff88006c636100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
quoted
ffff88006c636180: fc fc fc fc fc fc fc fc fc fc fc fc fc 00 00 fc
                                                                ^
 ffff88006c636200: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
 ffff88006c636280: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
==================================================================


sctp_setsockopt_bindx verifies that the user-passed address has valid
len for the specified family, but then sctp_add_bind_addr copies whole
sctp_addr from there. This causes heap out-of-bounds access and can
crash kernel. Not sure if it is possible to copy out the trailing
garbage to user-space later.
It does more than that though.  sctp_setsockopt_bindx checks the following:
1) That passed addr_size is greater than zero
2) that the entire range of memory between addrs and addrs+addr_size is readable
3) That at least one address structure worth of data is available (implicit in
the while (walk_size < addr_size) loop).

Could one of the sockaddr_len fields in one of the addresses have been mangled
so that it appeared shorter in the the while loop from (3), so that a copy of
sizeof(sctp_addr in sctp_add_bind_addr overrun the allocated memory?
I may be missing something, but what I see is:

1. we check that there is at least family:
if (walk_size + sizeof(sa_family_t) > addrs_size) {

2. get family descriptor:
af = sctp_get_af_specific(sa_addr->sa_family);

3. check that the address size is enough to hold the declared family:
if (!af || (walk_size + af->sockaddr_len) > addrs_size) {

4. then we do sctp_add_bind_addr, which copies whole sctp_addr from addr:

int sctp_add_bind_addr(struct sctp_bind_addr *bp, union sctp_addr *new,
...
memcpy(&addr->a, new, sizeof(*new));

Now imagine that the addr is ipv4 (16 or so bytes, that's what we
checked) and we copy 28 bytes (ipv6) from addr.
Yes, that's pretty much it I think. That memcpy should be limited to
af->sockaddr_len, it's just that af is not readily available in that
function.
Yeah, ok, we're on the same page.  If the size of the sctp_addr struct is larger
than the size that the address family specifies, we're up the creek.  We should
augment sctp_add_bind_addr to take the family length as a parameter and either
limit the copy to the min of the sruct size and the family size

Neil

  Marcelo

--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: net/sctp: out-of-bounds access in sctp_add_bind_addr

From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date: 2016-01-25 16:16:14 

Something like this. Builds, but UNTESTED.
Uses union sizeof where possible but when reading from a buffer that is
not aligned to it, like that user supplied one. Then relies on
af->sockaddr_len

--8<--

---
 include/net/sctp/structs.h |  2 +-
 net/sctp/bind_addr.c       | 14 ++++++++------
 net/sctp/protocol.c        |  1 +
 net/sctp/sm_make_chunk.c   |  2 +-
 net/sctp/socket.c          |  5 +++--
 5 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 20e72129be1ce0063eeafcbaadcee1f37e0c614c..97ba8a8c466f5c50bdc87ec578792e56553baa91 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -1099,7 +1099,7 @@ int sctp_bind_addr_dup(struct sctp_bind_addr *dest,
 			const struct sctp_bind_addr *src,
 			gfp_t gfp);
 int sctp_add_bind_addr(struct sctp_bind_addr *, union sctp_addr *,
-		       __u8 addr_state, gfp_t gfp);
+		       int new_size, __u8 addr_state, gfp_t gfp);
 int sctp_del_bind_addr(struct sctp_bind_addr *, union sctp_addr *);
 int sctp_bind_addr_match(struct sctp_bind_addr *, const union sctp_addr *,
 			 struct sctp_sock *);
diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c
index 871cdf9567e6bc9c13cb1077dc6866a67e6e4367..80129d10a0af9c33e7348b79d010b9e5e948e584 100644
--- a/net/sctp/bind_addr.c
+++ b/net/sctp/bind_addr.c
@@ -111,7 +111,8 @@ int sctp_bind_addr_dup(struct sctp_bind_addr *dest,
 	dest->port = src->port;
 
 	list_for_each_entry(addr, &src->address_list, list) {
-		error = sctp_add_bind_addr(dest, &addr->a, 1, gfp);
+		error = sctp_add_bind_addr(dest, &addr->a, sizeof(addr->a),
+					   1, gfp);
 		if (error < 0)
 			break;
 	}
@@ -150,7 +151,7 @@ void sctp_bind_addr_free(struct sctp_bind_addr *bp)
 
 /* Add an address to the bind address list in the SCTP_bind_addr structure. */
 int sctp_add_bind_addr(struct sctp_bind_addr *bp, union sctp_addr *new,
-		       __u8 addr_state, gfp_t gfp)
+		       int new_size, __u8 addr_state, gfp_t gfp)
 {
 	struct sctp_sockaddr_entry *addr;
 
@@ -159,7 +160,7 @@ int sctp_add_bind_addr(struct sctp_bind_addr *bp, union sctp_addr *new,
 	if (!addr)
 		return -ENOMEM;
 
-	memcpy(&addr->a, new, sizeof(*new));
+	memcpy(&addr->a, new, min_t(size_t, sizeof(*new), new_size));
 
 	/* Fix up the port if it has not yet been set.
 	 * Both v4 and v6 have the port at the same offset.
@@ -291,7 +292,8 @@ int sctp_raw_to_bind_addrs(struct sctp_bind_addr *bp, __u8 *raw_addr_list,
 		}
 
 		af->from_addr_param(&addr, rawaddr, htons(port), 0);
-		retval = sctp_add_bind_addr(bp, &addr, SCTP_ADDR_SRC, gfp);
+		retval = sctp_add_bind_addr(bp, &addr, sizeof(addr),
+					    SCTP_ADDR_SRC, gfp);
 		if (retval) {
 			/* Can't finish building the list, clean up. */
 			sctp_bind_addr_clean(bp);
@@ -453,8 +455,8 @@ static int sctp_copy_one_addr(struct net *net, struct sctp_bind_addr *dest,
 		    (((AF_INET6 == addr->sa.sa_family) &&
 		      (flags & SCTP_ADDR6_ALLOWED) &&
 		      (flags & SCTP_ADDR6_PEERSUPP))))
-			error = sctp_add_bind_addr(dest, addr, SCTP_ADDR_SRC,
-						    gfp);
+			error = sctp_add_bind_addr(dest, addr, sizeof(addr),
+						   SCTP_ADDR_SRC, gfp);
 	}
 
 	return error;
diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index ab0d538a74ed593571cfaef02cd1bb7ce872abe6..2fb609008311f51344704d82f21b4de9f08253da 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -214,6 +214,7 @@ int sctp_copy_local_addr_list(struct net *net, struct sctp_bind_addr *bp,
 			      (copy_flags & SCTP_ADDR6_ALLOWED) &&
 			      (copy_flags & SCTP_ADDR6_PEERSUPP)))) {
 				error = sctp_add_bind_addr(bp, &addr->a,
+						    sizeof(addr->a),
 						    SCTP_ADDR_SRC, GFP_ATOMIC);
 				if (error)
 					goto end_copy;
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index 5d6a03fad3789a12290f5f14c5a7efa69c98f41a..1b91e9760fe514db6d89457e1d5da9e02800745e 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -1830,7 +1830,7 @@ no_hmac:
 	/* Also, add the destination address. */
 	if (list_empty(&retval->base.bind_addr.address_list)) {
 		sctp_add_bind_addr(&retval->base.bind_addr, &chunk->dest,
-				SCTP_ADDR_SRC, GFP_ATOMIC);
+				   sizeof(chunk->dest), SCTP_ADDR_SRC, GFP_ATOMIC);
 	}
 
 	retval->next_tsn = retval->c.initial_tsn;
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 9bb80ec4c08ff06f6e629078c5a926c3def3ce23..3765f1fd06aac253ec5ee8e8bd18fffefda64d62 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -386,7 +386,8 @@ static int sctp_do_bind(struct sock *sk, union sctp_addr *addr, int len)
 	/* Add the address to the bind address list.
 	 * Use GFP_ATOMIC since BHs will be disabled.
 	 */
-	ret = sctp_add_bind_addr(bp, addr, SCTP_ADDR_SRC, GFP_ATOMIC);
+	ret = sctp_add_bind_addr(bp, addr, af->sockaddr_len,
+				 SCTP_ADDR_SRC, GFP_ATOMIC);
 
 	/* Copy back into socket for getsockname() use. */
 	if (!ret) {
@@ -576,7 +577,7 @@ static int sctp_send_asconf_add_ip(struct sock		*sk,
 			addr = addr_buf;
 			af = sctp_get_af_specific(addr->v4.sin_family);
 			memcpy(&saveaddr, addr, af->sockaddr_len);
-			retval = sctp_add_bind_addr(bp, &saveaddr,
+			retval = sctp_add_bind_addr(bp, &saveaddr, sizeof(saveaddr),
 						    SCTP_ADDR_NEW, GFP_ATOMIC);
 			addr_buf += af->sockaddr_len;
 		}
-- 
2.5.0

Re: net/sctp: out-of-bounds access in sctp_add_bind_addr

From: Neil Horman <nhorman@tuxdriver.com>
Date: 2016-01-25 17:29:31 

On Mon, Jan 25, 2016 at 02:16:00PM -0200, Marcelo Ricardo Leitner wrote:
quoted hunk
Something like this. Builds, but UNTESTED.
Uses union sizeof where possible but when reading from a buffer that is
not aligned to it, like that user supplied one. Then relies on
af->sockaddr_len

--8<--

---
 include/net/sctp/structs.h |  2 +-
 net/sctp/bind_addr.c       | 14 ++++++++------
 net/sctp/protocol.c        |  1 +
 net/sctp/sm_make_chunk.c   |  2 +-
 net/sctp/socket.c          |  5 +++--
 5 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 20e72129be1ce0063eeafcbaadcee1f37e0c614c..97ba8a8c466f5c50bdc87ec578792e56553baa91 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -1099,7 +1099,7 @@ int sctp_bind_addr_dup(struct sctp_bind_addr *dest,
 			const struct sctp_bind_addr *src,
 			gfp_t gfp);
 int sctp_add_bind_addr(struct sctp_bind_addr *, union sctp_addr *,
-		       __u8 addr_state, gfp_t gfp);
+		       int new_size, __u8 addr_state, gfp_t gfp);
 int sctp_del_bind_addr(struct sctp_bind_addr *, union sctp_addr *);
 int sctp_bind_addr_match(struct sctp_bind_addr *, const union sctp_addr *,
 			 struct sctp_sock *);
diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c
index 871cdf9567e6bc9c13cb1077dc6866a67e6e4367..80129d10a0af9c33e7348b79d010b9e5e948e584 100644
--- a/net/sctp/bind_addr.c
+++ b/net/sctp/bind_addr.c
@@ -111,7 +111,8 @@ int sctp_bind_addr_dup(struct sctp_bind_addr *dest,
 	dest->port = src->port;
 
 	list_for_each_entry(addr, &src->address_list, list) {
-		error = sctp_add_bind_addr(dest, &addr->a, 1, gfp);
+		error = sctp_add_bind_addr(dest, &addr->a, sizeof(addr->a),
+					   1, gfp);
 		if (error < 0)
 			break;
 	}
@@ -150,7 +151,7 @@ void sctp_bind_addr_free(struct sctp_bind_addr *bp)
 
 /* Add an address to the bind address list in the SCTP_bind_addr structure. */
 int sctp_add_bind_addr(struct sctp_bind_addr *bp, union sctp_addr *new,
-		       __u8 addr_state, gfp_t gfp)
+		       int new_size, __u8 addr_state, gfp_t gfp)
 {
 	struct sctp_sockaddr_entry *addr;
 
@@ -159,7 +160,7 @@ int sctp_add_bind_addr(struct sctp_bind_addr *bp, union sctp_addr *new,
 	if (!addr)
 		return -ENOMEM;
 
-	memcpy(&addr->a, new, sizeof(*new));
+	memcpy(&addr->a, new, min_t(size_t, sizeof(*new), new_size));
 
 	/* Fix up the port if it has not yet been set.
 	 * Both v4 and v6 have the port at the same offset.
@@ -291,7 +292,8 @@ int sctp_raw_to_bind_addrs(struct sctp_bind_addr *bp, __u8 *raw_addr_list,
 		}
 
 		af->from_addr_param(&addr, rawaddr, htons(port), 0);
-		retval = sctp_add_bind_addr(bp, &addr, SCTP_ADDR_SRC, gfp);
+		retval = sctp_add_bind_addr(bp, &addr, sizeof(addr),
+					    SCTP_ADDR_SRC, gfp);
 		if (retval) {
 			/* Can't finish building the list, clean up. */
 			sctp_bind_addr_clean(bp);
@@ -453,8 +455,8 @@ static int sctp_copy_one_addr(struct net *net, struct sctp_bind_addr *dest,
 		    (((AF_INET6 == addr->sa.sa_family) &&
 		      (flags & SCTP_ADDR6_ALLOWED) &&
 		      (flags & SCTP_ADDR6_PEERSUPP))))
-			error = sctp_add_bind_addr(dest, addr, SCTP_ADDR_SRC,
-						    gfp);
+			error = sctp_add_bind_addr(dest, addr, sizeof(addr),
+						   SCTP_ADDR_SRC, gfp);
 	}
 
 	return error;
diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index ab0d538a74ed593571cfaef02cd1bb7ce872abe6..2fb609008311f51344704d82f21b4de9f08253da 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -214,6 +214,7 @@ int sctp_copy_local_addr_list(struct net *net, struct sctp_bind_addr *bp,
 			      (copy_flags & SCTP_ADDR6_ALLOWED) &&
 			      (copy_flags & SCTP_ADDR6_PEERSUPP)))) {
 				error = sctp_add_bind_addr(bp, &addr->a,
+						    sizeof(addr->a),
 						    SCTP_ADDR_SRC, GFP_ATOMIC);
 				if (error)
 					goto end_copy;
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index 5d6a03fad3789a12290f5f14c5a7efa69c98f41a..1b91e9760fe514db6d89457e1d5da9e02800745e 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -1830,7 +1830,7 @@ no_hmac:
 	/* Also, add the destination address. */
 	if (list_empty(&retval->base.bind_addr.address_list)) {
 		sctp_add_bind_addr(&retval->base.bind_addr, &chunk->dest,
-				SCTP_ADDR_SRC, GFP_ATOMIC);
+				   sizeof(chunk->dest), SCTP_ADDR_SRC, GFP_ATOMIC);
 	}
 
 	retval->next_tsn = retval->c.initial_tsn;
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 9bb80ec4c08ff06f6e629078c5a926c3def3ce23..3765f1fd06aac253ec5ee8e8bd18fffefda64d62 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -386,7 +386,8 @@ static int sctp_do_bind(struct sock *sk, union sctp_addr *addr, int len)
 	/* Add the address to the bind address list.
 	 * Use GFP_ATOMIC since BHs will be disabled.
 	 */
-	ret = sctp_add_bind_addr(bp, addr, SCTP_ADDR_SRC, GFP_ATOMIC);
+	ret = sctp_add_bind_addr(bp, addr, af->sockaddr_len,
+				 SCTP_ADDR_SRC, GFP_ATOMIC);
 
 	/* Copy back into socket for getsockname() use. */
 	if (!ret) {
@@ -576,7 +577,7 @@ static int sctp_send_asconf_add_ip(struct sock		*sk,
 			addr = addr_buf;
 			af = sctp_get_af_specific(addr->v4.sin_family);
 			memcpy(&saveaddr, addr, af->sockaddr_len);
-			retval = sctp_add_bind_addr(bp, &saveaddr,
+			retval = sctp_add_bind_addr(bp, &saveaddr, sizeof(saveaddr),
 						    SCTP_ADDR_NEW, GFP_ATOMIC);
 			addr_buf += af->sockaddr_len;
 		}
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Agreed, that looks correct
Neil

[PATCH net] sctp: fix copying more bytes than expected in sctp_add_bind_addr

From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date: 2016-01-25 17:53:08 

Great. Dmitry, please give this a run. Local tests looked good but who
knows what syzkaller may find.

Thanks

--8<--

Dmitry reported that sctp_add_bind_addr may read more bytes than
expected in case the parameter is a IPv4 addr supplied by the user
through calls such as sctp_bindx_add(), because it always copies
sizeof(union sctp_addr) while the buffer may be just a struct
sockaddr_in, which is smaller.

This patch then fixes it by limiting the memcpy to the min between the
union size and a (new parameter) provided addr size. Where possible this
parameter still is the size of that union, except for reading from
user-provided buffers, which then it accounts for protocol type.

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---
 include/net/sctp/structs.h |  2 +-
 net/sctp/bind_addr.c       | 14 ++++++++------
 net/sctp/protocol.c        |  1 +
 net/sctp/sm_make_chunk.c   |  2 +-
 net/sctp/socket.c          |  5 +++--
 5 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 20e72129be1ce0063eeafcbaadcee1f37e0c614c..97ba8a8c466f5c50bdc87ec578792e56553baa91 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -1099,7 +1099,7 @@ int sctp_bind_addr_dup(struct sctp_bind_addr *dest,
 			const struct sctp_bind_addr *src,
 			gfp_t gfp);
 int sctp_add_bind_addr(struct sctp_bind_addr *, union sctp_addr *,
-		       __u8 addr_state, gfp_t gfp);
+		       int new_size, __u8 addr_state, gfp_t gfp);
 int sctp_del_bind_addr(struct sctp_bind_addr *, union sctp_addr *);
 int sctp_bind_addr_match(struct sctp_bind_addr *, const union sctp_addr *,
 			 struct sctp_sock *);
diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c
index 871cdf9567e6bc9c13cb1077dc6866a67e6e4367..80129d10a0af9c33e7348b79d010b9e5e948e584 100644
--- a/net/sctp/bind_addr.c
+++ b/net/sctp/bind_addr.c
@@ -111,7 +111,8 @@ int sctp_bind_addr_dup(struct sctp_bind_addr *dest,
 	dest->port = src->port;
 
 	list_for_each_entry(addr, &src->address_list, list) {
-		error = sctp_add_bind_addr(dest, &addr->a, 1, gfp);
+		error = sctp_add_bind_addr(dest, &addr->a, sizeof(addr->a),
+					   1, gfp);
 		if (error < 0)
 			break;
 	}
@@ -150,7 +151,7 @@ void sctp_bind_addr_free(struct sctp_bind_addr *bp)
 
 /* Add an address to the bind address list in the SCTP_bind_addr structure. */
 int sctp_add_bind_addr(struct sctp_bind_addr *bp, union sctp_addr *new,
-		       __u8 addr_state, gfp_t gfp)
+		       int new_size, __u8 addr_state, gfp_t gfp)
 {
 	struct sctp_sockaddr_entry *addr;
 
@@ -159,7 +160,7 @@ int sctp_add_bind_addr(struct sctp_bind_addr *bp, union sctp_addr *new,
 	if (!addr)
 		return -ENOMEM;
 
-	memcpy(&addr->a, new, sizeof(*new));
+	memcpy(&addr->a, new, min_t(size_t, sizeof(*new), new_size));
 
 	/* Fix up the port if it has not yet been set.
 	 * Both v4 and v6 have the port at the same offset.
@@ -291,7 +292,8 @@ int sctp_raw_to_bind_addrs(struct sctp_bind_addr *bp, __u8 *raw_addr_list,
 		}
 
 		af->from_addr_param(&addr, rawaddr, htons(port), 0);
-		retval = sctp_add_bind_addr(bp, &addr, SCTP_ADDR_SRC, gfp);
+		retval = sctp_add_bind_addr(bp, &addr, sizeof(addr),
+					    SCTP_ADDR_SRC, gfp);
 		if (retval) {
 			/* Can't finish building the list, clean up. */
 			sctp_bind_addr_clean(bp);
@@ -453,8 +455,8 @@ static int sctp_copy_one_addr(struct net *net, struct sctp_bind_addr *dest,
 		    (((AF_INET6 == addr->sa.sa_family) &&
 		      (flags & SCTP_ADDR6_ALLOWED) &&
 		      (flags & SCTP_ADDR6_PEERSUPP))))
-			error = sctp_add_bind_addr(dest, addr, SCTP_ADDR_SRC,
-						    gfp);
+			error = sctp_add_bind_addr(dest, addr, sizeof(addr),
+						   SCTP_ADDR_SRC, gfp);
 	}
 
 	return error;
diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index ab0d538a74ed593571cfaef02cd1bb7ce872abe6..2fb609008311f51344704d82f21b4de9f08253da 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -214,6 +214,7 @@ int sctp_copy_local_addr_list(struct net *net, struct sctp_bind_addr *bp,
 			      (copy_flags & SCTP_ADDR6_ALLOWED) &&
 			      (copy_flags & SCTP_ADDR6_PEERSUPP)))) {
 				error = sctp_add_bind_addr(bp, &addr->a,
+						    sizeof(addr->a),
 						    SCTP_ADDR_SRC, GFP_ATOMIC);
 				if (error)
 					goto end_copy;
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index 5d6a03fad3789a12290f5f14c5a7efa69c98f41a..1b91e9760fe514db6d89457e1d5da9e02800745e 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -1830,7 +1830,7 @@ no_hmac:
 	/* Also, add the destination address. */
 	if (list_empty(&retval->base.bind_addr.address_list)) {
 		sctp_add_bind_addr(&retval->base.bind_addr, &chunk->dest,
-				SCTP_ADDR_SRC, GFP_ATOMIC);
+				   sizeof(chunk->dest), SCTP_ADDR_SRC, GFP_ATOMIC);
 	}
 
 	retval->next_tsn = retval->c.initial_tsn;
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 5ca2ebfe0be83882fcb841de6fa8029b6455ef85..213be3821a1d49e0c469c4ad4e9ff055a03205c5 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -386,7 +386,8 @@ static int sctp_do_bind(struct sock *sk, union sctp_addr *addr, int len)
 	/* Add the address to the bind address list.
 	 * Use GFP_ATOMIC since BHs will be disabled.
 	 */
-	ret = sctp_add_bind_addr(bp, addr, SCTP_ADDR_SRC, GFP_ATOMIC);
+	ret = sctp_add_bind_addr(bp, addr, af->sockaddr_len,
+				 SCTP_ADDR_SRC, GFP_ATOMIC);
 
 	/* Copy back into socket for getsockname() use. */
 	if (!ret) {
@@ -576,7 +577,7 @@ static int sctp_send_asconf_add_ip(struct sock		*sk,
 			addr = addr_buf;
 			af = sctp_get_af_specific(addr->v4.sin_family);
 			memcpy(&saveaddr, addr, af->sockaddr_len);
-			retval = sctp_add_bind_addr(bp, &saveaddr,
+			retval = sctp_add_bind_addr(bp, &saveaddr, sizeof(saveaddr),
 						    SCTP_ADDR_NEW, GFP_ATOMIC);
 			addr_buf += af->sockaddr_len;
 		}
-- 
2.5.0

Re: [PATCH net] sctp: fix copying more bytes than expected in sctp_add_bind_addr

From: Dmitry Vyukov <dvyukov@google.com>
Date: 2016-01-26 13:29:13 

On Mon, Jan 25, 2016 at 6:52 PM, Marcelo Ricardo Leitner
[off-list ref] wrote:
Great. Dmitry, please give this a run. Local tests looked good but who
knows what syzkaller may find.
Now running with this patch.
quoted hunk
Thanks

--8<--

Dmitry reported that sctp_add_bind_addr may read more bytes than
expected in case the parameter is a IPv4 addr supplied by the user
through calls such as sctp_bindx_add(), because it always copies
sizeof(union sctp_addr) while the buffer may be just a struct
sockaddr_in, which is smaller.

This patch then fixes it by limiting the memcpy to the min between the
union size and a (new parameter) provided addr size. Where possible this
parameter still is the size of that union, except for reading from
user-provided buffers, which then it accounts for protocol type.

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---
 include/net/sctp/structs.h |  2 +-
 net/sctp/bind_addr.c       | 14 ++++++++------
 net/sctp/protocol.c        |  1 +
 net/sctp/sm_make_chunk.c   |  2 +-
 net/sctp/socket.c          |  5 +++--
 5 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 20e72129be1ce0063eeafcbaadcee1f37e0c614c..97ba8a8c466f5c50bdc87ec578792e56553baa91 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -1099,7 +1099,7 @@ int sctp_bind_addr_dup(struct sctp_bind_addr *dest,
                        const struct sctp_bind_addr *src,
                        gfp_t gfp);
 int sctp_add_bind_addr(struct sctp_bind_addr *, union sctp_addr *,
-                      __u8 addr_state, gfp_t gfp);
+                      int new_size, __u8 addr_state, gfp_t gfp);
 int sctp_del_bind_addr(struct sctp_bind_addr *, union sctp_addr *);
 int sctp_bind_addr_match(struct sctp_bind_addr *, const union sctp_addr *,
                         struct sctp_sock *);
diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c
index 871cdf9567e6bc9c13cb1077dc6866a67e6e4367..80129d10a0af9c33e7348b79d010b9e5e948e584 100644
--- a/net/sctp/bind_addr.c
+++ b/net/sctp/bind_addr.c
@@ -111,7 +111,8 @@ int sctp_bind_addr_dup(struct sctp_bind_addr *dest,
        dest->port = src->port;

        list_for_each_entry(addr, &src->address_list, list) {
-               error = sctp_add_bind_addr(dest, &addr->a, 1, gfp);
+               error = sctp_add_bind_addr(dest, &addr->a, sizeof(addr->a),
+                                          1, gfp);
                if (error < 0)
                        break;
        }
@@ -150,7 +151,7 @@ void sctp_bind_addr_free(struct sctp_bind_addr *bp)

 /* Add an address to the bind address list in the SCTP_bind_addr structure. */
 int sctp_add_bind_addr(struct sctp_bind_addr *bp, union sctp_addr *new,
-                      __u8 addr_state, gfp_t gfp)
+                      int new_size, __u8 addr_state, gfp_t gfp)
 {
        struct sctp_sockaddr_entry *addr;

@@ -159,7 +160,7 @@ int sctp_add_bind_addr(struct sctp_bind_addr *bp, union sctp_addr *new,
        if (!addr)
                return -ENOMEM;

-       memcpy(&addr->a, new, sizeof(*new));
+       memcpy(&addr->a, new, min_t(size_t, sizeof(*new), new_size));

        /* Fix up the port if it has not yet been set.
         * Both v4 and v6 have the port at the same offset.
@@ -291,7 +292,8 @@ int sctp_raw_to_bind_addrs(struct sctp_bind_addr *bp, __u8 *raw_addr_list,
                }

                af->from_addr_param(&addr, rawaddr, htons(port), 0);
-               retval = sctp_add_bind_addr(bp, &addr, SCTP_ADDR_SRC, gfp);
+               retval = sctp_add_bind_addr(bp, &addr, sizeof(addr),
+                                           SCTP_ADDR_SRC, gfp);
                if (retval) {
                        /* Can't finish building the list, clean up. */
                        sctp_bind_addr_clean(bp);
@@ -453,8 +455,8 @@ static int sctp_copy_one_addr(struct net *net, struct sctp_bind_addr *dest,
                    (((AF_INET6 == addr->sa.sa_family) &&
                      (flags & SCTP_ADDR6_ALLOWED) &&
                      (flags & SCTP_ADDR6_PEERSUPP))))
-                       error = sctp_add_bind_addr(dest, addr, SCTP_ADDR_SRC,
-                                                   gfp);
+                       error = sctp_add_bind_addr(dest, addr, sizeof(addr),
+                                                  SCTP_ADDR_SRC, gfp);
        }

        return error;
diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index ab0d538a74ed593571cfaef02cd1bb7ce872abe6..2fb609008311f51344704d82f21b4de9f08253da 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -214,6 +214,7 @@ int sctp_copy_local_addr_list(struct net *net, struct sctp_bind_addr *bp,
                              (copy_flags & SCTP_ADDR6_ALLOWED) &&
                              (copy_flags & SCTP_ADDR6_PEERSUPP)))) {
                                error = sctp_add_bind_addr(bp, &addr->a,
+                                                   sizeof(addr->a),
                                                    SCTP_ADDR_SRC, GFP_ATOMIC);
                                if (error)
                                        goto end_copy;
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index 5d6a03fad3789a12290f5f14c5a7efa69c98f41a..1b91e9760fe514db6d89457e1d5da9e02800745e 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -1830,7 +1830,7 @@ no_hmac:
        /* Also, add the destination address. */
        if (list_empty(&retval->base.bind_addr.address_list)) {
                sctp_add_bind_addr(&retval->base.bind_addr, &chunk->dest,
-                               SCTP_ADDR_SRC, GFP_ATOMIC);
+                                  sizeof(chunk->dest), SCTP_ADDR_SRC, GFP_ATOMIC);
        }

        retval->next_tsn = retval->c.initial_tsn;
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 5ca2ebfe0be83882fcb841de6fa8029b6455ef85..213be3821a1d49e0c469c4ad4e9ff055a03205c5 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -386,7 +386,8 @@ static int sctp_do_bind(struct sock *sk, union sctp_addr *addr, int len)
        /* Add the address to the bind address list.
         * Use GFP_ATOMIC since BHs will be disabled.
         */
-       ret = sctp_add_bind_addr(bp, addr, SCTP_ADDR_SRC, GFP_ATOMIC);
+       ret = sctp_add_bind_addr(bp, addr, af->sockaddr_len,
+                                SCTP_ADDR_SRC, GFP_ATOMIC);

        /* Copy back into socket for getsockname() use. */
        if (!ret) {
@@ -576,7 +577,7 @@ static int sctp_send_asconf_add_ip(struct sock              *sk,
                        addr = addr_buf;
                        af = sctp_get_af_specific(addr->v4.sin_family);
                        memcpy(&saveaddr, addr, af->sockaddr_len);
-                       retval = sctp_add_bind_addr(bp, &saveaddr,
+                       retval = sctp_add_bind_addr(bp, &saveaddr, sizeof(saveaddr),
                                                    SCTP_ADDR_NEW, GFP_ATOMIC);
                        addr_buf += af->sockaddr_len;
                }
--
2.5.0

Re: [PATCH net] sctp: fix copying more bytes than expected in sctp_add_bind_addr

From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date: 2016-03-07 19:44:20 

On Tue, Jan 26, 2016 at 02:28:48PM +0100, Dmitry Vyukov wrote:
On Mon, Jan 25, 2016 at 6:52 PM, Marcelo Ricardo Leitner
[off-list ref] wrote:
quoted
Great. Dmitry, please give this a run. Local tests looked good but who
knows what syzkaller may find.
Now running with this patch.
Hi Dmitry, do you remember if this one succeeded?
quoted
Thanks

--8<--

Dmitry reported that sctp_add_bind_addr may read more bytes than
expected in case the parameter is a IPv4 addr supplied by the user
through calls such as sctp_bindx_add(), because it always copies
sizeof(union sctp_addr) while the buffer may be just a struct
sockaddr_in, which is smaller.

This patch then fixes it by limiting the memcpy to the min between the
union size and a (new parameter) provided addr size. Where possible this
parameter still is the size of that union, except for reading from
user-provided buffers, which then it accounts for protocol type.

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---
 include/net/sctp/structs.h |  2 +-
 net/sctp/bind_addr.c       | 14 ++++++++------
 net/sctp/protocol.c        |  1 +
 net/sctp/sm_make_chunk.c   |  2 +-
 net/sctp/socket.c          |  5 +++--
 5 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 20e72129be1ce0063eeafcbaadcee1f37e0c614c..97ba8a8c466f5c50bdc87ec578792e56553baa91 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -1099,7 +1099,7 @@ int sctp_bind_addr_dup(struct sctp_bind_addr *dest,
                        const struct sctp_bind_addr *src,
                        gfp_t gfp);
 int sctp_add_bind_addr(struct sctp_bind_addr *, union sctp_addr *,
-                      __u8 addr_state, gfp_t gfp);
+                      int new_size, __u8 addr_state, gfp_t gfp);
 int sctp_del_bind_addr(struct sctp_bind_addr *, union sctp_addr *);
 int sctp_bind_addr_match(struct sctp_bind_addr *, const union sctp_addr *,
                         struct sctp_sock *);
diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c
index 871cdf9567e6bc9c13cb1077dc6866a67e6e4367..80129d10a0af9c33e7348b79d010b9e5e948e584 100644
--- a/net/sctp/bind_addr.c
+++ b/net/sctp/bind_addr.c
@@ -111,7 +111,8 @@ int sctp_bind_addr_dup(struct sctp_bind_addr *dest,
        dest->port = src->port;

        list_for_each_entry(addr, &src->address_list, list) {
-               error = sctp_add_bind_addr(dest, &addr->a, 1, gfp);
+               error = sctp_add_bind_addr(dest, &addr->a, sizeof(addr->a),
+                                          1, gfp);
                if (error < 0)
                        break;
        }
@@ -150,7 +151,7 @@ void sctp_bind_addr_free(struct sctp_bind_addr *bp)

 /* Add an address to the bind address list in the SCTP_bind_addr structure. */
 int sctp_add_bind_addr(struct sctp_bind_addr *bp, union sctp_addr *new,
-                      __u8 addr_state, gfp_t gfp)
+                      int new_size, __u8 addr_state, gfp_t gfp)
 {
        struct sctp_sockaddr_entry *addr;

@@ -159,7 +160,7 @@ int sctp_add_bind_addr(struct sctp_bind_addr *bp, union sctp_addr *new,
        if (!addr)
                return -ENOMEM;

-       memcpy(&addr->a, new, sizeof(*new));
+       memcpy(&addr->a, new, min_t(size_t, sizeof(*new), new_size));

        /* Fix up the port if it has not yet been set.
         * Both v4 and v6 have the port at the same offset.
@@ -291,7 +292,8 @@ int sctp_raw_to_bind_addrs(struct sctp_bind_addr *bp, __u8 *raw_addr_list,
                }

                af->from_addr_param(&addr, rawaddr, htons(port), 0);
-               retval = sctp_add_bind_addr(bp, &addr, SCTP_ADDR_SRC, gfp);
+               retval = sctp_add_bind_addr(bp, &addr, sizeof(addr),
+                                           SCTP_ADDR_SRC, gfp);
                if (retval) {
                        /* Can't finish building the list, clean up. */
                        sctp_bind_addr_clean(bp);
@@ -453,8 +455,8 @@ static int sctp_copy_one_addr(struct net *net, struct sctp_bind_addr *dest,
                    (((AF_INET6 == addr->sa.sa_family) &&
                      (flags & SCTP_ADDR6_ALLOWED) &&
                      (flags & SCTP_ADDR6_PEERSUPP))))
-                       error = sctp_add_bind_addr(dest, addr, SCTP_ADDR_SRC,
-                                                   gfp);
+                       error = sctp_add_bind_addr(dest, addr, sizeof(addr),
+                                                  SCTP_ADDR_SRC, gfp);
        }

        return error;
diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index ab0d538a74ed593571cfaef02cd1bb7ce872abe6..2fb609008311f51344704d82f21b4de9f08253da 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -214,6 +214,7 @@ int sctp_copy_local_addr_list(struct net *net, struct sctp_bind_addr *bp,
                              (copy_flags & SCTP_ADDR6_ALLOWED) &&
                              (copy_flags & SCTP_ADDR6_PEERSUPP)))) {
                                error = sctp_add_bind_addr(bp, &addr->a,
+                                                   sizeof(addr->a),
                                                    SCTP_ADDR_SRC, GFP_ATOMIC);
                                if (error)
                                        goto end_copy;
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index 5d6a03fad3789a12290f5f14c5a7efa69c98f41a..1b91e9760fe514db6d89457e1d5da9e02800745e 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -1830,7 +1830,7 @@ no_hmac:
        /* Also, add the destination address. */
        if (list_empty(&retval->base.bind_addr.address_list)) {
                sctp_add_bind_addr(&retval->base.bind_addr, &chunk->dest,
-                               SCTP_ADDR_SRC, GFP_ATOMIC);
+                                  sizeof(chunk->dest), SCTP_ADDR_SRC, GFP_ATOMIC);
        }

        retval->next_tsn = retval->c.initial_tsn;
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 5ca2ebfe0be83882fcb841de6fa8029b6455ef85..213be3821a1d49e0c469c4ad4e9ff055a03205c5 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -386,7 +386,8 @@ static int sctp_do_bind(struct sock *sk, union sctp_addr *addr, int len)
        /* Add the address to the bind address list.
         * Use GFP_ATOMIC since BHs will be disabled.
         */
-       ret = sctp_add_bind_addr(bp, addr, SCTP_ADDR_SRC, GFP_ATOMIC);
+       ret = sctp_add_bind_addr(bp, addr, af->sockaddr_len,
+                                SCTP_ADDR_SRC, GFP_ATOMIC);

        /* Copy back into socket for getsockname() use. */
        if (!ret) {
@@ -576,7 +577,7 @@ static int sctp_send_asconf_add_ip(struct sock              *sk,
                        addr = addr_buf;
                        af = sctp_get_af_specific(addr->v4.sin_family);
                        memcpy(&saveaddr, addr, af->sockaddr_len);
-                       retval = sctp_add_bind_addr(bp, &saveaddr,
+                       retval = sctp_add_bind_addr(bp, &saveaddr, sizeof(saveaddr),
                                                    SCTP_ADDR_NEW, GFP_ATOMIC);
                        addr_buf += af->sockaddr_len;
                }
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] sctp: fix copying more bytes than expected in sctp_add_bind_addr

From: Dmitry Vyukov <dvyukov@google.com>
Date: 2016-03-07 19:46:37 

Yes, it is. I never saw this bug again. Forgot to update this thread. Sorry.

On Mon, Mar 7, 2016 at 8:44 PM, Marcelo Ricardo Leitner
[off-list ref] wrote:
On Tue, Jan 26, 2016 at 02:28:48PM +0100, Dmitry Vyukov wrote:
quoted
On Mon, Jan 25, 2016 at 6:52 PM, Marcelo Ricardo Leitner
[off-list ref] wrote:
quoted
Great. Dmitry, please give this a run. Local tests looked good but who
knows what syzkaller may find.
Now running with this patch.
Hi Dmitry, do you remember if this one succeeded?
quoted
quoted
Thanks

--8<--

Dmitry reported that sctp_add_bind_addr may read more bytes than
expected in case the parameter is a IPv4 addr supplied by the user
through calls such as sctp_bindx_add(), because it always copies
sizeof(union sctp_addr) while the buffer may be just a struct
sockaddr_in, which is smaller.

This patch then fixes it by limiting the memcpy to the min between the
union size and a (new parameter) provided addr size. Where possible this
parameter still is the size of that union, except for reading from
user-provided buffers, which then it accounts for protocol type.

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---
 include/net/sctp/structs.h |  2 +-
 net/sctp/bind_addr.c       | 14 ++++++++------
 net/sctp/protocol.c        |  1 +
 net/sctp/sm_make_chunk.c   |  2 +-
 net/sctp/socket.c          |  5 +++--
 5 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 20e72129be1ce0063eeafcbaadcee1f37e0c614c..97ba8a8c466f5c50bdc87ec578792e56553baa91 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -1099,7 +1099,7 @@ int sctp_bind_addr_dup(struct sctp_bind_addr *dest,
                        const struct sctp_bind_addr *src,
                        gfp_t gfp);
 int sctp_add_bind_addr(struct sctp_bind_addr *, union sctp_addr *,
-                      __u8 addr_state, gfp_t gfp);
+                      int new_size, __u8 addr_state, gfp_t gfp);
 int sctp_del_bind_addr(struct sctp_bind_addr *, union sctp_addr *);
 int sctp_bind_addr_match(struct sctp_bind_addr *, const union sctp_addr *,
                         struct sctp_sock *);
diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c
index 871cdf9567e6bc9c13cb1077dc6866a67e6e4367..80129d10a0af9c33e7348b79d010b9e5e948e584 100644
--- a/net/sctp/bind_addr.c
+++ b/net/sctp/bind_addr.c
@@ -111,7 +111,8 @@ int sctp_bind_addr_dup(struct sctp_bind_addr *dest,
        dest->port = src->port;

        list_for_each_entry(addr, &src->address_list, list) {
-               error = sctp_add_bind_addr(dest, &addr->a, 1, gfp);
+               error = sctp_add_bind_addr(dest, &addr->a, sizeof(addr->a),
+                                          1, gfp);
                if (error < 0)
                        break;
        }
@@ -150,7 +151,7 @@ void sctp_bind_addr_free(struct sctp_bind_addr *bp)

 /* Add an address to the bind address list in the SCTP_bind_addr structure. */
 int sctp_add_bind_addr(struct sctp_bind_addr *bp, union sctp_addr *new,
-                      __u8 addr_state, gfp_t gfp)
+                      int new_size, __u8 addr_state, gfp_t gfp)
 {
        struct sctp_sockaddr_entry *addr;

@@ -159,7 +160,7 @@ int sctp_add_bind_addr(struct sctp_bind_addr *bp, union sctp_addr *new,
        if (!addr)
                return -ENOMEM;

-       memcpy(&addr->a, new, sizeof(*new));
+       memcpy(&addr->a, new, min_t(size_t, sizeof(*new), new_size));

        /* Fix up the port if it has not yet been set.
         * Both v4 and v6 have the port at the same offset.
@@ -291,7 +292,8 @@ int sctp_raw_to_bind_addrs(struct sctp_bind_addr *bp, __u8 *raw_addr_list,
                }

                af->from_addr_param(&addr, rawaddr, htons(port), 0);
-               retval = sctp_add_bind_addr(bp, &addr, SCTP_ADDR_SRC, gfp);
+               retval = sctp_add_bind_addr(bp, &addr, sizeof(addr),
+                                           SCTP_ADDR_SRC, gfp);
                if (retval) {
                        /* Can't finish building the list, clean up. */
                        sctp_bind_addr_clean(bp);
@@ -453,8 +455,8 @@ static int sctp_copy_one_addr(struct net *net, struct sctp_bind_addr *dest,
                    (((AF_INET6 == addr->sa.sa_family) &&
                      (flags & SCTP_ADDR6_ALLOWED) &&
                      (flags & SCTP_ADDR6_PEERSUPP))))
-                       error = sctp_add_bind_addr(dest, addr, SCTP_ADDR_SRC,
-                                                   gfp);
+                       error = sctp_add_bind_addr(dest, addr, sizeof(addr),
+                                                  SCTP_ADDR_SRC, gfp);
        }

        return error;
diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index ab0d538a74ed593571cfaef02cd1bb7ce872abe6..2fb609008311f51344704d82f21b4de9f08253da 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -214,6 +214,7 @@ int sctp_copy_local_addr_list(struct net *net, struct sctp_bind_addr *bp,
                              (copy_flags & SCTP_ADDR6_ALLOWED) &&
                              (copy_flags & SCTP_ADDR6_PEERSUPP)))) {
                                error = sctp_add_bind_addr(bp, &addr->a,
+                                                   sizeof(addr->a),
                                                    SCTP_ADDR_SRC, GFP_ATOMIC);
                                if (error)
                                        goto end_copy;
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index 5d6a03fad3789a12290f5f14c5a7efa69c98f41a..1b91e9760fe514db6d89457e1d5da9e02800745e 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -1830,7 +1830,7 @@ no_hmac:
        /* Also, add the destination address. */
        if (list_empty(&retval->base.bind_addr.address_list)) {
                sctp_add_bind_addr(&retval->base.bind_addr, &chunk->dest,
-                               SCTP_ADDR_SRC, GFP_ATOMIC);
+                                  sizeof(chunk->dest), SCTP_ADDR_SRC, GFP_ATOMIC);
        }

        retval->next_tsn = retval->c.initial_tsn;
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 5ca2ebfe0be83882fcb841de6fa8029b6455ef85..213be3821a1d49e0c469c4ad4e9ff055a03205c5 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -386,7 +386,8 @@ static int sctp_do_bind(struct sock *sk, union sctp_addr *addr, int len)
        /* Add the address to the bind address list.
         * Use GFP_ATOMIC since BHs will be disabled.
         */
-       ret = sctp_add_bind_addr(bp, addr, SCTP_ADDR_SRC, GFP_ATOMIC);
+       ret = sctp_add_bind_addr(bp, addr, af->sockaddr_len,
+                                SCTP_ADDR_SRC, GFP_ATOMIC);

        /* Copy back into socket for getsockname() use. */
        if (!ret) {
@@ -576,7 +577,7 @@ static int sctp_send_asconf_add_ip(struct sock              *sk,
                        addr = addr_buf;
                        af = sctp_get_af_specific(addr->v4.sin_family);
                        memcpy(&saveaddr, addr, af->sockaddr_len);
-                       retval = sctp_add_bind_addr(bp, &saveaddr,
+                       retval = sctp_add_bind_addr(bp, &saveaddr, sizeof(saveaddr),
                                                    SCTP_ADDR_NEW, GFP_ATOMIC);
                        addr_buf += af->sockaddr_len;
                }
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] sctp: fix copying more bytes than expected in sctp_add_bind_addr

From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date: 2016-03-07 19:51:33 

On Mon, Mar 07, 2016 at 08:45:20PM +0100, Dmitry Vyukov wrote:
Yes, it is. I never saw this bug again. Forgot to update this thread. Sorry.
Cool, thanks. The patch isn't applied yet, so either some other patch
fixed it and this patch not necessary anymore or you kept the patch
applied.  Please confirm which one :-)

On Mon, Mar 7, 2016 at 8:44 PM, Marcelo Ricardo Leitner
[off-list ref] wrote:
quoted
On Tue, Jan 26, 2016 at 02:28:48PM +0100, Dmitry Vyukov wrote:
quoted
On Mon, Jan 25, 2016 at 6:52 PM, Marcelo Ricardo Leitner
[off-list ref] wrote:
quoted
Great. Dmitry, please give this a run. Local tests looked good but who
knows what syzkaller may find.
Now running with this patch.
Hi Dmitry, do you remember if this one succeeded?
quoted
quoted
Thanks

--8<--

Dmitry reported that sctp_add_bind_addr may read more bytes than
expected in case the parameter is a IPv4 addr supplied by the user
through calls such as sctp_bindx_add(), because it always copies
sizeof(union sctp_addr) while the buffer may be just a struct
sockaddr_in, which is smaller.

This patch then fixes it by limiting the memcpy to the min between the
union size and a (new parameter) provided addr size. Where possible this
parameter still is the size of that union, except for reading from
user-provided buffers, which then it accounts for protocol type.

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---
 include/net/sctp/structs.h |  2 +-
 net/sctp/bind_addr.c       | 14 ++++++++------
 net/sctp/protocol.c        |  1 +
 net/sctp/sm_make_chunk.c   |  2 +-
 net/sctp/socket.c          |  5 +++--
 5 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 20e72129be1ce0063eeafcbaadcee1f37e0c614c..97ba8a8c466f5c50bdc87ec578792e56553baa91 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -1099,7 +1099,7 @@ int sctp_bind_addr_dup(struct sctp_bind_addr *dest,
                        const struct sctp_bind_addr *src,
                        gfp_t gfp);
 int sctp_add_bind_addr(struct sctp_bind_addr *, union sctp_addr *,
-                      __u8 addr_state, gfp_t gfp);
+                      int new_size, __u8 addr_state, gfp_t gfp);
 int sctp_del_bind_addr(struct sctp_bind_addr *, union sctp_addr *);
 int sctp_bind_addr_match(struct sctp_bind_addr *, const union sctp_addr *,
                         struct sctp_sock *);
diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c
index 871cdf9567e6bc9c13cb1077dc6866a67e6e4367..80129d10a0af9c33e7348b79d010b9e5e948e584 100644
--- a/net/sctp/bind_addr.c
+++ b/net/sctp/bind_addr.c
@@ -111,7 +111,8 @@ int sctp_bind_addr_dup(struct sctp_bind_addr *dest,
        dest->port = src->port;

        list_for_each_entry(addr, &src->address_list, list) {
-               error = sctp_add_bind_addr(dest, &addr->a, 1, gfp);
+               error = sctp_add_bind_addr(dest, &addr->a, sizeof(addr->a),
+                                          1, gfp);
                if (error < 0)
                        break;
        }
@@ -150,7 +151,7 @@ void sctp_bind_addr_free(struct sctp_bind_addr *bp)

 /* Add an address to the bind address list in the SCTP_bind_addr structure. */
 int sctp_add_bind_addr(struct sctp_bind_addr *bp, union sctp_addr *new,
-                      __u8 addr_state, gfp_t gfp)
+                      int new_size, __u8 addr_state, gfp_t gfp)
 {
        struct sctp_sockaddr_entry *addr;

@@ -159,7 +160,7 @@ int sctp_add_bind_addr(struct sctp_bind_addr *bp, union sctp_addr *new,
        if (!addr)
                return -ENOMEM;

-       memcpy(&addr->a, new, sizeof(*new));
+       memcpy(&addr->a, new, min_t(size_t, sizeof(*new), new_size));

        /* Fix up the port if it has not yet been set.
         * Both v4 and v6 have the port at the same offset.
@@ -291,7 +292,8 @@ int sctp_raw_to_bind_addrs(struct sctp_bind_addr *bp, __u8 *raw_addr_list,
                }

                af->from_addr_param(&addr, rawaddr, htons(port), 0);
-               retval = sctp_add_bind_addr(bp, &addr, SCTP_ADDR_SRC, gfp);
+               retval = sctp_add_bind_addr(bp, &addr, sizeof(addr),
+                                           SCTP_ADDR_SRC, gfp);
                if (retval) {
                        /* Can't finish building the list, clean up. */
                        sctp_bind_addr_clean(bp);
@@ -453,8 +455,8 @@ static int sctp_copy_one_addr(struct net *net, struct sctp_bind_addr *dest,
                    (((AF_INET6 == addr->sa.sa_family) &&
                      (flags & SCTP_ADDR6_ALLOWED) &&
                      (flags & SCTP_ADDR6_PEERSUPP))))
-                       error = sctp_add_bind_addr(dest, addr, SCTP_ADDR_SRC,
-                                                   gfp);
+                       error = sctp_add_bind_addr(dest, addr, sizeof(addr),
+                                                  SCTP_ADDR_SRC, gfp);
        }

        return error;
diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index ab0d538a74ed593571cfaef02cd1bb7ce872abe6..2fb609008311f51344704d82f21b4de9f08253da 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -214,6 +214,7 @@ int sctp_copy_local_addr_list(struct net *net, struct sctp_bind_addr *bp,
                              (copy_flags & SCTP_ADDR6_ALLOWED) &&
                              (copy_flags & SCTP_ADDR6_PEERSUPP)))) {
                                error = sctp_add_bind_addr(bp, &addr->a,
+                                                   sizeof(addr->a),
                                                    SCTP_ADDR_SRC, GFP_ATOMIC);
                                if (error)
                                        goto end_copy;
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index 5d6a03fad3789a12290f5f14c5a7efa69c98f41a..1b91e9760fe514db6d89457e1d5da9e02800745e 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -1830,7 +1830,7 @@ no_hmac:
        /* Also, add the destination address. */
        if (list_empty(&retval->base.bind_addr.address_list)) {
                sctp_add_bind_addr(&retval->base.bind_addr, &chunk->dest,
-                               SCTP_ADDR_SRC, GFP_ATOMIC);
+                                  sizeof(chunk->dest), SCTP_ADDR_SRC, GFP_ATOMIC);
        }

        retval->next_tsn = retval->c.initial_tsn;
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 5ca2ebfe0be83882fcb841de6fa8029b6455ef85..213be3821a1d49e0c469c4ad4e9ff055a03205c5 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -386,7 +386,8 @@ static int sctp_do_bind(struct sock *sk, union sctp_addr *addr, int len)
        /* Add the address to the bind address list.
         * Use GFP_ATOMIC since BHs will be disabled.
         */
-       ret = sctp_add_bind_addr(bp, addr, SCTP_ADDR_SRC, GFP_ATOMIC);
+       ret = sctp_add_bind_addr(bp, addr, af->sockaddr_len,
+                                SCTP_ADDR_SRC, GFP_ATOMIC);

        /* Copy back into socket for getsockname() use. */
        if (!ret) {
@@ -576,7 +577,7 @@ static int sctp_send_asconf_add_ip(struct sock              *sk,
                        addr = addr_buf;
                        af = sctp_get_af_specific(addr->v4.sin_family);
                        memcpy(&saveaddr, addr, af->sockaddr_len);
-                       retval = sctp_add_bind_addr(bp, &saveaddr,
+                       retval = sctp_add_bind_addr(bp, &saveaddr, sizeof(saveaddr),
                                                    SCTP_ADDR_NEW, GFP_ATOMIC);
                        addr_buf += af->sockaddr_len;
                }
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] sctp: fix copying more bytes than expected in sctp_add_bind_addr

From: Dmitry Vyukov <dvyukov@google.com>
Date: 2016-03-07 19:56:39 

I kept the patch applied.

On Mon, Mar 7, 2016 at 8:51 PM, Marcelo Ricardo Leitner
[off-list ref] wrote:
On Mon, Mar 07, 2016 at 08:45:20PM +0100, Dmitry Vyukov wrote:
quoted
Yes, it is. I never saw this bug again. Forgot to update this thread. Sorry.
Cool, thanks. The patch isn't applied yet, so either some other patch
fixed it and this patch not necessary anymore or you kept the patch
applied.  Please confirm which one :-)
quoted
On Mon, Mar 7, 2016 at 8:44 PM, Marcelo Ricardo Leitner
[off-list ref] wrote:
quoted
On Tue, Jan 26, 2016 at 02:28:48PM +0100, Dmitry Vyukov wrote:
quoted
On Mon, Jan 25, 2016 at 6:52 PM, Marcelo Ricardo Leitner
[off-list ref] wrote:
quoted
Great. Dmitry, please give this a run. Local tests looked good but who
knows what syzkaller may find.
Now running with this patch.
Hi Dmitry, do you remember if this one succeeded?
quoted
quoted
Thanks

--8<--

Dmitry reported that sctp_add_bind_addr may read more bytes than
expected in case the parameter is a IPv4 addr supplied by the user
through calls such as sctp_bindx_add(), because it always copies
sizeof(union sctp_addr) while the buffer may be just a struct
sockaddr_in, which is smaller.

This patch then fixes it by limiting the memcpy to the min between the
union size and a (new parameter) provided addr size. Where possible this
parameter still is the size of that union, except for reading from
user-provided buffers, which then it accounts for protocol type.

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---
 include/net/sctp/structs.h |  2 +-
 net/sctp/bind_addr.c       | 14 ++++++++------
 net/sctp/protocol.c        |  1 +
 net/sctp/sm_make_chunk.c   |  2 +-
 net/sctp/socket.c          |  5 +++--
 5 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 20e72129be1ce0063eeafcbaadcee1f37e0c614c..97ba8a8c466f5c50bdc87ec578792e56553baa91 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -1099,7 +1099,7 @@ int sctp_bind_addr_dup(struct sctp_bind_addr *dest,
                        const struct sctp_bind_addr *src,
                        gfp_t gfp);
 int sctp_add_bind_addr(struct sctp_bind_addr *, union sctp_addr *,
-                      __u8 addr_state, gfp_t gfp);
+                      int new_size, __u8 addr_state, gfp_t gfp);
 int sctp_del_bind_addr(struct sctp_bind_addr *, union sctp_addr *);
 int sctp_bind_addr_match(struct sctp_bind_addr *, const union sctp_addr *,
                         struct sctp_sock *);
diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c
index 871cdf9567e6bc9c13cb1077dc6866a67e6e4367..80129d10a0af9c33e7348b79d010b9e5e948e584 100644
--- a/net/sctp/bind_addr.c
+++ b/net/sctp/bind_addr.c
@@ -111,7 +111,8 @@ int sctp_bind_addr_dup(struct sctp_bind_addr *dest,
        dest->port = src->port;

        list_for_each_entry(addr, &src->address_list, list) {
-               error = sctp_add_bind_addr(dest, &addr->a, 1, gfp);
+               error = sctp_add_bind_addr(dest, &addr->a, sizeof(addr->a),
+                                          1, gfp);
                if (error < 0)
                        break;
        }
@@ -150,7 +151,7 @@ void sctp_bind_addr_free(struct sctp_bind_addr *bp)

 /* Add an address to the bind address list in the SCTP_bind_addr structure. */
 int sctp_add_bind_addr(struct sctp_bind_addr *bp, union sctp_addr *new,
-                      __u8 addr_state, gfp_t gfp)
+                      int new_size, __u8 addr_state, gfp_t gfp)
 {
        struct sctp_sockaddr_entry *addr;

@@ -159,7 +160,7 @@ int sctp_add_bind_addr(struct sctp_bind_addr *bp, union sctp_addr *new,
        if (!addr)
                return -ENOMEM;

-       memcpy(&addr->a, new, sizeof(*new));
+       memcpy(&addr->a, new, min_t(size_t, sizeof(*new), new_size));

        /* Fix up the port if it has not yet been set.
         * Both v4 and v6 have the port at the same offset.
@@ -291,7 +292,8 @@ int sctp_raw_to_bind_addrs(struct sctp_bind_addr *bp, __u8 *raw_addr_list,
                }

                af->from_addr_param(&addr, rawaddr, htons(port), 0);
-               retval = sctp_add_bind_addr(bp, &addr, SCTP_ADDR_SRC, gfp);
+               retval = sctp_add_bind_addr(bp, &addr, sizeof(addr),
+                                           SCTP_ADDR_SRC, gfp);
                if (retval) {
                        /* Can't finish building the list, clean up. */
                        sctp_bind_addr_clean(bp);
@@ -453,8 +455,8 @@ static int sctp_copy_one_addr(struct net *net, struct sctp_bind_addr *dest,
                    (((AF_INET6 == addr->sa.sa_family) &&
                      (flags & SCTP_ADDR6_ALLOWED) &&
                      (flags & SCTP_ADDR6_PEERSUPP))))
-                       error = sctp_add_bind_addr(dest, addr, SCTP_ADDR_SRC,
-                                                   gfp);
+                       error = sctp_add_bind_addr(dest, addr, sizeof(addr),
+                                                  SCTP_ADDR_SRC, gfp);
        }

        return error;
diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index ab0d538a74ed593571cfaef02cd1bb7ce872abe6..2fb609008311f51344704d82f21b4de9f08253da 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -214,6 +214,7 @@ int sctp_copy_local_addr_list(struct net *net, struct sctp_bind_addr *bp,
                              (copy_flags & SCTP_ADDR6_ALLOWED) &&
                              (copy_flags & SCTP_ADDR6_PEERSUPP)))) {
                                error = sctp_add_bind_addr(bp, &addr->a,
+                                                   sizeof(addr->a),
                                                    SCTP_ADDR_SRC, GFP_ATOMIC);
                                if (error)
                                        goto end_copy;
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index 5d6a03fad3789a12290f5f14c5a7efa69c98f41a..1b91e9760fe514db6d89457e1d5da9e02800745e 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -1830,7 +1830,7 @@ no_hmac:
        /* Also, add the destination address. */
        if (list_empty(&retval->base.bind_addr.address_list)) {
                sctp_add_bind_addr(&retval->base.bind_addr, &chunk->dest,
-                               SCTP_ADDR_SRC, GFP_ATOMIC);
+                                  sizeof(chunk->dest), SCTP_ADDR_SRC, GFP_ATOMIC);
        }

        retval->next_tsn = retval->c.initial_tsn;
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 5ca2ebfe0be83882fcb841de6fa8029b6455ef85..213be3821a1d49e0c469c4ad4e9ff055a03205c5 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -386,7 +386,8 @@ static int sctp_do_bind(struct sock *sk, union sctp_addr *addr, int len)
        /* Add the address to the bind address list.
         * Use GFP_ATOMIC since BHs will be disabled.
         */
-       ret = sctp_add_bind_addr(bp, addr, SCTP_ADDR_SRC, GFP_ATOMIC);
+       ret = sctp_add_bind_addr(bp, addr, af->sockaddr_len,
+                                SCTP_ADDR_SRC, GFP_ATOMIC);

        /* Copy back into socket for getsockname() use. */
        if (!ret) {
@@ -576,7 +577,7 @@ static int sctp_send_asconf_add_ip(struct sock              *sk,
                        addr = addr_buf;
                        af = sctp_get_af_specific(addr->v4.sin_family);
                        memcpy(&saveaddr, addr, af->sockaddr_len);
-                       retval = sctp_add_bind_addr(bp, &saveaddr,
+                       retval = sctp_add_bind_addr(bp, &saveaddr, sizeof(saveaddr),
                                                    SCTP_ADDR_NEW, GFP_ATOMIC);
                        addr_buf += af->sockaddr_len;
                }
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] sctp: fix copying more bytes than expected in sctp_add_bind_addr

From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date: 2016-03-07 20:00:18 

On Mon, Mar 07, 2016 at 08:56:08PM +0100, Dmitry Vyukov wrote:
I kept the patch applied.
Then Dave, please consider applying this patch.

Thanks.

On Mon, Mar 7, 2016 at 8:51 PM, Marcelo Ricardo Leitner
[off-list ref] wrote:
quoted
On Mon, Mar 07, 2016 at 08:45:20PM +0100, Dmitry Vyukov wrote:
quoted
Yes, it is. I never saw this bug again. Forgot to update this thread. Sorry.
Cool, thanks. The patch isn't applied yet, so either some other patch
fixed it and this patch not necessary anymore or you kept the patch
applied.  Please confirm which one :-)
quoted
On Mon, Mar 7, 2016 at 8:44 PM, Marcelo Ricardo Leitner
[off-list ref] wrote:
quoted
On Tue, Jan 26, 2016 at 02:28:48PM +0100, Dmitry Vyukov wrote:
quoted
On Mon, Jan 25, 2016 at 6:52 PM, Marcelo Ricardo Leitner
[off-list ref] wrote:
quoted
Great. Dmitry, please give this a run. Local tests looked good but who
knows what syzkaller may find.
Now running with this patch.
Hi Dmitry, do you remember if this one succeeded?
quoted
quoted
Thanks

--8<--

Dmitry reported that sctp_add_bind_addr may read more bytes than
expected in case the parameter is a IPv4 addr supplied by the user
through calls such as sctp_bindx_add(), because it always copies
sizeof(union sctp_addr) while the buffer may be just a struct
sockaddr_in, which is smaller.

This patch then fixes it by limiting the memcpy to the min between the
union size and a (new parameter) provided addr size. Where possible this
parameter still is the size of that union, except for reading from
user-provided buffers, which then it accounts for protocol type.

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---
 include/net/sctp/structs.h |  2 +-
 net/sctp/bind_addr.c       | 14 ++++++++------
 net/sctp/protocol.c        |  1 +
 net/sctp/sm_make_chunk.c   |  2 +-
 net/sctp/socket.c          |  5 +++--
 5 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 20e72129be1ce0063eeafcbaadcee1f37e0c614c..97ba8a8c466f5c50bdc87ec578792e56553baa91 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -1099,7 +1099,7 @@ int sctp_bind_addr_dup(struct sctp_bind_addr *dest,
                        const struct sctp_bind_addr *src,
                        gfp_t gfp);
 int sctp_add_bind_addr(struct sctp_bind_addr *, union sctp_addr *,
-                      __u8 addr_state, gfp_t gfp);
+                      int new_size, __u8 addr_state, gfp_t gfp);
 int sctp_del_bind_addr(struct sctp_bind_addr *, union sctp_addr *);
 int sctp_bind_addr_match(struct sctp_bind_addr *, const union sctp_addr *,
                         struct sctp_sock *);
diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c
index 871cdf9567e6bc9c13cb1077dc6866a67e6e4367..80129d10a0af9c33e7348b79d010b9e5e948e584 100644
--- a/net/sctp/bind_addr.c
+++ b/net/sctp/bind_addr.c
@@ -111,7 +111,8 @@ int sctp_bind_addr_dup(struct sctp_bind_addr *dest,
        dest->port = src->port;

        list_for_each_entry(addr, &src->address_list, list) {
-               error = sctp_add_bind_addr(dest, &addr->a, 1, gfp);
+               error = sctp_add_bind_addr(dest, &addr->a, sizeof(addr->a),
+                                          1, gfp);
                if (error < 0)
                        break;
        }
@@ -150,7 +151,7 @@ void sctp_bind_addr_free(struct sctp_bind_addr *bp)

 /* Add an address to the bind address list in the SCTP_bind_addr structure. */
 int sctp_add_bind_addr(struct sctp_bind_addr *bp, union sctp_addr *new,
-                      __u8 addr_state, gfp_t gfp)
+                      int new_size, __u8 addr_state, gfp_t gfp)
 {
        struct sctp_sockaddr_entry *addr;

@@ -159,7 +160,7 @@ int sctp_add_bind_addr(struct sctp_bind_addr *bp, union sctp_addr *new,
        if (!addr)
                return -ENOMEM;

-       memcpy(&addr->a, new, sizeof(*new));
+       memcpy(&addr->a, new, min_t(size_t, sizeof(*new), new_size));

        /* Fix up the port if it has not yet been set.
         * Both v4 and v6 have the port at the same offset.
@@ -291,7 +292,8 @@ int sctp_raw_to_bind_addrs(struct sctp_bind_addr *bp, __u8 *raw_addr_list,
                }

                af->from_addr_param(&addr, rawaddr, htons(port), 0);
-               retval = sctp_add_bind_addr(bp, &addr, SCTP_ADDR_SRC, gfp);
+               retval = sctp_add_bind_addr(bp, &addr, sizeof(addr),
+                                           SCTP_ADDR_SRC, gfp);
                if (retval) {
                        /* Can't finish building the list, clean up. */
                        sctp_bind_addr_clean(bp);
@@ -453,8 +455,8 @@ static int sctp_copy_one_addr(struct net *net, struct sctp_bind_addr *dest,
                    (((AF_INET6 == addr->sa.sa_family) &&
                      (flags & SCTP_ADDR6_ALLOWED) &&
                      (flags & SCTP_ADDR6_PEERSUPP))))
-                       error = sctp_add_bind_addr(dest, addr, SCTP_ADDR_SRC,
-                                                   gfp);
+                       error = sctp_add_bind_addr(dest, addr, sizeof(addr),
+                                                  SCTP_ADDR_SRC, gfp);
        }

        return error;
diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index ab0d538a74ed593571cfaef02cd1bb7ce872abe6..2fb609008311f51344704d82f21b4de9f08253da 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -214,6 +214,7 @@ int sctp_copy_local_addr_list(struct net *net, struct sctp_bind_addr *bp,
                              (copy_flags & SCTP_ADDR6_ALLOWED) &&
                              (copy_flags & SCTP_ADDR6_PEERSUPP)))) {
                                error = sctp_add_bind_addr(bp, &addr->a,
+                                                   sizeof(addr->a),
                                                    SCTP_ADDR_SRC, GFP_ATOMIC);
                                if (error)
                                        goto end_copy;
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index 5d6a03fad3789a12290f5f14c5a7efa69c98f41a..1b91e9760fe514db6d89457e1d5da9e02800745e 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -1830,7 +1830,7 @@ no_hmac:
        /* Also, add the destination address. */
        if (list_empty(&retval->base.bind_addr.address_list)) {
                sctp_add_bind_addr(&retval->base.bind_addr, &chunk->dest,
-                               SCTP_ADDR_SRC, GFP_ATOMIC);
+                                  sizeof(chunk->dest), SCTP_ADDR_SRC, GFP_ATOMIC);
        }

        retval->next_tsn = retval->c.initial_tsn;
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 5ca2ebfe0be83882fcb841de6fa8029b6455ef85..213be3821a1d49e0c469c4ad4e9ff055a03205c5 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -386,7 +386,8 @@ static int sctp_do_bind(struct sock *sk, union sctp_addr *addr, int len)
        /* Add the address to the bind address list.
         * Use GFP_ATOMIC since BHs will be disabled.
         */
-       ret = sctp_add_bind_addr(bp, addr, SCTP_ADDR_SRC, GFP_ATOMIC);
+       ret = sctp_add_bind_addr(bp, addr, af->sockaddr_len,
+                                SCTP_ADDR_SRC, GFP_ATOMIC);

        /* Copy back into socket for getsockname() use. */
        if (!ret) {
@@ -576,7 +577,7 @@ static int sctp_send_asconf_add_ip(struct sock              *sk,
                        addr = addr_buf;
                        af = sctp_get_af_specific(addr->v4.sin_family);
                        memcpy(&saveaddr, addr, af->sockaddr_len);
-                       retval = sctp_add_bind_addr(bp, &saveaddr,
+                       retval = sctp_add_bind_addr(bp, &saveaddr, sizeof(saveaddr),
                                                    SCTP_ADDR_NEW, GFP_ATOMIC);
                        addr_buf += af->sockaddr_len;
                }
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] sctp: fix copying more bytes than expected in sctp_add_bind_addr

From: David Miller <davem@davemloft.net>
Date: 2016-03-07 20:21:35 

From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date: Mon, 7 Mar 2016 17:00:08 -0300

On Mon, Mar 07, 2016 at 08:56:08PM +0100, Dmitry Vyukov wrote:
quoted
I kept the patch applied.
Then Dave, please consider applying this patch.
Please submit the patch properly, as a fresh mailing list posting, and
integrating Tested-by: et al. tags as necessary.

Thanks.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help