Re: [Bugme-new] [Bug 8342] New: sctp_getsockopt_local_addrs_old() calls copy_to_user() while a spinlock is held
From: Vlad Yasevich [EMAIL PROTECTED] Date: Thu, 26 Apr 2007 09:22:11 -0400 Here is an updated patch. bytes_copied got turned into an int everywhere since that is what the new API expects, so it should be enough for the old api as well. This also makes the code more consistent. Applied, thanks Vlad. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Bugme-new] [Bug 8342] New: sctp_getsockopt_local_addrs_old() calls copy_to_user() while a spinlock is held
David Miller wrote:
From: Vlad Yasevich [EMAIL PROTECTED]
Date: Mon, 23 Apr 2007 13:43:35 -0400
[PATCH] [SCTP] Fix sctp_getsockopt_local_addrs_old() to use local storage
sctp_getsockopt_local_addrs_old() in net/sctp/socket.c calls copy_to_user()
while the spinlock addr_lock is held. this should not be done as
copy_to_user()
might sleep. the call to sctp_copy_laddrs_to_user() while holding the lock is
also problematic as it calls copy_to_user()
Signed-off-by: Vlad Yasevich [EMAIL PROTECTED]
As Andrew Morton just noticed and fixed in -mm, you're passing
in int pointers to arguments that should be size_t pointers,
specifically for some of the calls to sctp_copy_laddrs().
Please fix this, and please start testing builds on 64-bit
platforms (even if via cross compile) so that you can catch
these as the warnings generated by the compiler on this one
were obvious.
Sorry... I built and tested it on ia64, but the warning scrolled off the page.
Here is an updated patch. bytes_copied got turned into an int everywhere since
that
is what the new API expects, so it should be enough for the old api as well.
This also
makes the code more consistent.
-vlad
From 0df13de48d2c6cb27ee5a73ebd8dff85eb88d1f1 Mon Sep 17 00:00:00 2001
From: Vlad Yasevich [EMAIL PROTECTED]
Date: Mon, 23 Apr 2007 13:41:05 -0400
Subject: [PATCH] [SCTP] Fix sctp_getsockopt_local_addrs_old() to use local
storage
sctp_getsockopt_local_addrs_old() in net/sctp/socket.c calls copy_to_user()
while the spinlock addr_lock is held. this should not be done as copy_to_user()
might sleep. the call to sctp_copy_laddrs_to_user() while holding the lock is
also problematic as it calls copy_to_user()
Signed-off-by: Vlad Yasevich [EMAIL PROTECTED]
---
net/sctp/socket.c | 96 +
1 files changed, 60 insertions(+), 36 deletions(-)
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 6bfae12..e1a91b9 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -3849,7 +3849,7 @@ static int sctp_getsockopt_peer_addrs(struct sock *sk,
int len,
memcpy(temp, from-ipaddr, sizeof(temp));
sctp_get_pf_specific(sk-sk_family)-addr_v4map(sp, temp);
addrlen = sctp_get_af_specific(sk-sk_family)-sockaddr_len;
- if(space_left addrlen)
+ if (space_left addrlen)
return -ENOMEM;
if (copy_to_user(to, temp, addrlen))
return -EFAULT;
@@ -3938,8 +3938,9 @@ done:
/* Helper function that copies local addresses to user and returns the number
* of addresses copied.
*/
-static int sctp_copy_laddrs_to_user_old(struct sock *sk, __u16 port, int
max_addrs,
- void __user *to)
+static int sctp_copy_laddrs_old(struct sock *sk, __u16 port,
+ int max_addrs, void *to,
+ int *bytes_copied)
{
struct list_head *pos, *next;
struct sctp_sockaddr_entry *addr;
@@ -3956,10 +3957,10 @@ static int sctp_copy_laddrs_to_user_old(struct sock
*sk, __u16 port, int max_add
sctp_get_pf_specific(sk-sk_family)-addr_v4map(sctp_sk(sk),
temp);
addrlen = sctp_get_af_specific(temp.sa.sa_family)-sockaddr_len;
- if (copy_to_user(to, temp, addrlen))
- return -EFAULT;
+ memcpy(to, temp, addrlen);
to += addrlen;
+ *bytes_copied += addrlen;
cnt ++;
if (cnt = max_addrs) break;
}
@@ -3967,8 +3968,8 @@ static int sctp_copy_laddrs_to_user_old(struct sock *sk,
__u16 port, int max_add
return cnt;
}
-static int sctp_copy_laddrs_to_user(struct sock *sk, __u16 port,
- void __user **to, size_t space_left)
+static int sctp_copy_laddrs(struct sock *sk, __u16 port, void *to,
+ size_t space_left, int *bytes_copied)
{
struct list_head *pos, *next;
struct sctp_sockaddr_entry *addr;
@@ -3985,14 +3986,14 @@ static int sctp_copy_laddrs_to_user(struct sock *sk,
__u16 port,
sctp_get_pf_specific(sk-sk_family)-addr_v4map(sctp_sk(sk),
temp);
addrlen = sctp_get_af_specific(temp.sa.sa_family)-sockaddr_len;
- if(space_leftaddrlen)
+ if (space_left addrlen)
return -ENOMEM;
- if (copy_to_user(*to, temp, addrlen))
- return -EFAULT;
+ memcpy(to, temp, addrlen);
- *to += addrlen;
+ to += addrlen;
cnt ++;
space_left -= addrlen;
+ bytes_copied += addrlen;
}
return cnt;
@@ -4016,6 +4017,8 @@ static int
Re: [Bugme-new] [Bug 8342] New: sctp_getsockopt_local_addrs_old() calls copy_to_user() while a spinlock is held
From: Vlad Yasevich [EMAIL PROTECTED] Date: Mon, 23 Apr 2007 13:43:35 -0400 [PATCH] [SCTP] Fix sctp_getsockopt_local_addrs_old() to use local storage sctp_getsockopt_local_addrs_old() in net/sctp/socket.c calls copy_to_user() while the spinlock addr_lock is held. this should not be done as copy_to_user() might sleep. the call to sctp_copy_laddrs_to_user() while holding the lock is also problematic as it calls copy_to_user() Signed-off-by: Vlad Yasevich [EMAIL PROTECTED] As Andrew Morton just noticed and fixed in -mm, you're passing in int pointers to arguments that should be size_t pointers, specifically for some of the calls to sctp_copy_laddrs(). Please fix this, and please start testing builds on 64-bit platforms (even if via cross compile) so that you can catch these as the warnings generated by the compiler on this one were obvious. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Bugme-new] [Bug 8342] New: sctp_getsockopt_local_addrs_old() calls copy_to_user() while a spinlock is held
Andrew Morton wrote:
On Mon, 16 Apr 2007 14:34:22 -0700
[EMAIL PROTECTED] wrote:
http://bugzilla.kernel.org/show_bug.cgi?id=8342
Summary: sctp_getsockopt_local_addrs_old() calls copy_to_user()
while a spinlock is held
Kernel Version: 2.6.20
Status: NEW
Severity: normal
Owner: [EMAIL PROTECTED]
Submitter: [EMAIL PROTECTED]
Problem Description:
sctp_getsockopt_local_addrs_old() in net/sctp/socket.c calls copy_to_user()
while the spinlock addr_lock is held. this should not be done as
copy_to_user()
might sleep. the call to sctp_copy_laddrs_to_user() while holding the lock is
also problematic as it calls copy_to_user()
yup.
Thanks for reporting.
The area of this particular lock is quite ugly and will need to be cleaned up.
In the mean time, here is a patch that fixes this for now.
-vlad
[PATCH] [SCTP] Fix sctp_getsockopt_local_addrs_old() to use local storage
sctp_getsockopt_local_addrs_old() in net/sctp/socket.c calls copy_to_user()
while the spinlock addr_lock is held. this should not be done as copy_to_user()
might sleep. the call to sctp_copy_laddrs_to_user() while holding the lock is
also problematic as it calls copy_to_user()
Signed-off-by: Vlad Yasevich [EMAIL PROTECTED]
---
net/sctp/socket.c | 96 +
1 files changed, 60 insertions(+), 36 deletions(-)
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 6bfae12..56ef543 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -3849,7 +3849,7 @@ static int sctp_getsockopt_peer_addrs(struct sock *sk,
int len,
memcpy(temp, from-ipaddr, sizeof(temp));
sctp_get_pf_specific(sk-sk_family)-addr_v4map(sp, temp);
addrlen = sctp_get_af_specific(sk-sk_family)-sockaddr_len;
- if(space_left addrlen)
+ if (space_left addrlen)
return -ENOMEM;
if (copy_to_user(to, temp, addrlen))
return -EFAULT;
@@ -3938,8 +3938,9 @@ done:
/* Helper function that copies local addresses to user and returns the number
* of addresses copied.
*/
-static int sctp_copy_laddrs_to_user_old(struct sock *sk, __u16 port, int
max_addrs,
- void __user *to)
+static int sctp_copy_laddrs_old(struct sock *sk, __u16 port,
+ int max_addrs, void *to,
+ size_t *bytes_copied)
{
struct list_head *pos, *next;
struct sctp_sockaddr_entry *addr;
@@ -3956,10 +3957,10 @@ static int sctp_copy_laddrs_to_user_old(struct sock
*sk, __u16 port, int max_add
sctp_get_pf_specific(sk-sk_family)-addr_v4map(sctp_sk(sk),
temp);
addrlen = sctp_get_af_specific(temp.sa.sa_family)-sockaddr_len;
- if (copy_to_user(to, temp, addrlen))
- return -EFAULT;
+ memcpy(to, temp, addrlen);
to += addrlen;
+ *bytes_copied += addrlen;
cnt ++;
if (cnt = max_addrs) break;
}
@@ -3967,8 +3968,8 @@ static int sctp_copy_laddrs_to_user_old(struct sock *sk,
__u16 port, int max_add
return cnt;
}
-static int sctp_copy_laddrs_to_user(struct sock *sk, __u16 port,
- void __user **to, size_t space_left)
+static int sctp_copy_laddrs(struct sock *sk, __u16 port, void *to,
+ size_t space_left, size_t *bytes_copied)
{
struct list_head *pos, *next;
struct sctp_sockaddr_entry *addr;
@@ -3985,14 +3986,14 @@ static int sctp_copy_laddrs_to_user(struct sock *sk,
__u16 port,
sctp_get_pf_specific(sk-sk_family)-addr_v4map(sctp_sk(sk),
temp);
addrlen = sctp_get_af_specific(temp.sa.sa_family)-sockaddr_len;
- if(space_leftaddrlen)
+ if (space_left addrlen)
return -ENOMEM;
- if (copy_to_user(*to, temp, addrlen))
- return -EFAULT;
+ memcpy(to, temp, addrlen);
- *to += addrlen;
+ to += addrlen;
cnt ++;
space_left -= addrlen;
+ bytes_copied += addrlen;
}
return cnt;
@@ -4016,6 +4017,8 @@ static int sctp_getsockopt_local_addrs_old(struct sock
*sk, int len,
int addrlen;
rwlock_t *addr_lock;
int err = 0;
+ void *addrs;
+ size_t bytes_copied = 0;
if (len != sizeof(struct sctp_getaddrs_old))
return -EINVAL;
@@ -4043,6 +4046,15 @@ static int sctp_getsockopt_local_addrs_old(struct sock
*sk, int len,
to = getaddrs.addrs;
+ /* Allocate space for a local instance of packed
Re: [Bugme-new] [Bug 8342] New: sctp_getsockopt_local_addrs_old() calls copy_to_user() while a spinlock is held
On Mon, 16 Apr 2007 14:34:22 -0700 [EMAIL PROTECTED] wrote: http://bugzilla.kernel.org/show_bug.cgi?id=8342 Summary: sctp_getsockopt_local_addrs_old() calls copy_to_user() while a spinlock is held Kernel Version: 2.6.20 Status: NEW Severity: normal Owner: [EMAIL PROTECTED] Submitter: [EMAIL PROTECTED] Problem Description: sctp_getsockopt_local_addrs_old() in net/sctp/socket.c calls copy_to_user() while the spinlock addr_lock is held. this should not be done as copy_to_user() might sleep. the call to sctp_copy_laddrs_to_user() while holding the lock is also problematic as it calls copy_to_user() yup. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
