* Re: [PATCH net] sctp: fix copying more bytes than expected in sctp_add_bind_addr
2016-03-07 21:17 ` Marcelo Ricardo Leitner
@ 2016-03-07 23:17 kbuild test robot
2016-03-07 21:17 ` Marcelo Ricardo Leitner
2016-03-07 23:27 ` [PATCH net] sctp: fix copying more bytes than expected in sctp_add_bind_addr Marcelo Ricardo Leitner
0 siblings, 2 replies; 13+ messages in thread
From: kbuild test robot @ 2016-03-07 23:17 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: kbuild-all, netdev, dvyukov, nhorman, vyasevich, linux-sctp,
linux-kernel, syzkaller, kcc, glider, sasha.levin, edumazet,
davem
Hi Marcelo,
[auto build test WARNING on net/master]
url: https://github.com/0day-ci/linux/commits/Marcelo-Ricardo-Leitner/sctp-fix-copying-more-bytes-than-expected-in-sctp_add_bind_addr/20160308-052009
coccinelle warnings: (new ones prefixed by >>)
>> net/sctp/bind_addr.c:458:42-48: ERROR: application of sizeof to pointer
Please review and possibly fold the followup patch.
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH net] sctp: fix copying more bytes than expected in sctp_add_bind_addr
@ 2016-03-07 21:17 ` Marcelo Ricardo Leitner
2016-03-07 23:17 ` [PATCH] sctp: fix noderef.cocci warnings kbuild test robot
0 siblings, 1 reply; 13+ messages in thread
From: Marcelo Ricardo Leitner @ 2016-03-07 21:17 UTC (permalink / raw)
To: netdev
Cc: dvyukov, nhorman, vyasevich, linux-sctp, linux-kernel, syzkaller,
kcc, glider, sasha.levin, edumazet, davem
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>
Tested-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 | 3 ++-
net/sctp/socket.c | 4 +++-
5 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 205630bb5010b8ac76b84651b302e488fc1c76ff..f816344f65f2dc47d5a3088d92d77e68aa6fd5c3 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -1098,7 +1098,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 1099e99a53c485402ddd9c0693ff5cdd707accca..d3d50daa248b06d7a4306d903b2dad89e9d2acbd 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -216,6 +216,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..7fe971e30ad6b60e21bfa2986f1c9909a0dabc21 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -1830,7 +1830,8 @@ 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 e878da0949dbfc0012d2e7985bf6e28386678d0f..0e3de0c71137c9ae823bebd7b827561826792d4a 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) {
@@ -577,6 +578,7 @@ static int sctp_send_asconf_add_ip(struct sock *sk,
af = sctp_get_af_specific(addr->v4.sin_family);
memcpy(&saveaddr, addr, af->sockaddr_len);
retval = sctp_add_bind_addr(bp, &saveaddr,
+ sizeof(saveaddr),
SCTP_ADDR_NEW, GFP_ATOMIC);
addr_buf += af->sockaddr_len;
}
--
2.5.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH] sctp: fix noderef.cocci warnings
2016-03-07 21:17 ` Marcelo Ricardo Leitner
@ 2016-03-07 23:17 ` kbuild test robot
0 siblings, 0 replies; 13+ messages in thread
From: kbuild test robot @ 2016-03-07 23:17 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: kbuild-all, netdev, dvyukov, nhorman, vyasevich, linux-sctp,
linux-kernel, syzkaller, kcc, glider, sasha.levin, edumazet,
davem
net/sctp/bind_addr.c:458:42-48: ERROR: application of sizeof to pointer
sizeof when applied to a pointer typed expression gives the size of
the pointer
Generated by: scripts/coccinelle/misc/noderef.cocci
CC: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---
bind_addr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/net/sctp/bind_addr.c
+++ b/net/sctp/bind_addr.c
@@ -455,7 +455,7 @@ static int sctp_copy_one_addr(struct net
(((AF_INET6 == addr->sa.sa_family) &&
(flags & SCTP_ADDR6_ALLOWED) &&
(flags & SCTP_ADDR6_PEERSUPP))))
- error = sctp_add_bind_addr(dest, addr, sizeof(addr),
+ error = sctp_add_bind_addr(dest, addr, sizeof(*addr),
SCTP_ADDR_SRC, gfp);
}
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net] sctp: fix copying more bytes than expected in sctp_add_bind_addr
2016-03-07 23:17 [PATCH net] sctp: fix copying more bytes than expected in sctp_add_bind_addr kbuild test robot
2016-03-07 21:17 ` Marcelo Ricardo Leitner
@ 2016-03-07 23:27 ` Marcelo Ricardo Leitner
2016-03-08 4:15 ` David Miller
1 sibling, 1 reply; 13+ messages in thread
From: Marcelo Ricardo Leitner @ 2016-03-07 23:27 UTC (permalink / raw)
To: kbuild test robot
Cc: kbuild-all, netdev, dvyukov, nhorman, vyasevich, linux-sctp,
linux-kernel, syzkaller, kcc, glider, sasha.levin, edumazet,
davem
Hi,
Em 07-03-2016 20:17, kbuild test robot escreveu:
> Hi Marcelo,
>
> [auto build test WARNING on net/master]
>
> url: https://github.com/0day-ci/linux/commits/Marcelo-Ricardo-Leitner/sctp-fix-copying-more-bytes-than-expected-in-sctp_add_bind_addr/20160308-052009
>
>
> coccinelle warnings: (new ones prefixed by >>)
>
>>> net/sctp/bind_addr.c:458:42-48: ERROR: application of sizeof to pointer
>
> Please review and possibly fold the followup patch.
Oops, nice catch, thanks.
I can fold it if Dave prefers, no problem. I'll wait for a confirmation.
Marcelo
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net] sctp: fix copying more bytes than expected in sctp_add_bind_addr
2016-03-07 23:27 ` [PATCH net] sctp: fix copying more bytes than expected in sctp_add_bind_addr Marcelo Ricardo Leitner
@ 2016-03-08 4:15 ` David Miller
0 siblings, 0 replies; 13+ messages in thread
From: David Miller @ 2016-03-08 4:15 UTC (permalink / raw)
To: marcelo.leitner
Cc: lkp, kbuild-all, netdev, dvyukov, nhorman, vyasevich, linux-sctp,
linux-kernel, syzkaller, kcc, glider, sasha.levin, edumazet
From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date: Mon, 7 Mar 2016 20:27:11 -0300
> Hi,
>
> Em 07-03-2016 20:17, kbuild test robot escreveu:
>> Hi Marcelo,
>>
>> [auto build test WARNING on net/master]
>>
>> url:
>> https://github.com/0day-ci/linux/commits/Marcelo-Ricardo-Leitner/sctp-fix-copying-more-bytes-than-expected-in-sctp_add_bind_addr/20160308-052009
>>
>>
>> coccinelle warnings: (new ones prefixed by >>)
>>
>>>> net/sctp/bind_addr.c:458:42-48: ERROR: application of sizeof to
>>>> pointer
>>
>> Please review and possibly fold the followup patch.
>
> Oops, nice catch, thanks.
>
> I can fold it if Dave prefers, no problem. I'll wait for a
> confirmation.
Please respin your patch with this folded into it, thanks.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: net/sctp: out-of-bounds access in sctp_add_bind_addr
@ 2016-01-25 17:29 Neil Horman
2016-01-25 17:52 ` [PATCH net] sctp: fix copying more bytes than expected " Marcelo Ricardo Leitner
0 siblings, 1 reply; 13+ messages in thread
From: Neil Horman @ 2016-01-25 17:29 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: dvyukov, Vlad Yasevich, netdev, linux-sctp, linux-kernel, davem,
syzkaller, kcc, glider, sasha.levin, edumazet
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, &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
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH net] sctp: fix copying more bytes than expected in sctp_add_bind_addr
2016-01-25 17:29 net/sctp: out-of-bounds access " Neil Horman
@ 2016-01-25 17:52 ` Marcelo Ricardo Leitner
2016-01-26 13:28 ` Dmitry Vyukov
0 siblings, 1 reply; 13+ messages in thread
From: Marcelo Ricardo Leitner @ 2016-01-25 17:52 UTC (permalink / raw)
To: dvyukov
Cc: Neil Horman, Vlad Yasevich, netdev, linux-sctp, linux-kernel,
davem, syzkaller, kcc, glider, sasha.levin, edumazet
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH net] sctp: fix copying more bytes than expected in sctp_add_bind_addr
2016-01-25 17:52 ` [PATCH net] sctp: fix copying more bytes than expected " Marcelo Ricardo Leitner
@ 2016-01-26 13:28 ` Dmitry Vyukov
2016-03-07 19:44 ` Marcelo Ricardo Leitner
0 siblings, 1 reply; 13+ messages in thread
From: Dmitry Vyukov @ 2016-01-26 13:28 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: Neil Horman, Vlad Yasevich, netdev, linux-sctp, LKML,
David Miller, syzkaller, Kostya Serebryany, Alexander Potapenko,
Sasha Levin, Eric Dumazet
On Mon, Jan 25, 2016 at 6:52 PM, Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
> Great. Dmitry, please give this a run. Local tests looked good but who
> knows what syzkaller may find.
Now running with this patch.
> 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
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net] sctp: fix copying more bytes than expected in sctp_add_bind_addr
2016-01-26 13:28 ` Dmitry Vyukov
@ 2016-03-07 19:44 ` Marcelo Ricardo Leitner
2016-03-07 19:45 ` Dmitry Vyukov
0 siblings, 1 reply; 13+ messages in thread
From: Marcelo Ricardo Leitner @ 2016-03-07 19:44 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Neil Horman, Vlad Yasevich, netdev, linux-sctp, LKML,
David Miller, syzkaller, Kostya Serebryany, Alexander Potapenko,
Sasha Levin, Eric Dumazet
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
> <marcelo.leitner@gmail.com> wrote:
> > 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?
> > 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
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net] sctp: fix copying more bytes than expected in sctp_add_bind_addr
2016-03-07 19:44 ` Marcelo Ricardo Leitner
@ 2016-03-07 19:45 ` Dmitry Vyukov
2016-03-07 19:51 ` Marcelo Ricardo Leitner
0 siblings, 1 reply; 13+ messages in thread
From: Dmitry Vyukov @ 2016-03-07 19:45 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: Neil Horman, Vlad Yasevich, netdev, linux-sctp, LKML,
David Miller, syzkaller, Kostya Serebryany, Alexander Potapenko,
Sasha Levin, Eric Dumazet
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
<marcelo.leitner@gmail.com> wrote:
> 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
>> <marcelo.leitner@gmail.com> wrote:
>> > 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?
>
>> > 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
>>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net] sctp: fix copying more bytes than expected in sctp_add_bind_addr
2016-03-07 19:45 ` Dmitry Vyukov
@ 2016-03-07 19:51 ` Marcelo Ricardo Leitner
2016-03-07 19:56 ` Dmitry Vyukov
0 siblings, 1 reply; 13+ messages in thread
From: Marcelo Ricardo Leitner @ 2016-03-07 19:51 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Neil Horman, Vlad Yasevich, netdev, linux-sctp, LKML,
David Miller, syzkaller, Kostya Serebryany, Alexander Potapenko,
Sasha Levin, Eric Dumazet
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
> <marcelo.leitner@gmail.com> wrote:
> > 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
> >> <marcelo.leitner@gmail.com> wrote:
> >> > 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?
> >
> >> > 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
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net] sctp: fix copying more bytes than expected in sctp_add_bind_addr
2016-03-07 19:51 ` Marcelo Ricardo Leitner
@ 2016-03-07 19:56 ` Dmitry Vyukov
2016-03-07 20:00 ` Marcelo Ricardo Leitner
0 siblings, 1 reply; 13+ messages in thread
From: Dmitry Vyukov @ 2016-03-07 19:56 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: Neil Horman, Vlad Yasevich, netdev, linux-sctp, LKML,
David Miller, syzkaller, Kostya Serebryany, Alexander Potapenko,
Sasha Levin, Eric Dumazet
I kept the patch applied.
On Mon, Mar 7, 2016 at 8:51 PM, Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
> 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
>> <marcelo.leitner@gmail.com> wrote:
>> > 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
>> >> <marcelo.leitner@gmail.com> wrote:
>> >> > 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?
>> >
>> >> > 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
>>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net] sctp: fix copying more bytes than expected in sctp_add_bind_addr
2016-03-07 19:56 ` Dmitry Vyukov
@ 2016-03-07 20:00 ` Marcelo Ricardo Leitner
2016-03-07 20:21 ` David Miller
0 siblings, 1 reply; 13+ messages in thread
From: Marcelo Ricardo Leitner @ 2016-03-07 20:00 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Neil Horman, Vlad Yasevich, netdev, linux-sctp, LKML,
David Miller, syzkaller, Kostya Serebryany, Alexander Potapenko,
Sasha Levin, Eric Dumazet
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
> <marcelo.leitner@gmail.com> wrote:
> > 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
> >> <marcelo.leitner@gmail.com> wrote:
> >> > 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
> >> >> <marcelo.leitner@gmail.com> wrote:
> >> >> > 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?
> >> >
> >> >> > 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
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net] sctp: fix copying more bytes than expected in sctp_add_bind_addr
2016-03-07 20:00 ` Marcelo Ricardo Leitner
@ 2016-03-07 20:21 ` David Miller
0 siblings, 0 replies; 13+ messages in thread
From: David Miller @ 2016-03-07 20:21 UTC (permalink / raw)
To: marcelo.leitner
Cc: dvyukov, nhorman, vyasevich, netdev, linux-sctp, linux-kernel,
syzkaller, kcc, glider, sasha.levin, edumazet
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:
>> 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.
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-03-08 4:15 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-03-07 23:17 [PATCH net] sctp: fix copying more bytes than expected in sctp_add_bind_addr kbuild test robot
2016-03-07 21:17 ` Marcelo Ricardo Leitner
2016-03-07 23:17 ` [PATCH] sctp: fix noderef.cocci warnings kbuild test robot
2016-03-07 23:27 ` [PATCH net] sctp: fix copying more bytes than expected in sctp_add_bind_addr Marcelo Ricardo Leitner
2016-03-08 4:15 ` David Miller
-- strict thread matches above, loose matches on Subject: below --
2016-01-25 17:29 net/sctp: out-of-bounds access " Neil Horman
2016-01-25 17:52 ` [PATCH net] sctp: fix copying more bytes than expected " Marcelo Ricardo Leitner
2016-01-26 13:28 ` Dmitry Vyukov
2016-03-07 19:44 ` Marcelo Ricardo Leitner
2016-03-07 19:45 ` Dmitry Vyukov
2016-03-07 19:51 ` Marcelo Ricardo Leitner
2016-03-07 19:56 ` Dmitry Vyukov
2016-03-07 20:00 ` Marcelo Ricardo Leitner
2016-03-07 20:21 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).