sctp: fix memory leak in sctp_datamsg_from_user() when copy from user space fails
Subsystems (3)
PATCH 1/1 sctp: fix memory leak in sctp_datamsg_from_user() when copy from user space fails
Trinity (the syscall fuzzer) discovered a memory leak in SCTP, reproducible e.g. with the sendto() syscall by passing invalid user space pointer in the second argument:
#include <string.h>
#include <arpa/inet.h>
#include <sys/socket.h>
int main(void)
{
int fd;
struct sockaddr_in sa;
fd = socket(AF_INET, SOCK_STREAM, 132 /*IPPROTO_SCTP*/);
if (fd < 0)
return 1;
memset(&sa, 0, sizeof(sa));
sa.sin_family = AF_INET;
sa.sin_addr.s_addr = inet_addr("127.0.0.1");
sa.sin_port = htons(11111);
sendto(fd, NULL, 1, 0, (struct sockaddr *)&sa, sizeof(sa));
return 0;}
As far as I can tell, the leak has been around since ~2003.
--- net/sctp/chunk.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
net/sctp/chunk.c +5 -2
@@ -284,7 +284,7 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc, goto errout; err =sctp_user_addto_chunk (chunk, offset, len, msgh->msg_iov);if (err <0 ) - goto errout; + goto errout_chunk_put; offset += len;
@@ -324,7 +324,7 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,__skb_pull (chunk->skb, (__u8 *)chunk->chunk_hdr - (__u8 *)chunk->skb->data);if (err <0 ) - goto errout; + goto errout_chunk_put;sctp_datamsg_assign (msg, chunk);list_add_tail (&chunk->frag_list, &msg->chunks);
@@ -332,6 +332,9 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,return msg; +errout_chunk_put: +sctp_chunk_put (chunk); + errout:list_for_each_safe (pos, temp, &msg->chunks) {list_del_init (pos);
Signature · 1 line
1.7.9.5
David Miller
maintainerFrom: Tommi Rantala <[email protected]>
Date: Thu, 22 Nov 2012 15:21:31 +0200
Quoted 3 lines
Trinity (the syscall fuzzer) discovered a memory leak in SCTP, reproducible e.g. with the
sendto()syscall by passing invalid user space pointer in the second argument:
...
Quoted 3 lines
As far as I can tell, the leak has been around since ~2003.
Signed-off-by Tommi Rantala <[email protected]>
Looks good to me, Vlad please review.
Quoted 58 lines · Tommi Rantala
Trinity (the syscall fuzzer) discovered a memory leak in SCTP, reproducible e.g. with the
sendto()syscall by passing invalid user space pointer in the second argument:#include <string.h> #include <arpa/inet.h> #include <sys/socket.h> int main(void) { int fd; struct sockaddr_in sa;fd = socket(AF_INET, SOCK_STREAM, 132 /*IPPROTO_SCTP*/); if (fd < 0) return 1; memset(&sa, 0, sizeof(sa)); sa.sin_family = AF_INET; sa.sin_addr.s_addr = inet_addr("127.0.0.1"); sa.sin_port = htons(11111); sendto(fd, NULL, 1, 0, (struct sockaddr *)&sa, sizeof(sa)); return 0;}
As far as I can tell, the leak has been around since ~2003.
Signed-off-by Tommi Rantala <[email protected]>--- net/sctp/chunk.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)net/sctp/chunk.c +2
-2@@ -284,7 +284,7 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc, goto errout; err =sctp_user_addto_chunk (chunk, offset, len, msgh->msg_iov);if (err <0 ) - goto errout; + goto errout_chunk_put; offset += len;@@ -324,7 +324,7 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,__skb_pull (chunk->skb, (__u8 *)chunk->chunk_hdr - (__u8 *)chunk->skb->data);if (err <0 ) - goto errout; + goto errout_chunk_put;sctp_datamsg_assign (msg, chunk);list_add_tail (&chunk->frag_list, &msg->chunks);
net/sctp/chunk.c struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc, · line 340
Quoted 9 lines
net/sctp/chunk.c +3
-0@@ -332,6 +332,9 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,return msg; +errout_chunk_put: +sctp_chunk_put (chunk); + errout:list_for_each_safe (pos, temp, &msg->chunks) {list_del_init (pos);
Should be using sctp_chunk_free(). Good find.
-vlad
David Miller
maintainerFrom: Vlad Yasevich <[email protected]>
Date: Mon, 26 Nov 2012 09:52:37 -0500
Quoted 1 line
Should be using
sctp_chunk_free(). Good find.
Tommi, please respin your patch to use sctp_chunk_free() as Vlad has asked.
Thanks.
PATCH 1/1 sctp: fix memory leak in sctp_datamsg_from_user() when copy from user space fails
Trinity (the syscall fuzzer) discovered a memory leak in SCTP, reproducible e.g. with the sendto() syscall by passing invalid user space pointer in the second argument:
#include <string.h>
#include <arpa/inet.h>
#include <sys/socket.h>
int main(void)
{
int fd;
struct sockaddr_in sa;
fd = socket(AF_INET, SOCK_STREAM, 132 /*IPPROTO_SCTP*/);
if (fd < 0)
return 1;
memset(&sa, 0, sizeof(sa));
sa.sin_family = AF_INET;
sa.sin_addr.s_addr = inet_addr("127.0.0.1");
sa.sin_port = htons(11111);
sendto(fd, NULL, 1, 0, (struct sockaddr *)&sa, sizeof(sa));
return 0;}
As far as I can tell, the leak has been around since ~2003.
--- net/sctp/chunk.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
net/sctp/chunk.c +5 -2
@@ -284,7 +284,7 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc, goto errout; err =sctp_user_addto_chunk (chunk, offset, len, msgh->msg_iov);if (err <0 ) - goto errout; + goto errout_chunk_free; offset += len;
@@ -324,7 +324,7 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,__skb_pull (chunk->skb, (__u8 *)chunk->chunk_hdr - (__u8 *)chunk->skb->data);if (err <0 ) - goto errout; + goto errout_chunk_free;sctp_datamsg_assign (msg, chunk);list_add_tail (&chunk->frag_list, &msg->chunks);
@@ -332,6 +332,9 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,return msg; +errout_chunk_free: +sctp_chunk_free (chunk); + errout:list_for_each_safe (pos, temp, &msg->chunks) {list_del_init (pos);
Signature · 1 line
1.7.9.5
Quoted 25 lines · Tommi Rantala
Trinity (the syscall fuzzer) discovered a memory leak in SCTP, reproducible e.g. with the
sendto()syscall by passing invalid user space pointer in the second argument:#include <string.h> #include <arpa/inet.h> #include <sys/socket.h> int main(void) { int fd; struct sockaddr_in sa;fd = socket(AF_INET, SOCK_STREAM, 132 /*IPPROTO_SCTP*/); if (fd < 0) return 1; memset(&sa, 0, sizeof(sa)); sa.sin_family = AF_INET; sa.sin_addr.s_addr = inet_addr("127.0.0.1"); sa.sin_port = htons(11111); sendto(fd, NULL, 1, 0, (struct sockaddr *)&sa, sizeof(sa)); return 0;
Quoted 5 lines
}
As far as I can tell, the leak has been around since ~2003.
Signed-off-by Tommi Rantala <[email protected]>
-vlad
Quoted 37 lines
--- net/sctp/chunk.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)net/sctp/chunk.c +5
-2@@ -284,7 +284,7 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc, goto errout; err =sctp_user_addto_chunk (chunk, offset, len, msgh->msg_iov);if (err <0 ) - goto errout; + goto errout_chunk_free; offset += len;@@ -324,7 +324,7 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,__skb_pull (chunk->skb, (__u8 *)chunk->chunk_hdr - (__u8 *)chunk->skb->data);if (err <0 ) - goto errout; + goto errout_chunk_free;sctp_datamsg_assign (msg, chunk);list_add_tail (&chunk->frag_list, &msg->chunks);@@ -332,6 +332,9 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,return msg; +errout_chunk_free: +sctp_chunk_free (chunk); + errout:list_for_each_safe (pos, temp, &msg->chunks) {list_del_init (pos);
From: Tommi Rantala <[email protected]>
Date: Tue, 27 Nov 2012 16:01:46 +0200
Quoted 3 lines
Trinity (the syscall fuzzer) discovered a memory leak in SCTP, reproducible e.g. with the
sendto()syscall by passing invalid user space pointer in the second argument:
...
Quoted 3 lines
As far as I can tell, the leak has been around since ~2003.
Signed-off-by Tommi Rantala <[email protected]>
Applied.
Quoted 74 lines · Tommi Rantala
Trinity (the syscall fuzzer) discovered a memory leak in SCTP, reproducible e.g. with the
sendto()syscall by passing invalid user space pointer in the second argument:#include <
string.h>
#include <arpa/inet.h>
#include <sys/socket.h>int main(void)
{ int fd; struct sockaddr_in sa; fd = socket(AF_INET, SOCK_STREAM, 132 /*IPPROTO_SCTP*/); if (fd < 0) return 1; memset(&sa, 0, sizeof(sa)); sa.sin_family = AF_INET; sa.sin_addr.s_addr = inet_addr("127.0.0.1"); sa.sin_port = htons(11111); sendto(fd, NULL, 1, 0, (struct sockaddr *)&sa, sizeof(sa)); return 0;}
As far as I can tell, the leak has been around since ~2003.
Signed-off-by Tommi Rantala <[email protected]>--- net/sctp/chunk.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)net/sctp/chunk.c +5
-2@@ -284,7 +284,7 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc, goto errout; err =sctp_user_addto_chunk (chunk, offset, len, msgh->msg_iov);if (err <0 ) - goto errout; + goto errout_chunk_put; offset += len;@@ -324,7 +324,7 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,__skb_pull (chunk->skb, (__u8 *)chunk->chunk_hdr - (__u8 *)chunk->skb->data);if (err <0 ) - goto errout; + goto errout_chunk_put;sctp_datamsg_assign (msg, chunk);list_add_tail (&chunk->frag_list, &msg->chunks);@@ -332,6 +332,9 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,return msg; +errout_chunk_put: +sctp_chunk_put (chunk); + errout:list_for_each_safe (pos, temp, &msg->chunks) {list_del_init (pos);Signature · 6 lines
1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-sctp" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
I'm fine with it the way it is, but it might be nicer if you instead just moved the list_add_tail call up between the if (!chunk) check and the sctp_user_addto_chunk call. That way the unwind loop at the errout label can just free the chunk without the need for an extra label.
Neil