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

2016-01-25 Thread Neil Horman 
On Mon, Jan 25, 2016 at 02:16:00PM -0200, Marcelo Ricardo Leitner wrote:
> 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, >address_list, list) {
> - error = sctp_add_bind_addr(dest, >a, 1, gfp);
> + error = sctp_add_bind_addr(dest, >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(>a, new, sizeof(*new));
> + memcpy(>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(, rawaddr, htons(port), 0);
> - retval = sctp_add_bind_addr(bp, , SCTP_ADDR_SRC, gfp);
> + retval = sctp_add_bind_addr(bp, , 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, >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. */
>

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

2016-01-25 Thread Marcelo Ricardo Leitner 
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, >address_list, list) {
-   error = sctp_add_bind_addr(dest, >a, 1, gfp);
+   error = sctp_add_bind_addr(dest, >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(>a, new, sizeof(*new));
+   memcpy(>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(, rawaddr, htons(port), 0);
-   retval = sctp_add_bind_addr(bp, , SCTP_ADDR_SRC, gfp);
+   retval = sctp_add_bind_addr(bp, , 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, >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(>base.bind_addr.address_list)) {
sctp_add_bind_addr(>base.bind_addr, >dest,
-   SCTP_ADDR_SRC, GFP_ATOMIC);
+

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

2016-01-25 Thread Neil Horman 
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:
> > On Mon, Jan 25, 2016 at 3:31 PM, Neil Horman  wrote:
> > > 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 
> > >> 88006c6361e8
> > >> 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 0xea0001b18d80 objects=16 used=4 fp=0x88006c6376e0
> > >> flags=0x5fffc004080
> > >> INFO: Object 0x88006c6361e8 @offset=488 fp=0x0002
> > >> Bytes b4 88006c6361d8: 00 00 00 00 00 00 00 00 2f 98 34 88 ff ff
> > >> ff ff  /.4.
> > >> Object 88006c6361e8: 02 00 00 00 00 00 00 00 02 00 ab 07 7f 00 00
> > >> 01  
> > >> CPU: 2 PID: 12551 Comm: syz-executor Tainted: GB   
> > >> 4.5.0-rc1+ #278
> > >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 
> > >> 01/01/2011
> > >>   880036397928 8299a02d 88003e807900
> > >>  88006c6361e8 88006c636000 880036397958 81752814
> > >>  88003e807900 ea0001b18d80 88006c6361e8 88006c6361e8
> > >>
> > >> Call Trace:
> > >>  [] __asan_loadN+0x124/0x1a0 mm/kasan/kasan.c:512
> > >>  [] memcpy+0x1d/0x40 mm/kasan/kasan.c:297
> > >>  [] sctp_add_bind_addr+0xa9/0x270 
> > >> net/sctp/bind_addr.c:162
> > >>  [] sctp_do_bind+0x336/0x580 net/sctp/socket.c:389
> > >>  [] sctp_bindx_add+0xac/0x1a0 net/sctp/socket.c:471
> > >>  [] sctp_setsockopt_bindx+0x2f8/0x3e0 
> > >> net/sctp/socket.c:1010
> > >>  [] sctp_setsockopt+0x1493/0x3630 
> > >> net/sctp/socket.c:3711
> > >>  [] sock_common_setsockopt+0x97/0xd0 
> > >> net/core/sock.c:2620
> > >>  [< inline >] SYSC_setsockopt net/socket.c:1752
> > >>  [] SyS_setsockopt+0x15b/0x250 net/socket.c:1731
> > >>  [] entry_SYSCALL_64_fastpath+0x16/0x7a
> > >> arch/x86/entry/entry_64.S:185
> > >>
> > >> Memory state around the buggy address:
> > >>  88006c636080: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> > >>  88006c636100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> > >> >88006c636180: fc fc fc fc fc fc fc fc fc fc fc fc fc 00 00 fc
> > >> ^
> > >>  88006c636200: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> > >>  88006c636280: 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) {

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

2016-01-25 Thread Marcelo Ricardo Leitner 
On Mon, Jan 25, 2016 at 03:42:14PM +0100, Dmitry Vyukov wrote:
> On Mon, Jan 25, 2016 at 3:31 PM, Neil Horman  wrote:
> > 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 88006c6361e8
> >> 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 0xea0001b18d80 objects=16 used=4 fp=0x88006c6376e0
> >> flags=0x5fffc004080
> >> INFO: Object 0x88006c6361e8 @offset=488 fp=0x0002
> >> Bytes b4 88006c6361d8: 00 00 00 00 00 00 00 00 2f 98 34 88 ff ff
> >> ff ff  /.4.
> >> Object 88006c6361e8: 02 00 00 00 00 00 00 00 02 00 ab 07 7f 00 00
> >> 01  
> >> CPU: 2 PID: 12551 Comm: syz-executor Tainted: GB   4.5.0-rc1+ 
> >> #278
> >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 
> >> 01/01/2011
> >>   880036397928 8299a02d 88003e807900
> >>  88006c6361e8 88006c636000 880036397958 81752814
> >>  88003e807900 ea0001b18d80 88006c6361e8 88006c6361e8
> >>
> >> Call Trace:
> >>  [] __asan_loadN+0x124/0x1a0 mm/kasan/kasan.c:512
> >>  [] memcpy+0x1d/0x40 mm/kasan/kasan.c:297
> >>  [] sctp_add_bind_addr+0xa9/0x270 
> >> net/sctp/bind_addr.c:162
> >>  [] sctp_do_bind+0x336/0x580 net/sctp/socket.c:389
> >>  [] sctp_bindx_add+0xac/0x1a0 net/sctp/socket.c:471
> >>  [] sctp_setsockopt_bindx+0x2f8/0x3e0 
> >> net/sctp/socket.c:1010
> >>  [] sctp_setsockopt+0x1493/0x3630 net/sctp/socket.c:3711
> >>  [] sock_common_setsockopt+0x97/0xd0 net/core/sock.c:2620
> >>  [< inline >] SYSC_setsockopt net/socket.c:1752
> >>  [] SyS_setsockopt+0x15b/0x250 net/socket.c:1731
> >>  [] entry_SYSCALL_64_fastpath+0x16/0x7a
> >> arch/x86/entry/entry_64.S:185
> >>
> >> Memory state around the buggy address:
> >>  88006c636080: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> >>  88006c636100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> >> >88006c636180: fc fc fc fc fc fc fc fc fc fc fc fc fc 00 00 fc
> >> ^
> >>  88006c636200: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> >>  88006c636280: 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(>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

2016-01-25 Thread Dmitry Vyukov 
On Mon, Jan 25, 2016 at 3:31 PM, Neil Horman  wrote:
> 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 88006c6361e8
>> 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 0xea0001b18d80 objects=16 used=4 fp=0x88006c6376e0
>> flags=0x5fffc004080
>> INFO: Object 0x88006c6361e8 @offset=488 fp=0x0002
>> Bytes b4 88006c6361d8: 00 00 00 00 00 00 00 00 2f 98 34 88 ff ff
>> ff ff  /.4.
>> Object 88006c6361e8: 02 00 00 00 00 00 00 00 02 00 ab 07 7f 00 00
>> 01  
>> CPU: 2 PID: 12551 Comm: syz-executor Tainted: GB   4.5.0-rc1+ 
>> #278
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>>   880036397928 8299a02d 88003e807900
>>  88006c6361e8 88006c636000 880036397958 81752814
>>  88003e807900 ea0001b18d80 88006c6361e8 88006c6361e8
>>
>> Call Trace:
>>  [] __asan_loadN+0x124/0x1a0 mm/kasan/kasan.c:512
>>  [] memcpy+0x1d/0x40 mm/kasan/kasan.c:297
>>  [] sctp_add_bind_addr+0xa9/0x270 net/sctp/bind_addr.c:162
>>  [] sctp_do_bind+0x336/0x580 net/sctp/socket.c:389
>>  [] sctp_bindx_add+0xac/0x1a0 net/sctp/socket.c:471
>>  [] sctp_setsockopt_bindx+0x2f8/0x3e0 
>> net/sctp/socket.c:1010
>>  [] sctp_setsockopt+0x1493/0x3630 net/sctp/socket.c:3711
>>  [] sock_common_setsockopt+0x97/0xd0 net/core/sock.c:2620
>>  [< inline >] SYSC_setsockopt net/socket.c:1752
>>  [] SyS_setsockopt+0x15b/0x250 net/socket.c:1731
>>  [] entry_SYSCALL_64_fastpath+0x16/0x7a
>> arch/x86/entry/entry_64.S:185
>>
>> Memory state around the buggy address:
>>  88006c636080: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>>  88006c636100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>> >88006c636180: fc fc fc fc fc fc fc fc fc fc fc fc fc 00 00 fc
>> ^
>>  88006c636200: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>>  88006c636280: 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(>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

2016-01-25 Thread Neil Horman 
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 88006c6361e8
> 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 0xea0001b18d80 objects=16 used=4 fp=0x88006c6376e0
> flags=0x5fffc004080
> INFO: Object 0x88006c6361e8 @offset=488 fp=0x0002
> Bytes b4 88006c6361d8: 00 00 00 00 00 00 00 00 2f 98 34 88 ff ff
> ff ff  /.4.
> Object 88006c6361e8: 02 00 00 00 00 00 00 00 02 00 ab 07 7f 00 00
> 01  
> CPU: 2 PID: 12551 Comm: syz-executor Tainted: GB   4.5.0-rc1+ #278
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>   880036397928 8299a02d 88003e807900
>  88006c6361e8 88006c636000 880036397958 81752814
>  88003e807900 ea0001b18d80 88006c6361e8 88006c6361e8
> 
> Call Trace:
>  [] __asan_loadN+0x124/0x1a0 mm/kasan/kasan.c:512
>  [] memcpy+0x1d/0x40 mm/kasan/kasan.c:297
>  [] sctp_add_bind_addr+0xa9/0x270 net/sctp/bind_addr.c:162
>  [] sctp_do_bind+0x336/0x580 net/sctp/socket.c:389
>  [] sctp_bindx_add+0xac/0x1a0 net/sctp/socket.c:471
>  [] sctp_setsockopt_bindx+0x2f8/0x3e0 net/sctp/socket.c:1010
>  [] sctp_setsockopt+0x1493/0x3630 net/sctp/socket.c:3711
>  [] sock_common_setsockopt+0x97/0xd0 net/core/sock.c:2620
>  [< inline >] SYSC_setsockopt net/socket.c:1752
>  [] SyS_setsockopt+0x15b/0x250 net/socket.c:1731
>  [] entry_SYSCALL_64_fastpath+0x16/0x7a
> arch/x86/entry/entry_64.S:185
> 
> Memory state around the buggy address:
>  88006c636080: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>  88006c636100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> >88006c636180: fc fc fc fc fc fc fc fc fc fc fc fc fc 00 00 fc
> ^
>  88006c636200: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>  88006c636280: 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 majord...@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

2016-01-25 Thread Neil Horman 
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 88006c6361e8
> 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 0xea0001b18d80 objects=16 used=4 fp=0x88006c6376e0
> flags=0x5fffc004080
> INFO: Object 0x88006c6361e8 @offset=488 fp=0x0002
> Bytes b4 88006c6361d8: 00 00 00 00 00 00 00 00 2f 98 34 88 ff ff
> ff ff  /.4.
> Object 88006c6361e8: 02 00 00 00 00 00 00 00 02 00 ab 07 7f 00 00
> 01  
> CPU: 2 PID: 12551 Comm: syz-executor Tainted: GB   4.5.0-rc1+ #278
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>   880036397928 8299a02d 88003e807900
>  88006c6361e8 88006c636000 880036397958 81752814
>  88003e807900 ea0001b18d80 88006c6361e8 88006c6361e8
> 
> Call Trace:
>  [] __asan_loadN+0x124/0x1a0 mm/kasan/kasan.c:512
>  [] memcpy+0x1d/0x40 mm/kasan/kasan.c:297
>  [] sctp_add_bind_addr+0xa9/0x270 net/sctp/bind_addr.c:162
>  [] sctp_do_bind+0x336/0x580 net/sctp/socket.c:389
>  [] sctp_bindx_add+0xac/0x1a0 net/sctp/socket.c:471
>  [] sctp_setsockopt_bindx+0x2f8/0x3e0 net/sctp/socket.c:1010
>  [] sctp_setsockopt+0x1493/0x3630 net/sctp/socket.c:3711
>  [] sock_common_setsockopt+0x97/0xd0 net/core/sock.c:2620
>  [< inline >] SYSC_setsockopt net/socket.c:1752
>  [] SyS_setsockopt+0x15b/0x250 net/socket.c:1731
>  [] entry_SYSCALL_64_fastpath+0x16/0x7a
> arch/x86/entry/entry_64.S:185
> 
> Memory state around the buggy address:
>  88006c636080: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>  88006c636100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> >88006c636180: fc fc fc fc fc fc fc fc fc fc fc fc fc 00 00 fc
> ^
>  88006c636200: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>  88006c636280: 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 majord...@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

2016-01-25 Thread Marcelo Ricardo Leitner 
On Mon, Jan 25, 2016 at 03:42:14PM +0100, Dmitry Vyukov wrote:
> On Mon, Jan 25, 2016 at 3:31 PM, Neil Horman  wrote:
> > 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 88006c6361e8
> >> 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 0xea0001b18d80 objects=16 used=4 fp=0x88006c6376e0
> >> flags=0x5fffc004080
> >> INFO: Object 0x88006c6361e8 @offset=488 fp=0x0002
> >> Bytes b4 88006c6361d8: 00 00 00 00 00 00 00 00 2f 98 34 88 ff ff
> >> ff ff  /.4.
> >> Object 88006c6361e8: 02 00 00 00 00 00 00 00 02 00 ab 07 7f 00 00
> >> 01  
> >> CPU: 2 PID: 12551 Comm: syz-executor Tainted: GB   4.5.0-rc1+ 
> >> #278
> >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 
> >> 01/01/2011
> >>   880036397928 8299a02d 88003e807900
> >>  88006c6361e8 88006c636000 880036397958 81752814
> >>  88003e807900 ea0001b18d80 88006c6361e8 88006c6361e8
> >>
> >> Call Trace:
> >>  [] __asan_loadN+0x124/0x1a0 mm/kasan/kasan.c:512
> >>  [] memcpy+0x1d/0x40 mm/kasan/kasan.c:297
> >>  [] sctp_add_bind_addr+0xa9/0x270 
> >> net/sctp/bind_addr.c:162
> >>  [] sctp_do_bind+0x336/0x580 net/sctp/socket.c:389
> >>  [] sctp_bindx_add+0xac/0x1a0 net/sctp/socket.c:471
> >>  [] sctp_setsockopt_bindx+0x2f8/0x3e0 
> >> net/sctp/socket.c:1010
> >>  [] sctp_setsockopt+0x1493/0x3630 net/sctp/socket.c:3711
> >>  [] sock_common_setsockopt+0x97/0xd0 net/core/sock.c:2620
> >>  [< inline >] SYSC_setsockopt net/socket.c:1752
> >>  [] SyS_setsockopt+0x15b/0x250 net/socket.c:1731
> >>  [] entry_SYSCALL_64_fastpath+0x16/0x7a
> >> arch/x86/entry/entry_64.S:185
> >>
> >> Memory state around the buggy address:
> >>  88006c636080: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> >>  88006c636100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> >> >88006c636180: fc fc fc fc fc fc fc fc fc fc fc fc fc 00 00 fc
> >> ^
> >>  88006c636200: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> >>  88006c636280: 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(>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

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

2016-01-25 Thread Dmitry Vyukov 
On Mon, Jan 25, 2016 at 3:31 PM, Neil Horman  wrote:
> 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 88006c6361e8
>> 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 0xea0001b18d80 objects=16 used=4 fp=0x88006c6376e0
>> flags=0x5fffc004080
>> INFO: Object 0x88006c6361e8 @offset=488 fp=0x0002
>> Bytes b4 88006c6361d8: 00 00 00 00 00 00 00 00 2f 98 34 88 ff ff
>> ff ff  /.4.
>> Object 88006c6361e8: 02 00 00 00 00 00 00 00 02 00 ab 07 7f 00 00
>> 01  
>> CPU: 2 PID: 12551 Comm: syz-executor Tainted: GB   4.5.0-rc1+ 
>> #278
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>>   880036397928 8299a02d 88003e807900
>>  88006c6361e8 88006c636000 880036397958 81752814
>>  88003e807900 ea0001b18d80 88006c6361e8 88006c6361e8
>>
>> Call Trace:
>>  [] __asan_loadN+0x124/0x1a0 mm/kasan/kasan.c:512
>>  [] memcpy+0x1d/0x40 mm/kasan/kasan.c:297
>>  [] sctp_add_bind_addr+0xa9/0x270 net/sctp/bind_addr.c:162
>>  [] sctp_do_bind+0x336/0x580 net/sctp/socket.c:389
>>  [] sctp_bindx_add+0xac/0x1a0 net/sctp/socket.c:471
>>  [] sctp_setsockopt_bindx+0x2f8/0x3e0 
>> net/sctp/socket.c:1010
>>  [] sctp_setsockopt+0x1493/0x3630 net/sctp/socket.c:3711
>>  [] sock_common_setsockopt+0x97/0xd0 net/core/sock.c:2620
>>  [< inline >] SYSC_setsockopt net/socket.c:1752
>>  [] SyS_setsockopt+0x15b/0x250 net/socket.c:1731
>>  [] entry_SYSCALL_64_fastpath+0x16/0x7a
>> arch/x86/entry/entry_64.S:185
>>
>> Memory state around the buggy address:
>>  88006c636080: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>>  88006c636100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>> >88006c636180: fc fc fc fc fc fc fc fc fc fc fc fc fc 00 00 fc
>> ^
>>  88006c636200: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>>  88006c636280: 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(>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

2016-01-25 Thread Marcelo Ricardo Leitner 
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, >address_list, list) {
-   error = sctp_add_bind_addr(dest, >a, 1, gfp);
+   error = sctp_add_bind_addr(dest, >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(>a, new, sizeof(*new));
+   memcpy(>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(, rawaddr, htons(port), 0);
-   retval = sctp_add_bind_addr(bp, , SCTP_ADDR_SRC, gfp);
+   retval = sctp_add_bind_addr(bp, , 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, >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(>base.bind_addr.address_list)) {
sctp_add_bind_addr(>base.bind_addr, >dest,
-   SCTP_ADDR_SRC, GFP_ATOMIC);
+

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

2016-01-25 Thread Neil Horman 
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:
> > On Mon, Jan 25, 2016 at 3:31 PM, Neil Horman  wrote:
> > > 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 
> > >> 88006c6361e8
> > >> 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 0xea0001b18d80 objects=16 used=4 fp=0x88006c6376e0
> > >> flags=0x5fffc004080
> > >> INFO: Object 0x88006c6361e8 @offset=488 fp=0x0002
> > >> Bytes b4 88006c6361d8: 00 00 00 00 00 00 00 00 2f 98 34 88 ff ff
> > >> ff ff  /.4.
> > >> Object 88006c6361e8: 02 00 00 00 00 00 00 00 02 00 ab 07 7f 00 00
> > >> 01  
> > >> CPU: 2 PID: 12551 Comm: syz-executor Tainted: GB   
> > >> 4.5.0-rc1+ #278
> > >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 
> > >> 01/01/2011
> > >>   880036397928 8299a02d 88003e807900
> > >>  88006c6361e8 88006c636000 880036397958 81752814
> > >>  88003e807900 ea0001b18d80 88006c6361e8 88006c6361e8
> > >>
> > >> Call Trace:
> > >>  [] __asan_loadN+0x124/0x1a0 mm/kasan/kasan.c:512
> > >>  [] memcpy+0x1d/0x40 mm/kasan/kasan.c:297
> > >>  [] sctp_add_bind_addr+0xa9/0x270 
> > >> net/sctp/bind_addr.c:162
> > >>  [] sctp_do_bind+0x336/0x580 net/sctp/socket.c:389
> > >>  [] sctp_bindx_add+0xac/0x1a0 net/sctp/socket.c:471
> > >>  [] sctp_setsockopt_bindx+0x2f8/0x3e0 
> > >> net/sctp/socket.c:1010
> > >>  [] sctp_setsockopt+0x1493/0x3630 
> > >> net/sctp/socket.c:3711
> > >>  [] sock_common_setsockopt+0x97/0xd0 
> > >> net/core/sock.c:2620
> > >>  [< inline >] SYSC_setsockopt net/socket.c:1752
> > >>  [] SyS_setsockopt+0x15b/0x250 net/socket.c:1731
> > >>  [] entry_SYSCALL_64_fastpath+0x16/0x7a
> > >> arch/x86/entry/entry_64.S:185
> > >>
> > >> Memory state around the buggy address:
> > >>  88006c636080: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> > >>  88006c636100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> > >> >88006c636180: fc fc fc fc fc fc fc fc fc fc fc fc fc 00 00 fc
> > >> ^
> > >>  88006c636200: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> > >>  88006c636280: 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 +

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

2016-01-25 Thread Neil Horman 
On Mon, Jan 25, 2016 at 02:16:00PM -0200, Marcelo Ricardo Leitner wrote:
> 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, >address_list, list) {
> - error = sctp_add_bind_addr(dest, >a, 1, gfp);
> + error = sctp_add_bind_addr(dest, >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(>a, new, sizeof(*new));
> + memcpy(>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(, rawaddr, htons(port), 0);
> - retval = sctp_add_bind_addr(bp, , SCTP_ADDR_SRC, gfp);
> + retval = sctp_add_bind_addr(bp, , 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, >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. */
>