Re: [PATCH] Fix verify_iovec() to not allow overflow of iov_len values
On Tue, 2006-08-29 at 21:44 -0700, David Miller wrote:
From: Sridhar Samudrala [EMAIL PROTECTED]
Date: Tue, 29 Aug 2006 10:55:29 -0700
verify_iovec() has the following piece of code that allows overflow
of iov_len values in an iovec.
for (ct = 0; ct m-msg_iovlen; ct++) {
err += iov[ct].iov_len;
/*
* Goal is not to verify user data, but to prevent returning
* negative value, which is interpreted as errno.
* Overflow is still possible, but it is harmless.
*/
if (err 0)
return -EMSGSIZE;
}
The comment specifically says that overflow is harmless, but I don't see any
valid reason to allow overflow.
Also, iov_len is of type size_t which is a 64-bit value on 64-bit systems,
but we store and return the overall iovec length in an int.
Just some notes so that it is understood the context in which
such changes are being proposed.
First of all, several memcpy() and copy_{to,from}_user()
implementations BUG trap on lengths which have bit 31 or higher set.
It always indicates a bug of some sort in the call chain, so behaving
as if we support larger than positive signed 32-bit lengths here is
probably asking for trouble.
Also, the protocol downcalls all return int, not long so it's even
impossible to report successful sending of larger than positive signed
32-bit lengths. Even kernel_sendmsg() and friends return int.
We can pretend that larger than positive signed 32-bit lengths in an
iovec will work, but they surely won't.
And since the sendmsg() man page was quoted:
If the message is too long to pass atomically through the underlying
protocol, the error EMSGSIZE is returned, and the message is not
trans$,1rp
mitted.
which matches perfectly with the fact that the protocol downcalls
return int not size_t nor ssize_t.
OK. So the maximum message size that can be supported via sendmsg/recvmsg
is limited to INT_MAX. Then i think we should limit each iov_len value
and the sum of all iov_len values to INT_MAX.
It looks like copy_{to,from}_user() support upto LONG_MAX but most of the
networking routines return only an int.
Currently if we pass an iovec with 2 iov_len values 0x7fff and
0x8002, verify_iovec returns a total length as 1 and we proceed
without any error with a truncated user message.
The following patch avoids overflow of iov_len values beyond INT_MAX.
diff --git a/net/compat.c b/net/compat.c
index d5d69fa..e8f5345 100644
--- a/net/compat.c
+++ b/net/compat.c
@@ -44,7 +44,11 @@ static inline int iov_from_user_compat_t
tot_len = -EFAULT;
break;
}
+ if (unlikely(len INT_MAX))
+ return -EMSGSIZE;
tot_len += len;
+ if (unlikely(tot_len 0))
+ return -EMSGSIZE;
kiov-iov_base = compat_ptr(buf);
kiov-iov_len = (__kernel_size_t) len;
uiov32++;
diff --git a/net/core/iovec.c b/net/core/iovec.c
index 65e4b56..ee5a265 100644
--- a/net/core/iovec.c
+++ b/net/core/iovec.c
@@ -61,11 +61,12 @@ int verify_iovec(struct msghdr *m, struc
err = 0;
for (ct = 0; ct m-msg_iovlen; ct++) {
+ if (unlikely(iov[ct].iov_len INT_MAX))
+ return -EMSGSIZE;
err += iov[ct].iov_len;
/*
* Goal is not to verify user data, but to prevent returning
* negative value, which is interpreted as errno.
-* Overflow is still possible, but it is harmless.
*/
if (err 0)
return -EMSGSIZE;
-
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
[PATCH] Fix verify_iovec() to not allow overflow of iov_len values
verify_iovec() has the following piece of code that allows overflow
of iov_len values in an iovec.
for (ct = 0; ct m-msg_iovlen; ct++) {
err += iov[ct].iov_len;
/*
* Goal is not to verify user data, but to prevent returning
* negative value, which is interpreted as errno.
* Overflow is still possible, but it is harmless.
*/
if (err 0)
return -EMSGSIZE;
}
The comment specifically says that overflow is harmless, but I don't see any
valid reason to allow overflow.
Also, iov_len is of type size_t which is a 64-bit value on 64-bit systems,
but we store and return the overall iovec length in an int.
This patch changes the return type of verify_iovec() and verify_compat_iovec()
to ssize_t and also validates that each iov_len value and the sum of all
the iov_len values do not overflow ssize_t. In case of a overflow, EINVAL is
returned.
The man pages for sendmsg/recvmsg also list this failure mode.
http://www.die.net/doc/linux/man/man3/sendmsg.3.html
EINVAL
The sum of the iov_len values overflows an ssize_t
This is based on some earlier patches from Chris Wright and Andrew Morton that
were incomplete.
Signed-off-by: Sridhar Samudrala [EMAIL PROTECTED]
diff --git a/include/linux/socket.h b/include/linux/socket.h
index 3614090..25645b0 100644
--- a/include/linux/socket.h
+++ b/include/linux/socket.h
@@ -294,7 +294,7 @@ extern int csum_partial_copy_fromiovecen
int offset,
unsigned int len, int *csump);
-extern int verify_iovec(struct msghdr *m, struct iovec *iov, char *address,
int mode);
+extern ssize_t verify_iovec(struct msghdr *m, struct iovec *iov, char
*address, int mode);
extern int memcpy_toiovec(struct iovec *v, unsigned char *kdata, int len);
extern int move_addr_to_user(void *kaddr, int klen, void __user *uaddr, int
__user *ulen);
extern int move_addr_to_kernel(void __user *uaddr, int ulen, void *kaddr);
diff --git a/include/net/compat.h b/include/net/compat.h
index 9859b60..c30d648 100644
--- a/include/net/compat.h
+++ b/include/net/compat.h
@@ -31,7 +31,7 @@ #define compat_msghdr msghdr /* to avoi
#endif /* defined(CONFIG_COMPAT) */
extern int get_compat_msghdr(struct msghdr *, struct compat_msghdr __user *);
-extern int verify_compat_iovec(struct msghdr *, struct iovec *, char *, int);
+extern ssize_t verify_compat_iovec(struct msghdr *, struct iovec *, char *,
int);
extern asmlinkage long compat_sys_sendmsg(int,struct compat_msghdr __user
*,unsigned);
extern asmlinkage long compat_sys_recvmsg(int,struct compat_msghdr __user
*,unsigned);
extern asmlinkage long compat_sys_getsockopt(int, int, int, char __user *, int
__user *);
diff --git a/net/compat.c b/net/compat.c
index d5d69fa..f5d1a96 100644
--- a/net/compat.c
+++ b/net/compat.c
@@ -29,22 +29,29 @@ #include net/sock.h
#include asm/uaccess.h
#include net/compat.h
-static inline int iov_from_user_compat_to_kern(struct iovec *kiov,
+static inline ssize_t iov_from_user_compat_to_kern(struct iovec *kiov,
struct compat_iovec __user *uiov32,
int niov)
{
- int tot_len = 0;
+ compat_ssize_t tot_len = 0;
while(niov 0) {
compat_uptr_t buf;
- compat_size_t len;
+ compat_ssize_t len;
if(get_user(len, uiov32-iov_len) ||
get_user(buf, uiov32-iov_base)) {
tot_len = -EFAULT;
break;
}
+ /* Validate that each iov_len value and the sum of all the
+* iov_len values do not overflow ssize_t.
+*/
+ if (len 0)
+ return -EINVAL;
tot_len += len;
+ if (tot_len 0)
+ return -EINVAL;
kiov-iov_base = compat_ptr(buf);
kiov-iov_len = (__kernel_size_t) len;
uiov32++;
@@ -74,10 +81,10 @@ int get_compat_msghdr(struct msghdr *kms
}
/* I've named the args so it is easy to tell whose space the pointers are in.
*/
-int verify_compat_iovec(struct msghdr *kern_msg, struct iovec *kern_iov,
+ssize_t verify_compat_iovec(struct msghdr *kern_msg, struct iovec *kern_iov,
char *kern_address, int mode)
{
- int tot_len;
+ ssize_t tot_len;
if(kern_msg-msg_namelen) {
if(mode==VERIFY_READ) {
diff --git a/net/core/iovec.c b/net/core/iovec.c
index 65e4b56..14b0f8f 100644
--- a/net/core/iovec.c
+++ b/net/core/iovec.c
@@ -37,9 +37,10 @@ #include net/sock.h
* in any case.
*/
-int verify_iovec(struct msghdr *m, struct iovec *iov, char *address, int mode)
+ssize_t verify_iovec(struct msghdr *m, struct iovec *iov,
Re: [PATCH] Fix verify_iovec() to not allow overflow of iov_len values
* Sridhar Samudrala ([EMAIL PROTECTED]) wrote:
-int verify_iovec(struct msghdr *m, struct iovec *iov, char *address, int
mode)
+ssize_t verify_iovec(struct msghdr *m, struct iovec *iov, char *address, int
mode)
{
int size, err, ct;
+ ssize_t tot_len = 0;
if (m-msg_namelen) {
if (mode == VERIFY_READ) {
@@ -61,17 +62,22 @@ int verify_iovec(struct msghdr *m, struc
err = 0;
for (ct = 0; ct m-msg_iovlen; ct++) {
- err += iov[ct].iov_len;
+ ssize_t len;
+
/*
- * Goal is not to verify user data, but to prevent returning
- * negative value, which is interpreted as errno.
- * Overflow is still possible, but it is harmless.
+ * Goal is not to verify user data, but to prevent the cases
+ * where an iov_len value or the sum of all iov_len values
+ * overflows ssize_t.
*/
- if (err 0)
- return -EMSGSIZE;
+ len = (ssize_t)iov[ct].iov_len;
+ if (len 0)
+ return -EINVAL;
+ tot_len += len;
+ if (tot_len 0)
I specifically used size_t here, because signed integer overflow is not
defined in C.
thanks,
-chris
-
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: [PATCH] Fix verify_iovec() to not allow overflow of iov_len values
On Tue, 2006-08-29 at 11:20 -0700, Chris Wright wrote:
* Sridhar Samudrala ([EMAIL PROTECTED]) wrote:
-int verify_iovec(struct msghdr *m, struct iovec *iov, char *address, int
mode)
+ssize_t verify_iovec(struct msghdr *m, struct iovec *iov, char *address,
int mode)
{
int size, err, ct;
+ ssize_t tot_len = 0;
if (m-msg_namelen) {
if (mode == VERIFY_READ) {
@@ -61,17 +62,22 @@ int verify_iovec(struct msghdr *m, struc
err = 0;
for (ct = 0; ct m-msg_iovlen; ct++) {
- err += iov[ct].iov_len;
+ ssize_t len;
+
/*
-* Goal is not to verify user data, but to prevent returning
-* negative value, which is interpreted as errno.
-* Overflow is still possible, but it is harmless.
+* Goal is not to verify user data, but to prevent the cases
+* where an iov_len value or the sum of all iov_len values
+* overflows ssize_t.
*/
- if (err 0)
- return -EMSGSIZE;
+ len = (ssize_t)iov[ct].iov_len;
+ if (len 0)
+ return -EINVAL;
+ tot_len += len;
+ if (tot_len 0)
I specifically used size_t here, because signed integer overflow is not
defined in C.
Is this a problem even if we are adding only positive integers?
I tried some tests and i didn't see any problem.
For ex: adding 2 max positive integers 0x7fff gave a result of -2
which passes the test for overflow.
0x7fff + 0x7ff = -2
0x7fff + 1 = -2147483648
Thanks
Sridhar
thanks,
-chris
-
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
-
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: [PATCH] Fix verify_iovec() to not allow overflow of iov_len values
From: Sridhar Samudrala [EMAIL PROTECTED]
Date: Tue, 29 Aug 2006 10:55:29 -0700
verify_iovec() has the following piece of code that allows overflow
of iov_len values in an iovec.
for (ct = 0; ct m-msg_iovlen; ct++) {
err += iov[ct].iov_len;
/*
* Goal is not to verify user data, but to prevent returning
* negative value, which is interpreted as errno.
* Overflow is still possible, but it is harmless.
*/
if (err 0)
return -EMSGSIZE;
}
The comment specifically says that overflow is harmless, but I don't see any
valid reason to allow overflow.
Also, iov_len is of type size_t which is a 64-bit value on 64-bit systems,
but we store and return the overall iovec length in an int.
Just some notes so that it is understood the context in which
such changes are being proposed.
First of all, several memcpy() and copy_{to,from}_user()
implementations BUG trap on lengths which have bit 31 or higher set.
It always indicates a bug of some sort in the call chain, so behaving
as if we support larger than positive signed 32-bit lengths here is
probably asking for trouble.
Also, the protocol downcalls all return int, not long so it's even
impossible to report successful sending of larger than positive signed
32-bit lengths. Even kernel_sendmsg() and friends return int.
We can pretend that larger than positive signed 32-bit lengths in an
iovec will work, but they surely won't.
And since the sendmsg() man page was quoted:
If the message is too long to pass atomically through the underlying
protocol, the error EMSGSIZE is returned, and the message is not
trans$,1rp(B
mitted.
which matches perfectly with the fact that the protocol downcalls
return int not size_t nor ssize_t.
-
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
