* [PATCH] sctp: prevent reading out-of-bounds memory
@ 2010-09-03 13:48 Dan Rosenberg
2010-09-03 14:15 ` Vlad Yasevich
0 siblings, 1 reply; 7+ messages in thread
From: Dan Rosenberg @ 2010-09-03 13:48 UTC (permalink / raw)
To: vladislav.yasevich, sri; +Cc: linux-sctp, linux-kernel
Two user-controlled allocations in SCTP are subsequently dereferenced
as sockaddr structs, without checking if the dereferenced struct
members fall beyond the end of the allocated chunk. There doesn't
appear to be any information leakage here based on how these members
are used and additional checking, but it's still worth fixing.
Signed-off-by: Dan Rosenberg <dan.j.rosenberg@gmail.com>
--- linux-2.6.35.4.orig/net/sctp/socket.c 2010-09-03 08:58:48.127080114 -0400
+++ linux-2.6.35.4/net/sctp/socket.c 2010-09-03 09:22:06.337096825 -0400
@@ -889,6 +889,7 @@ SCTP_STATIC int sctp_setsockopt_bindx(st
int err;
int addrcnt = 0;
int walk_size = 0;
+ unsigned int remaining = addrs_size;
struct sockaddr *sa_addr;
void *addr_buf;
struct sctp_af *af;
@@ -916,6 +917,13 @@ SCTP_STATIC int sctp_setsockopt_bindx(st
/* Walk through the addrs buffer and count the number of addresses. */
addr_buf = kaddrs;
while (walk_size < addrs_size) {
+
+ /* Don't read out-of-bounds memory */
+ if (remaining < sizeof(struct sockaddr)) {
+ kfree(kaddrs);
+ return -EINVAL;
+ }
+
sa_addr = (struct sockaddr *)addr_buf;
af = sctp_get_af_specific(sa_addr->sa_family);
@@ -929,6 +937,7 @@ SCTP_STATIC int sctp_setsockopt_bindx(st
addrcnt++;
addr_buf += af->sockaddr_len;
walk_size += af->sockaddr_len;
+ remaining -= af->sockaddr_len;
}
/* Do the work. */
@@ -984,6 +993,7 @@ static int __sctp_connect(struct sock* s
void *addr_buf;
unsigned short port;
unsigned int f_flags = 0;
+ unsigned int remaining = addrs_size;
sp = sctp_sk(sk);
ep = sp->ep;
@@ -1002,6 +1012,13 @@ static int __sctp_connect(struct sock* s
/* Walk through the addrs buffer and count the number of addresses. */
addr_buf = kaddrs;
while (walk_size < addrs_size) {
+
+ /* Don't read out-of-bounds memory */
+ if (remaining < sizeof(union sctp_addr)) {
+ err = -EINVAL;
+ goto out_free;
+ }
+
sa_addr = (union sctp_addr *)addr_buf;
af = sctp_get_af_specific(sa_addr->sa.sa_family);
port = ntohs(sa_addr->v4.sin_port);
@@ -1101,6 +1118,7 @@ static int __sctp_connect(struct sock* s
addrcnt++;
addr_buf += af->sockaddr_len;
walk_size += af->sockaddr_len;
+ remaining -= af->sockaddr_len;
}
/* In case the user of sctp_connectx() wants an association
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] sctp: prevent reading out-of-bounds memory 2010-09-03 13:48 [PATCH] sctp: prevent reading out-of-bounds memory Dan Rosenberg @ 2010-09-03 14:15 ` Vlad Yasevich 2010-09-03 14:35 ` Dan Rosenberg 0 siblings, 1 reply; 7+ messages in thread From: Vlad Yasevich @ 2010-09-03 14:15 UTC (permalink / raw) To: Dan Rosenberg; +Cc: sri, linux-sctp, linux-kernel On 09/03/2010 09:48 AM, Dan Rosenberg wrote: > Two user-controlled allocations in SCTP are subsequently dereferenced > as sockaddr structs, without checking if the dereferenced struct > members fall beyond the end of the allocated chunk. There doesn't > appear to be any information leakage here based on how these members > are used and additional checking, but it's still worth fixing. > > Signed-off-by: Dan Rosenberg <dan.j.rosenberg@gmail.com> > > --- linux-2.6.35.4.orig/net/sctp/socket.c 2010-09-03 08:58:48.127080114 -0400 > +++ linux-2.6.35.4/net/sctp/socket.c 2010-09-03 09:22:06.337096825 -0400 > @@ -889,6 +889,7 @@ SCTP_STATIC int sctp_setsockopt_bindx(st > int err; > int addrcnt = 0; > int walk_size = 0; > + unsigned int remaining = addrs_size; > struct sockaddr *sa_addr; > void *addr_buf; > struct sctp_af *af; > @@ -916,6 +917,13 @@ SCTP_STATIC int sctp_setsockopt_bindx(st > /* Walk through the addrs buffer and count the number of addresses. */ > addr_buf = kaddrs; > while (walk_size < addrs_size) { > + > + /* Don't read out-of-bounds memory */ > + if (remaining < sizeof(struct sockaddr)) { > + kfree(kaddrs); > + return -EINVAL; > + } > + Hm.. we already validate that we have the proper amount of space for a given sockaddr. The only thing we are missing is making sure that there is room to get the proper address family and I think you can do that without adding any extra variables: if (walk_size + sizeof(sa_family_t) > addr_size) { /* Not enough room for address family */ kfree(kaddrs); return -EINVAL; } -vlad > sa_addr = (struct sockaddr *)addr_buf; > af = sctp_get_af_specific(sa_addr->sa_family); > > @@ -929,6 +937,7 @@ SCTP_STATIC int sctp_setsockopt_bindx(st > addrcnt++; > addr_buf += af->sockaddr_len; > walk_size += af->sockaddr_len; > + remaining -= af->sockaddr_len; > } > > /* Do the work. */ > @@ -984,6 +993,7 @@ static int __sctp_connect(struct sock* s > void *addr_buf; > unsigned short port; > unsigned int f_flags = 0; > + unsigned int remaining = addrs_size; > > sp = sctp_sk(sk); > ep = sp->ep; > @@ -1002,6 +1012,13 @@ static int __sctp_connect(struct sock* s > /* Walk through the addrs buffer and count the number of addresses. */ > addr_buf = kaddrs; > while (walk_size < addrs_size) { > + > + /* Don't read out-of-bounds memory */ > + if (remaining < sizeof(union sctp_addr)) { > + err = -EINVAL; > + goto out_free; > + } > + > sa_addr = (union sctp_addr *)addr_buf; > af = sctp_get_af_specific(sa_addr->sa.sa_family); > port = ntohs(sa_addr->v4.sin_port); > @@ -1101,6 +1118,7 @@ static int __sctp_connect(struct sock* s > addrcnt++; > addr_buf += af->sockaddr_len; > walk_size += af->sockaddr_len; > + remaining -= af->sockaddr_len; > } > > /* In case the user of sctp_connectx() wants an association > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] sctp: prevent reading out-of-bounds memory 2010-09-03 14:15 ` Vlad Yasevich @ 2010-09-03 14:35 ` Dan Rosenberg 2010-09-03 14:47 ` Dan Rosenberg 0 siblings, 1 reply; 7+ messages in thread From: Dan Rosenberg @ 2010-09-03 14:35 UTC (permalink / raw) To: Vlad Yasevich; +Cc: sri, linux-sctp, linux-kernel Ha, I knew there was an easier way. Take two: Signed-off-by: Dan Rosenberg <dan.j.rosenberg@gmail.com> --- linux-2.6.35.4.orig/net/sctp/socket.c 2010-09-03 08:58:48.127080114 -0400 +++ linux-2.6.35.4/net/sctp/socket.c 2010-09-03 10:28:14.929595312 -0400 @@ -916,6 +916,12 @@ SCTP_STATIC int sctp_setsockopt_bindx(st /* Walk through the addrs buffer and count the number of addresses. */ addr_buf = kaddrs; while (walk_size < addrs_size) { + + if (walk_size + sizeof(sa_family_t) > addrs_size) { + kfree(kaddrs); + return -EINVAL; + } + sa_addr = (struct sockaddr *)addr_buf; af = sctp_get_af_specific(sa_addr->sa_family); @@ -1002,6 +1008,12 @@ static int __sctp_connect(struct sock* s /* Walk through the addrs buffer and count the number of addresses. */ addr_buf = kaddrs; while (walk_size < addrs_size) { + + if (walk_size + sizeof(sa_family_t) > addrs_size) { + err = -EINVAL; + goto out_free; + } + sa_addr = (union sctp_addr *)addr_buf; af = sctp_get_af_specific(sa_addr->sa.sa_family); port = ntohs(sa_addr->v4.sin_port); > > Hm.. we already validate that we have the proper amount of space for a given sockaddr. > The only thing we are missing is making sure that there is room to get the proper address > family and I think you can do that without adding any extra variables: > > if (walk_size + sizeof(sa_family_t) > addr_size) { > /* Not enough room for address family */ > kfree(kaddrs); > return -EINVAL; > } > > -vlad > On Fri, Sep 3, 2010 at 10:15 AM, Vlad Yasevich <vladislav.yasevich@hp.com> wrote: > On 09/03/2010 09:48 AM, Dan Rosenberg wrote: >> Two user-controlled allocations in SCTP are subsequently dereferenced >> as sockaddr structs, without checking if the dereferenced struct >> members fall beyond the end of the allocated chunk. There doesn't >> appear to be any information leakage here based on how these members >> are used and additional checking, but it's still worth fixing. >> >> Signed-off-by: Dan Rosenberg <dan.j.rosenberg@gmail.com> >> >> --- linux-2.6.35.4.orig/net/sctp/socket.c 2010-09-03 08:58:48.127080114 -0400 >> +++ linux-2.6.35.4/net/sctp/socket.c 2010-09-03 09:22:06.337096825 -0400 >> @@ -889,6 +889,7 @@ SCTP_STATIC int sctp_setsockopt_bindx(st >> int err; >> int addrcnt = 0; >> int walk_size = 0; >> + unsigned int remaining = addrs_size; >> struct sockaddr *sa_addr; >> void *addr_buf; >> struct sctp_af *af; >> @@ -916,6 +917,13 @@ SCTP_STATIC int sctp_setsockopt_bindx(st >> /* Walk through the addrs buffer and count the number of addresses. */ >> addr_buf = kaddrs; >> while (walk_size < addrs_size) { >> + >> + /* Don't read out-of-bounds memory */ >> + if (remaining < sizeof(struct sockaddr)) { >> + kfree(kaddrs); >> + return -EINVAL; >> + } >> + >> sa_addr = (struct sockaddr *)addr_buf; >> af = sctp_get_af_specific(sa_addr->sa_family); >> >> @@ -929,6 +937,7 @@ SCTP_STATIC int sctp_setsockopt_bindx(st >> addrcnt++; >> addr_buf += af->sockaddr_len; >> walk_size += af->sockaddr_len; >> + remaining -= af->sockaddr_len; >> } >> >> /* Do the work. */ >> @@ -984,6 +993,7 @@ static int __sctp_connect(struct sock* s >> void *addr_buf; >> unsigned short port; >> unsigned int f_flags = 0; >> + unsigned int remaining = addrs_size; >> >> sp = sctp_sk(sk); >> ep = sp->ep; >> @@ -1002,6 +1012,13 @@ static int __sctp_connect(struct sock* s >> /* Walk through the addrs buffer and count the number of addresses. */ >> addr_buf = kaddrs; >> while (walk_size < addrs_size) { >> + >> + /* Don't read out-of-bounds memory */ >> + if (remaining < sizeof(union sctp_addr)) { >> + err = -EINVAL; >> + goto out_free; >> + } >> + >> sa_addr = (union sctp_addr *)addr_buf; >> af = sctp_get_af_specific(sa_addr->sa.sa_family); >> port = ntohs(sa_addr->v4.sin_port); >> @@ -1101,6 +1118,7 @@ static int __sctp_connect(struct sock* s >> addrcnt++; >> addr_buf += af->sockaddr_len; >> walk_size += af->sockaddr_len; >> + remaining -= af->sockaddr_len; >> } >> >> /* In case the user of sctp_connectx() wants an association >> > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] sctp: prevent reading out-of-bounds memory 2010-09-03 14:35 ` Dan Rosenberg @ 2010-09-03 14:47 ` Dan Rosenberg 2010-09-03 15:49 ` Vlad Yasevich 0 siblings, 1 reply; 7+ messages in thread From: Dan Rosenberg @ 2010-09-03 14:47 UTC (permalink / raw) To: Vlad Yasevich; +Cc: sri, linux-sctp, linux-kernel Ugh, just remembered the port number is also dereferenced, so the second of these two checks needs to be expanded to the size of a sockaddr_in. Note to self: don't write patches on too little sleep. Apologies for the unnecessary traffic. Signed-off-by: Dan Rosenberg <dan.j.rosenberg@gmail.com> --- linux-2.6.35.4.orig/net/sctp/socket.c 2010-09-03 08:58:48.127080114 -0400 +++ linux-2.6.35.4/net/sctp/socket.c 2010-09-03 10:45:08.467098052 -0400 @@ -916,6 +916,12 @@ SCTP_STATIC int sctp_setsockopt_bindx(st /* Walk through the addrs buffer and count the number of addresses. */ addr_buf = kaddrs; while (walk_size < addrs_size) { + + if (walk_size + sizeof(sa_family_t) > addrs_size) { + kfree(kaddrs); + return -EINVAL; + } + sa_addr = (struct sockaddr *)addr_buf; af = sctp_get_af_specific(sa_addr->sa_family); @@ -1002,6 +1008,12 @@ static int __sctp_connect(struct sock* s /* Walk through the addrs buffer and count the number of addresses. */ addr_buf = kaddrs; while (walk_size < addrs_size) { + + if (walk_size + sizeof(struct sockaddr_in) > addrs_size) { + err = -EINVAL; + goto out_free; + } + sa_addr = (union sctp_addr *)addr_buf; af = sctp_get_af_specific(sa_addr->sa.sa_family); port = ntohs(sa_addr->v4.sin_port); On Fri, Sep 3, 2010 at 10:35 AM, Dan Rosenberg <dan.j.rosenberg@gmail.com> wrote: > Ha, I knew there was an easier way. Take two: > > Signed-off-by: Dan Rosenberg <dan.j.rosenberg@gmail.com> > > --- linux-2.6.35.4.orig/net/sctp/socket.c 2010-09-03 08:58:48.127080114 -0400 > +++ linux-2.6.35.4/net/sctp/socket.c 2010-09-03 10:28:14.929595312 -0400 > @@ -916,6 +916,12 @@ SCTP_STATIC int sctp_setsockopt_bindx(st > /* Walk through the addrs buffer and count the number of addresses. */ > addr_buf = kaddrs; > while (walk_size < addrs_size) { > + > + if (walk_size + sizeof(sa_family_t) > addrs_size) { > + kfree(kaddrs); > + return -EINVAL; > + } > + > sa_addr = (struct sockaddr *)addr_buf; > af = sctp_get_af_specific(sa_addr->sa_family); > > @@ -1002,6 +1008,12 @@ static int __sctp_connect(struct sock* s > /* Walk through the addrs buffer and count the number of addresses. */ > addr_buf = kaddrs; > while (walk_size < addrs_size) { > + > + if (walk_size + sizeof(sa_family_t) > addrs_size) { > + err = -EINVAL; > + goto out_free; > + } > + > sa_addr = (union sctp_addr *)addr_buf; > af = sctp_get_af_specific(sa_addr->sa.sa_family); > port = ntohs(sa_addr->v4.sin_port); > > >> >> Hm.. we already validate that we have the proper amount of space for a given sockaddr. >> The only thing we are missing is making sure that there is room to get the proper address >> family and I think you can do that without adding any extra variables: >> >> if (walk_size + sizeof(sa_family_t) > addr_size) { >> /* Not enough room for address family */ >> kfree(kaddrs); >> return -EINVAL; >> } >> >> -vlad >> > > On Fri, Sep 3, 2010 at 10:15 AM, Vlad Yasevich > <vladislav.yasevich@hp.com> wrote: >> On 09/03/2010 09:48 AM, Dan Rosenberg wrote: >>> Two user-controlled allocations in SCTP are subsequently dereferenced >>> as sockaddr structs, without checking if the dereferenced struct >>> members fall beyond the end of the allocated chunk. There doesn't >>> appear to be any information leakage here based on how these members >>> are used and additional checking, but it's still worth fixing. >>> >>> Signed-off-by: Dan Rosenberg <dan.j.rosenberg@gmail.com> >>> >>> --- linux-2.6.35.4.orig/net/sctp/socket.c 2010-09-03 08:58:48.127080114 -0400 >>> +++ linux-2.6.35.4/net/sctp/socket.c 2010-09-03 09:22:06.337096825 -0400 >>> @@ -889,6 +889,7 @@ SCTP_STATIC int sctp_setsockopt_bindx(st >>> int err; >>> int addrcnt = 0; >>> int walk_size = 0; >>> + unsigned int remaining = addrs_size; >>> struct sockaddr *sa_addr; >>> void *addr_buf; >>> struct sctp_af *af; >>> @@ -916,6 +917,13 @@ SCTP_STATIC int sctp_setsockopt_bindx(st >>> /* Walk through the addrs buffer and count the number of addresses. */ >>> addr_buf = kaddrs; >>> while (walk_size < addrs_size) { >>> + >>> + /* Don't read out-of-bounds memory */ >>> + if (remaining < sizeof(struct sockaddr)) { >>> + kfree(kaddrs); >>> + return -EINVAL; >>> + } >>> + >>> sa_addr = (struct sockaddr *)addr_buf; >>> af = sctp_get_af_specific(sa_addr->sa_family); >>> >>> @@ -929,6 +937,7 @@ SCTP_STATIC int sctp_setsockopt_bindx(st >>> addrcnt++; >>> addr_buf += af->sockaddr_len; >>> walk_size += af->sockaddr_len; >>> + remaining -= af->sockaddr_len; >>> } >>> >>> /* Do the work. */ >>> @@ -984,6 +993,7 @@ static int __sctp_connect(struct sock* s >>> void *addr_buf; >>> unsigned short port; >>> unsigned int f_flags = 0; >>> + unsigned int remaining = addrs_size; >>> >>> sp = sctp_sk(sk); >>> ep = sp->ep; >>> @@ -1002,6 +1012,13 @@ static int __sctp_connect(struct sock* s >>> /* Walk through the addrs buffer and count the number of addresses. */ >>> addr_buf = kaddrs; >>> while (walk_size < addrs_size) { >>> + >>> + /* Don't read out-of-bounds memory */ >>> + if (remaining < sizeof(union sctp_addr)) { >>> + err = -EINVAL; >>> + goto out_free; >>> + } >>> + >>> sa_addr = (union sctp_addr *)addr_buf; >>> af = sctp_get_af_specific(sa_addr->sa.sa_family); >>> port = ntohs(sa_addr->v4.sin_port); >>> @@ -1101,6 +1118,7 @@ static int __sctp_connect(struct sock* s >>> addrcnt++; >>> addr_buf += af->sockaddr_len; >>> walk_size += af->sockaddr_len; >>> + remaining -= af->sockaddr_len; >>> } >>> >>> /* In case the user of sctp_connectx() wants an association >>> >> >> > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] sctp: prevent reading out-of-bounds memory 2010-09-03 14:47 ` Dan Rosenberg @ 2010-09-03 15:49 ` Vlad Yasevich 2010-09-03 15:54 ` Dan Rosenberg 0 siblings, 1 reply; 7+ messages in thread From: Vlad Yasevich @ 2010-09-03 15:49 UTC (permalink / raw) To: Dan Rosenberg; +Cc: sri, linux-sctp, linux-kernel On 09/03/2010 10:47 AM, Dan Rosenberg wrote: > Ugh, just remembered the port number is also dereferenced, so the > second of these two checks needs to be expanded to the size of a > sockaddr_in. Note to self: don't write patches on too little sleep. > Apologies for the unnecessary traffic. > Actually, you can move that down. Otherwise, we'd end up executing the same code twice which is just silly. So, the code should be like this: 1. see if we can get the address family. 2. Get the address family. 3. see if we get the sockaddr of appropriate size, 4. Get that structure. 5. reference fields. -vlad > Signed-off-by: Dan Rosenberg <dan.j.rosenberg@gmail.com> > > --- linux-2.6.35.4.orig/net/sctp/socket.c 2010-09-03 08:58:48.127080114 -0400 > +++ linux-2.6.35.4/net/sctp/socket.c 2010-09-03 10:45:08.467098052 -0400 > @@ -916,6 +916,12 @@ SCTP_STATIC int sctp_setsockopt_bindx(st > /* Walk through the addrs buffer and count the number of addresses. */ > addr_buf = kaddrs; > while (walk_size < addrs_size) { > + > + if (walk_size + sizeof(sa_family_t) > addrs_size) { > + kfree(kaddrs); > + return -EINVAL; > + } > + > sa_addr = (struct sockaddr *)addr_buf; > af = sctp_get_af_specific(sa_addr->sa_family); > > @@ -1002,6 +1008,12 @@ static int __sctp_connect(struct sock* s > /* Walk through the addrs buffer and count the number of addresses. */ > addr_buf = kaddrs; > while (walk_size < addrs_size) { > + > + if (walk_size + sizeof(struct sockaddr_in) > addrs_size) { > + err = -EINVAL; > + goto out_free; > + } > + > sa_addr = (union sctp_addr *)addr_buf; > af = sctp_get_af_specific(sa_addr->sa.sa_family); > port = ntohs(sa_addr->v4.sin_port); > > > > On Fri, Sep 3, 2010 at 10:35 AM, Dan Rosenberg > <dan.j.rosenberg@gmail.com> wrote: >> Ha, I knew there was an easier way. Take two: >> >> Signed-off-by: Dan Rosenberg <dan.j.rosenberg@gmail.com> >> >> --- linux-2.6.35.4.orig/net/sctp/socket.c 2010-09-03 08:58:48.127080114 -0400 >> +++ linux-2.6.35.4/net/sctp/socket.c 2010-09-03 10:28:14.929595312 -0400 >> @@ -916,6 +916,12 @@ SCTP_STATIC int sctp_setsockopt_bindx(st >> /* Walk through the addrs buffer and count the number of addresses. */ >> addr_buf = kaddrs; >> while (walk_size < addrs_size) { >> + >> + if (walk_size + sizeof(sa_family_t) > addrs_size) { >> + kfree(kaddrs); >> + return -EINVAL; >> + } >> + >> sa_addr = (struct sockaddr *)addr_buf; >> af = sctp_get_af_specific(sa_addr->sa_family); >> >> @@ -1002,6 +1008,12 @@ static int __sctp_connect(struct sock* s >> /* Walk through the addrs buffer and count the number of addresses. */ >> addr_buf = kaddrs; >> while (walk_size < addrs_size) { >> + >> + if (walk_size + sizeof(sa_family_t) > addrs_size) { >> + err = -EINVAL; >> + goto out_free; >> + } >> + >> sa_addr = (union sctp_addr *)addr_buf; >> af = sctp_get_af_specific(sa_addr->sa.sa_family); >> port = ntohs(sa_addr->v4.sin_port); >> >> >>> >>> Hm.. we already validate that we have the proper amount of space for a given sockaddr. >>> The only thing we are missing is making sure that there is room to get the proper address >>> family and I think you can do that without adding any extra variables: >>> >>> if (walk_size + sizeof(sa_family_t) > addr_size) { >>> /* Not enough room for address family */ >>> kfree(kaddrs); >>> return -EINVAL; >>> } >>> >>> -vlad >>> >> >> On Fri, Sep 3, 2010 at 10:15 AM, Vlad Yasevich >> <vladislav.yasevich@hp.com> wrote: >>> On 09/03/2010 09:48 AM, Dan Rosenberg wrote: >>>> Two user-controlled allocations in SCTP are subsequently dereferenced >>>> as sockaddr structs, without checking if the dereferenced struct >>>> members fall beyond the end of the allocated chunk. There doesn't >>>> appear to be any information leakage here based on how these members >>>> are used and additional checking, but it's still worth fixing. >>>> >>>> Signed-off-by: Dan Rosenberg <dan.j.rosenberg@gmail.com> >>>> >>>> --- linux-2.6.35.4.orig/net/sctp/socket.c 2010-09-03 08:58:48.127080114 -0400 >>>> +++ linux-2.6.35.4/net/sctp/socket.c 2010-09-03 09:22:06.337096825 -0400 >>>> @@ -889,6 +889,7 @@ SCTP_STATIC int sctp_setsockopt_bindx(st >>>> int err; >>>> int addrcnt = 0; >>>> int walk_size = 0; >>>> + unsigned int remaining = addrs_size; >>>> struct sockaddr *sa_addr; >>>> void *addr_buf; >>>> struct sctp_af *af; >>>> @@ -916,6 +917,13 @@ SCTP_STATIC int sctp_setsockopt_bindx(st >>>> /* Walk through the addrs buffer and count the number of addresses. */ >>>> addr_buf = kaddrs; >>>> while (walk_size < addrs_size) { >>>> + >>>> + /* Don't read out-of-bounds memory */ >>>> + if (remaining < sizeof(struct sockaddr)) { >>>> + kfree(kaddrs); >>>> + return -EINVAL; >>>> + } >>>> + >>>> sa_addr = (struct sockaddr *)addr_buf; >>>> af = sctp_get_af_specific(sa_addr->sa_family); >>>> >>>> @@ -929,6 +937,7 @@ SCTP_STATIC int sctp_setsockopt_bindx(st >>>> addrcnt++; >>>> addr_buf += af->sockaddr_len; >>>> walk_size += af->sockaddr_len; >>>> + remaining -= af->sockaddr_len; >>>> } >>>> >>>> /* Do the work. */ >>>> @@ -984,6 +993,7 @@ static int __sctp_connect(struct sock* s >>>> void *addr_buf; >>>> unsigned short port; >>>> unsigned int f_flags = 0; >>>> + unsigned int remaining = addrs_size; >>>> >>>> sp = sctp_sk(sk); >>>> ep = sp->ep; >>>> @@ -1002,6 +1012,13 @@ static int __sctp_connect(struct sock* s >>>> /* Walk through the addrs buffer and count the number of addresses. */ >>>> addr_buf = kaddrs; >>>> while (walk_size < addrs_size) { >>>> + >>>> + /* Don't read out-of-bounds memory */ >>>> + if (remaining < sizeof(union sctp_addr)) { >>>> + err = -EINVAL; >>>> + goto out_free; >>>> + } >>>> + >>>> sa_addr = (union sctp_addr *)addr_buf; >>>> af = sctp_get_af_specific(sa_addr->sa.sa_family); >>>> port = ntohs(sa_addr->v4.sin_port); >>>> @@ -1101,6 +1118,7 @@ static int __sctp_connect(struct sock* s >>>> addrcnt++; >>>> addr_buf += af->sockaddr_len; >>>> walk_size += af->sockaddr_len; >>>> + remaining -= af->sockaddr_len; >>>> } >>>> >>>> /* In case the user of sctp_connectx() wants an association >>>> >>> >>> >> > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] sctp: prevent reading out-of-bounds memory 2010-09-03 15:49 ` Vlad Yasevich @ 2010-09-03 15:54 ` Dan Rosenberg 2010-09-03 17:10 ` Vlad Yasevich 0 siblings, 1 reply; 7+ messages in thread From: Dan Rosenberg @ 2010-09-03 15:54 UTC (permalink / raw) To: Vlad Yasevich; +Cc: sri, linux-sctp, linux-kernel Hopefully this covers everything. Signed-off-by: Dan Rosenberg <dan.j.rosenberg@gmail.com> --- linux-2.6.35.4.orig/net/sctp/socket.c 2010-09-03 08:58:48.127080114 -0400 +++ linux-2.6.35.4/net/sctp/socket.c 2010-09-03 11:52:28.239595395 -0400 @@ -916,6 +916,12 @@ SCTP_STATIC int sctp_setsockopt_bindx(st /* Walk through the addrs buffer and count the number of addresses. */ addr_buf = kaddrs; while (walk_size < addrs_size) { + + if (walk_size + sizeof(sa_family_t) > addrs_size) { + kfree(kaddrs); + return -EINVAL; + } + sa_addr = (struct sockaddr *)addr_buf; af = sctp_get_af_specific(sa_addr->sa_family); @@ -1002,9 +1008,14 @@ static int __sctp_connect(struct sock* s /* Walk through the addrs buffer and count the number of addresses. */ addr_buf = kaddrs; while (walk_size < addrs_size) { + + if (walk_size + sizeof(sa_family_t) > addrs_size) { + err = -EINVAL; + goto out_free; + } + sa_addr = (union sctp_addr *)addr_buf; af = sctp_get_af_specific(sa_addr->sa.sa_family); - port = ntohs(sa_addr->v4.sin_port); /* If the address family is not supported or if this address * causes the address buffer to overflow return EINVAL. @@ -1013,6 +1024,8 @@ static int __sctp_connect(struct sock* s err = -EINVAL; goto out_free; } + + port = ntohs(sa_addr->v4.sin_port); /* Save current address so we can work with it */ memcpy(&to, sa_addr, af->sockaddr_len); On Fri, Sep 3, 2010 at 11:49 AM, Vlad Yasevich <vladislav.yasevich@hp.com> wrote: > On 09/03/2010 10:47 AM, Dan Rosenberg wrote: >> Ugh, just remembered the port number is also dereferenced, so the >> second of these two checks needs to be expanded to the size of a >> sockaddr_in. Note to self: don't write patches on too little sleep. >> Apologies for the unnecessary traffic. >> > > Actually, you can move that down. Otherwise, we'd end up executing the same code > twice which is just silly. > > So, the code should be like this: > 1. see if we can get the address family. > 2. Get the address family. > 3. see if we get the sockaddr of appropriate size, > 4. Get that structure. > 5. reference fields. > > -vlad > >> Signed-off-by: Dan Rosenberg <dan.j.rosenberg@gmail.com> >> >> --- linux-2.6.35.4.orig/net/sctp/socket.c 2010-09-03 08:58:48.127080114 -0400 >> +++ linux-2.6.35.4/net/sctp/socket.c 2010-09-03 10:45:08.467098052 -0400 >> @@ -916,6 +916,12 @@ SCTP_STATIC int sctp_setsockopt_bindx(st >> /* Walk through the addrs buffer and count the number of addresses. */ >> addr_buf = kaddrs; >> while (walk_size < addrs_size) { >> + >> + if (walk_size + sizeof(sa_family_t) > addrs_size) { >> + kfree(kaddrs); >> + return -EINVAL; >> + } >> + >> sa_addr = (struct sockaddr *)addr_buf; >> af = sctp_get_af_specific(sa_addr->sa_family); >> >> @@ -1002,6 +1008,12 @@ static int __sctp_connect(struct sock* s >> /* Walk through the addrs buffer and count the number of addresses. */ >> addr_buf = kaddrs; >> while (walk_size < addrs_size) { >> + >> + if (walk_size + sizeof(struct sockaddr_in) > addrs_size) { >> + err = -EINVAL; >> + goto out_free; >> + } >> + >> sa_addr = (union sctp_addr *)addr_buf; >> af = sctp_get_af_specific(sa_addr->sa.sa_family); >> port = ntohs(sa_addr->v4.sin_port); >> >> >> >> On Fri, Sep 3, 2010 at 10:35 AM, Dan Rosenberg >> <dan.j.rosenberg@gmail.com> wrote: >>> Ha, I knew there was an easier way. Take two: >>> >>> Signed-off-by: Dan Rosenberg <dan.j.rosenberg@gmail.com> >>> >>> --- linux-2.6.35.4.orig/net/sctp/socket.c 2010-09-03 08:58:48.127080114 -0400 >>> +++ linux-2.6.35.4/net/sctp/socket.c 2010-09-03 10:28:14.929595312 -0400 >>> @@ -916,6 +916,12 @@ SCTP_STATIC int sctp_setsockopt_bindx(st >>> /* Walk through the addrs buffer and count the number of addresses. */ >>> addr_buf = kaddrs; >>> while (walk_size < addrs_size) { >>> + >>> + if (walk_size + sizeof(sa_family_t) > addrs_size) { >>> + kfree(kaddrs); >>> + return -EINVAL; >>> + } >>> + >>> sa_addr = (struct sockaddr *)addr_buf; >>> af = sctp_get_af_specific(sa_addr->sa_family); >>> >>> @@ -1002,6 +1008,12 @@ static int __sctp_connect(struct sock* s >>> /* Walk through the addrs buffer and count the number of addresses. */ >>> addr_buf = kaddrs; >>> while (walk_size < addrs_size) { >>> + >>> + if (walk_size + sizeof(sa_family_t) > addrs_size) { >>> + err = -EINVAL; >>> + goto out_free; >>> + } >>> + >>> sa_addr = (union sctp_addr *)addr_buf; >>> af = sctp_get_af_specific(sa_addr->sa.sa_family); >>> port = ntohs(sa_addr->v4.sin_port); >>> >>> >>>> >>>> Hm.. we already validate that we have the proper amount of space for a given sockaddr. >>>> The only thing we are missing is making sure that there is room to get the proper address >>>> family and I think you can do that without adding any extra variables: >>>> >>>> if (walk_size + sizeof(sa_family_t) > addr_size) { >>>> /* Not enough room for address family */ >>>> kfree(kaddrs); >>>> return -EINVAL; >>>> } >>>> >>>> -vlad >>>> >>> >>> On Fri, Sep 3, 2010 at 10:15 AM, Vlad Yasevich >>> <vladislav.yasevich@hp.com> wrote: >>>> On 09/03/2010 09:48 AM, Dan Rosenberg wrote: >>>>> Two user-controlled allocations in SCTP are subsequently dereferenced >>>>> as sockaddr structs, without checking if the dereferenced struct >>>>> members fall beyond the end of the allocated chunk. There doesn't >>>>> appear to be any information leakage here based on how these members >>>>> are used and additional checking, but it's still worth fixing. >>>>> >>>>> Signed-off-by: Dan Rosenberg <dan.j.rosenberg@gmail.com> >>>>> >>>>> --- linux-2.6.35.4.orig/net/sctp/socket.c 2010-09-03 08:58:48.127080114 -0400 >>>>> +++ linux-2.6.35.4/net/sctp/socket.c 2010-09-03 09:22:06.337096825 -0400 >>>>> @@ -889,6 +889,7 @@ SCTP_STATIC int sctp_setsockopt_bindx(st >>>>> int err; >>>>> int addrcnt = 0; >>>>> int walk_size = 0; >>>>> + unsigned int remaining = addrs_size; >>>>> struct sockaddr *sa_addr; >>>>> void *addr_buf; >>>>> struct sctp_af *af; >>>>> @@ -916,6 +917,13 @@ SCTP_STATIC int sctp_setsockopt_bindx(st >>>>> /* Walk through the addrs buffer and count the number of addresses. */ >>>>> addr_buf = kaddrs; >>>>> while (walk_size < addrs_size) { >>>>> + >>>>> + /* Don't read out-of-bounds memory */ >>>>> + if (remaining < sizeof(struct sockaddr)) { >>>>> + kfree(kaddrs); >>>>> + return -EINVAL; >>>>> + } >>>>> + >>>>> sa_addr = (struct sockaddr *)addr_buf; >>>>> af = sctp_get_af_specific(sa_addr->sa_family); >>>>> >>>>> @@ -929,6 +937,7 @@ SCTP_STATIC int sctp_setsockopt_bindx(st >>>>> addrcnt++; >>>>> addr_buf += af->sockaddr_len; >>>>> walk_size += af->sockaddr_len; >>>>> + remaining -= af->sockaddr_len; >>>>> } >>>>> >>>>> /* Do the work. */ >>>>> @@ -984,6 +993,7 @@ static int __sctp_connect(struct sock* s >>>>> void *addr_buf; >>>>> unsigned short port; >>>>> unsigned int f_flags = 0; >>>>> + unsigned int remaining = addrs_size; >>>>> >>>>> sp = sctp_sk(sk); >>>>> ep = sp->ep; >>>>> @@ -1002,6 +1012,13 @@ static int __sctp_connect(struct sock* s >>>>> /* Walk through the addrs buffer and count the number of addresses. */ >>>>> addr_buf = kaddrs; >>>>> while (walk_size < addrs_size) { >>>>> + >>>>> + /* Don't read out-of-bounds memory */ >>>>> + if (remaining < sizeof(union sctp_addr)) { >>>>> + err = -EINVAL; >>>>> + goto out_free; >>>>> + } >>>>> + >>>>> sa_addr = (union sctp_addr *)addr_buf; >>>>> af = sctp_get_af_specific(sa_addr->sa.sa_family); >>>>> port = ntohs(sa_addr->v4.sin_port); >>>>> @@ -1101,6 +1118,7 @@ static int __sctp_connect(struct sock* s >>>>> addrcnt++; >>>>> addr_buf += af->sockaddr_len; >>>>> walk_size += af->sockaddr_len; >>>>> + remaining -= af->sockaddr_len; >>>>> } >>>>> >>>>> /* In case the user of sctp_connectx() wants an association >>>>> >>>> >>>> >>> >> > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] sctp: prevent reading out-of-bounds memory 2010-09-03 15:54 ` Dan Rosenberg @ 2010-09-03 17:10 ` Vlad Yasevich 0 siblings, 0 replies; 7+ messages in thread From: Vlad Yasevich @ 2010-09-03 17:10 UTC (permalink / raw) To: Dan Rosenberg; +Cc: sri, linux-sctp, linux-kernel On 09/03/2010 11:54 AM, Dan Rosenberg wrote: > Hopefully this covers everything. > > Signed-off-by: Dan Rosenberg <dan.j.rosenberg@gmail.com> > > --- linux-2.6.35.4.orig/net/sctp/socket.c 2010-09-03 08:58:48.127080114 -0400 > +++ linux-2.6.35.4/net/sctp/socket.c 2010-09-03 11:52:28.239595395 -0400 > @@ -916,6 +916,12 @@ SCTP_STATIC int sctp_setsockopt_bindx(st > /* Walk through the addrs buffer and count the number of addresses. */ > addr_buf = kaddrs; > while (walk_size < addrs_size) { > + > + if (walk_size + sizeof(sa_family_t) > addrs_size) { > + kfree(kaddrs); > + return -EINVAL; > + } > + > sa_addr = (struct sockaddr *)addr_buf; > af = sctp_get_af_specific(sa_addr->sa_family); > > @@ -1002,9 +1008,14 @@ static int __sctp_connect(struct sock* s > /* Walk through the addrs buffer and count the number of addresses. */ > addr_buf = kaddrs; > while (walk_size < addrs_size) { > + > + if (walk_size + sizeof(sa_family_t) > addrs_size) { > + err = -EINVAL; > + goto out_free; > + } > + > sa_addr = (union sctp_addr *)addr_buf; > af = sctp_get_af_specific(sa_addr->sa.sa_family); > - port = ntohs(sa_addr->v4.sin_port); > > /* If the address family is not supported or if this address > * causes the address buffer to overflow return EINVAL. > @@ -1013,6 +1024,8 @@ static int __sctp_connect(struct sock* s > err = -EINVAL; > goto out_free; > } > + > + port = ntohs(sa_addr->v4.sin_port); > > /* Save current address so we can work with it */ > memcpy(&to, sa_addr, af->sockaddr_len); > > Looks good. Now you just need to resend a clean version. :) -vlad > > On Fri, Sep 3, 2010 at 11:49 AM, Vlad Yasevich > <vladislav.yasevich@hp.com> wrote: >> On 09/03/2010 10:47 AM, Dan Rosenberg wrote: >>> Ugh, just remembered the port number is also dereferenced, so the >>> second of these two checks needs to be expanded to the size of a >>> sockaddr_in. Note to self: don't write patches on too little sleep. >>> Apologies for the unnecessary traffic. >>> >> >> Actually, you can move that down. Otherwise, we'd end up executing the same code >> twice which is just silly. >> >> So, the code should be like this: >> 1. see if we can get the address family. >> 2. Get the address family. >> 3. see if we get the sockaddr of appropriate size, >> 4. Get that structure. >> 5. reference fields. >> >> -vlad >> >>> Signed-off-by: Dan Rosenberg <dan.j.rosenberg@gmail.com> >>> >>> --- linux-2.6.35.4.orig/net/sctp/socket.c 2010-09-03 08:58:48.127080114 -0400 >>> +++ linux-2.6.35.4/net/sctp/socket.c 2010-09-03 10:45:08.467098052 -0400 >>> @@ -916,6 +916,12 @@ SCTP_STATIC int sctp_setsockopt_bindx(st >>> /* Walk through the addrs buffer and count the number of addresses. */ >>> addr_buf = kaddrs; >>> while (walk_size < addrs_size) { >>> + >>> + if (walk_size + sizeof(sa_family_t) > addrs_size) { >>> + kfree(kaddrs); >>> + return -EINVAL; >>> + } >>> + >>> sa_addr = (struct sockaddr *)addr_buf; >>> af = sctp_get_af_specific(sa_addr->sa_family); >>> >>> @@ -1002,6 +1008,12 @@ static int __sctp_connect(struct sock* s >>> /* Walk through the addrs buffer and count the number of addresses. */ >>> addr_buf = kaddrs; >>> while (walk_size < addrs_size) { >>> + >>> + if (walk_size + sizeof(struct sockaddr_in) > addrs_size) { >>> + err = -EINVAL; >>> + goto out_free; >>> + } >>> + >>> sa_addr = (union sctp_addr *)addr_buf; >>> af = sctp_get_af_specific(sa_addr->sa.sa_family); >>> port = ntohs(sa_addr->v4.sin_port); >>> >>> >>> >>> On Fri, Sep 3, 2010 at 10:35 AM, Dan Rosenberg >>> <dan.j.rosenberg@gmail.com> wrote: >>>> Ha, I knew there was an easier way. Take two: >>>> >>>> Signed-off-by: Dan Rosenberg <dan.j.rosenberg@gmail.com> >>>> >>>> --- linux-2.6.35.4.orig/net/sctp/socket.c 2010-09-03 08:58:48.127080114 -0400 >>>> +++ linux-2.6.35.4/net/sctp/socket.c 2010-09-03 10:28:14.929595312 -0400 >>>> @@ -916,6 +916,12 @@ SCTP_STATIC int sctp_setsockopt_bindx(st >>>> /* Walk through the addrs buffer and count the number of addresses. */ >>>> addr_buf = kaddrs; >>>> while (walk_size < addrs_size) { >>>> + >>>> + if (walk_size + sizeof(sa_family_t) > addrs_size) { >>>> + kfree(kaddrs); >>>> + return -EINVAL; >>>> + } >>>> + >>>> sa_addr = (struct sockaddr *)addr_buf; >>>> af = sctp_get_af_specific(sa_addr->sa_family); >>>> >>>> @@ -1002,6 +1008,12 @@ static int __sctp_connect(struct sock* s >>>> /* Walk through the addrs buffer and count the number of addresses. */ >>>> addr_buf = kaddrs; >>>> while (walk_size < addrs_size) { >>>> + >>>> + if (walk_size + sizeof(sa_family_t) > addrs_size) { >>>> + err = -EINVAL; >>>> + goto out_free; >>>> + } >>>> + >>>> sa_addr = (union sctp_addr *)addr_buf; >>>> af = sctp_get_af_specific(sa_addr->sa.sa_family); >>>> port = ntohs(sa_addr->v4.sin_port); >>>> >>>> >>>>> >>>>> Hm.. we already validate that we have the proper amount of space for a given sockaddr. >>>>> The only thing we are missing is making sure that there is room to get the proper address >>>>> family and I think you can do that without adding any extra variables: >>>>> >>>>> if (walk_size + sizeof(sa_family_t) > addr_size) { >>>>> /* Not enough room for address family */ >>>>> kfree(kaddrs); >>>>> return -EINVAL; >>>>> } >>>>> >>>>> -vlad >>>>> >>>> >>>> On Fri, Sep 3, 2010 at 10:15 AM, Vlad Yasevich >>>> <vladislav.yasevich@hp.com> wrote: >>>>> On 09/03/2010 09:48 AM, Dan Rosenberg wrote: >>>>>> Two user-controlled allocations in SCTP are subsequently dereferenced >>>>>> as sockaddr structs, without checking if the dereferenced struct >>>>>> members fall beyond the end of the allocated chunk. There doesn't >>>>>> appear to be any information leakage here based on how these members >>>>>> are used and additional checking, but it's still worth fixing. >>>>>> >>>>>> Signed-off-by: Dan Rosenberg <dan.j.rosenberg@gmail.com> >>>>>> >>>>>> --- linux-2.6.35.4.orig/net/sctp/socket.c 2010-09-03 08:58:48.127080114 -0400 >>>>>> +++ linux-2.6.35.4/net/sctp/socket.c 2010-09-03 09:22:06.337096825 -0400 >>>>>> @@ -889,6 +889,7 @@ SCTP_STATIC int sctp_setsockopt_bindx(st >>>>>> int err; >>>>>> int addrcnt = 0; >>>>>> int walk_size = 0; >>>>>> + unsigned int remaining = addrs_size; >>>>>> struct sockaddr *sa_addr; >>>>>> void *addr_buf; >>>>>> struct sctp_af *af; >>>>>> @@ -916,6 +917,13 @@ SCTP_STATIC int sctp_setsockopt_bindx(st >>>>>> /* Walk through the addrs buffer and count the number of addresses. */ >>>>>> addr_buf = kaddrs; >>>>>> while (walk_size < addrs_size) { >>>>>> + >>>>>> + /* Don't read out-of-bounds memory */ >>>>>> + if (remaining < sizeof(struct sockaddr)) { >>>>>> + kfree(kaddrs); >>>>>> + return -EINVAL; >>>>>> + } >>>>>> + >>>>>> sa_addr = (struct sockaddr *)addr_buf; >>>>>> af = sctp_get_af_specific(sa_addr->sa_family); >>>>>> >>>>>> @@ -929,6 +937,7 @@ SCTP_STATIC int sctp_setsockopt_bindx(st >>>>>> addrcnt++; >>>>>> addr_buf += af->sockaddr_len; >>>>>> walk_size += af->sockaddr_len; >>>>>> + remaining -= af->sockaddr_len; >>>>>> } >>>>>> >>>>>> /* Do the work. */ >>>>>> @@ -984,6 +993,7 @@ static int __sctp_connect(struct sock* s >>>>>> void *addr_buf; >>>>>> unsigned short port; >>>>>> unsigned int f_flags = 0; >>>>>> + unsigned int remaining = addrs_size; >>>>>> >>>>>> sp = sctp_sk(sk); >>>>>> ep = sp->ep; >>>>>> @@ -1002,6 +1012,13 @@ static int __sctp_connect(struct sock* s >>>>>> /* Walk through the addrs buffer and count the number of addresses. */ >>>>>> addr_buf = kaddrs; >>>>>> while (walk_size < addrs_size) { >>>>>> + >>>>>> + /* Don't read out-of-bounds memory */ >>>>>> + if (remaining < sizeof(union sctp_addr)) { >>>>>> + err = -EINVAL; >>>>>> + goto out_free; >>>>>> + } >>>>>> + >>>>>> sa_addr = (union sctp_addr *)addr_buf; >>>>>> af = sctp_get_af_specific(sa_addr->sa.sa_family); >>>>>> port = ntohs(sa_addr->v4.sin_port); >>>>>> @@ -1101,6 +1118,7 @@ static int __sctp_connect(struct sock* s >>>>>> addrcnt++; >>>>>> addr_buf += af->sockaddr_len; >>>>>> walk_size += af->sockaddr_len; >>>>>> + remaining -= af->sockaddr_len; >>>>>> } >>>>>> >>>>>> /* In case the user of sctp_connectx() wants an association >>>>>> >>>>> >>>>> >>>> >>> >> >> > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2010-09-03 17:10 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-09-03 13:48 [PATCH] sctp: prevent reading out-of-bounds memory Dan Rosenberg 2010-09-03 14:15 ` Vlad Yasevich 2010-09-03 14:35 ` Dan Rosenberg 2010-09-03 14:47 ` Dan Rosenberg 2010-09-03 15:49 ` Vlad Yasevich 2010-09-03 15:54 ` Dan Rosenberg 2010-09-03 17:10 ` Vlad Yasevich
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox