Patchwork [Bugme-new,Bug,16603] New: send of data > 4 GB fails on 64 bit systems

login
register
mail settings
Submitter David Miller
Date Sept. 28, 2010, 3:24 a.m.
Message ID <20100927.202425.71120040.davem@davemloft.net>
Download mbox | patch
Permalink /patch/65933/
State Accepted
Delegated to: David Miller
Headers show

Comments

David Miller - Sept. 28, 2010, 3:24 a.m.
Ok, I suspect the following is enough to fix this specific bug
report, TCP sending.

However, as I stated in my previous reply there are creepy
crawlies all over the place.

For example, all of the routines in net/core/iovec.c (memcpy_toiovec,
memcpy_toiovecend, memcpy_fromiovec, memcpy_fromiovecend,
csum_partial_copy_fromiovecend) that cast using min_t() are currently
casting "down" to "unsigned int".

They should probably case "up" to "size_t".

Otherwise 4GB iov_len's on 64-bit will be truncated to zero just
as they do in the TCP path being fixed here.

If, alterntatively, we want to try and take the size_t values all
the way down the code paths to the individual copies, that is
a huge undertaking.

It's huge because we end up getting to the architecture specific
csum_copy_*() routines, which all take 'int' as the length and
many are written in assembler and would need audits and potentially
changes before we can make the type 'long' or 'size_t'.

Anyways, this part here is simple enough and I'll push it to
Linus and -stable.

--------------------
tcp: Fix >4GB writes on 64-bit.

Fixes kernel bugzilla #16603

tcp_sendmsg() truncates iov_len to an 'int' which a 4GB write to write
zero bytes, for example.

There is also the problem higher up of how verify_iovec() works.  It
wants to prevent the total length from looking like an error return
value.

However it does this using 'int', but syscalls return 'long' (and
thus signed 64-bit on 64-bit machines).  So it could trigger
false-positives on 64-bit as written.  So fix it to use 'long'.

Reported-by: Olaf Bonorden <bono@onlinehome.de>
Reported-by: Daniel Büse <dbuese@gmx.de>
Reported-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: David S. Miller <davem@davemloft.net>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andrew Morton - Sept. 28, 2010, 3:56 a.m.
On Mon, 27 Sep 2010 20:24:25 -0700 (PDT) David Miller <davem@davemloft.net> wrote:

> Ok, I suspect the following is enough to fix this specific bug
> report, TCP sending.
> 
> However, as I stated in my previous reply there are creepy
> crawlies all over the place.
> 
> For example, all of the routines in net/core/iovec.c (memcpy_toiovec,
> memcpy_toiovecend, memcpy_fromiovec, memcpy_fromiovecend,
> csum_partial_copy_fromiovecend) that cast using min_t() are currently
> casting "down" to "unsigned int".
> 
> They should probably case "up" to "size_t".

eep.

A blanket suckyfix might be, at the syscall level:

	if (size > 4g) {
		do_it_in_4g_hunks();
		do_the_last_bit();
	}

unless that would break some networking syscall->framesize guarantees
or something?

And such a "fix" would make it hard to test the real fix!


I'm surprised that this issue hasn't come up before.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Sept. 28, 2010, 4:07 a.m.
From: Andrew Morton <akpm@linux-foundation.org>
Date: Mon, 27 Sep 2010 20:56:24 -0700

> A blanket suckyfix might be, at the syscall level:
> 
> 	if (size > 4g) {
> 		do_it_in_4g_hunks();
> 		do_the_last_bit();
> 	}
> 
> unless that would break some networking syscall->framesize guarantees
> or something?

There is not a single length passed in, but a vector of them, that's
what the iovec conveys.

Even if you could, socket send calls have atomicity guarentees.  For a
datagram socket, for example, a single send call corresponds to one
packet on the network.

BTW, this aspect of datagram sockets is a part of the reason we don't
see many reports about this stuff. :-)  Only stream protocols can
really take such enormous lengths, and the most popular (TCP) does
much of the iovec handling by hand.

We just need to fix our junk, and the {min,max}_t(size_t, ...) change
I suggested is likely the easiest path.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/linux/socket.h b/include/linux/socket.h
index a2fada9..a8f56e1 100644
--- a/include/linux/socket.h
+++ b/include/linux/socket.h
@@ -322,7 +322,7 @@  extern int csum_partial_copy_fromiovecend(unsigned char *kdata,
 					  int offset, 
 					  unsigned int len, __wsum *csump);
 
-extern int verify_iovec(struct msghdr *m, struct iovec *iov, struct sockaddr *address, int mode);
+extern long verify_iovec(struct msghdr *m, struct iovec *iov, struct sockaddr *address, int mode);
 extern int memcpy_toiovec(struct iovec *v, unsigned char *kdata, int len);
 extern int memcpy_toiovecend(const struct iovec *v, unsigned char *kdata,
 			     int offset, int len);
diff --git a/net/core/iovec.c b/net/core/iovec.c
index 1cd98df..e6b133b 100644
--- a/net/core/iovec.c
+++ b/net/core/iovec.c
@@ -35,9 +35,10 @@ 
  *	in any case.
  */
 
-int verify_iovec(struct msghdr *m, struct iovec *iov, struct sockaddr *address, int mode)
+long verify_iovec(struct msghdr *m, struct iovec *iov, struct sockaddr *address, int mode)
 {
-	int size, err, ct;
+	int size, ct;
+	long err;
 
 	if (m->msg_namelen) {
 		if (mode == VERIFY_READ) {
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 95d75d4..f115ea6 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -943,7 +943,7 @@  int tcp_sendmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
 	sg = sk->sk_route_caps & NETIF_F_SG;
 
 	while (--iovlen >= 0) {
-		int seglen = iov->iov_len;
+		size_t seglen = iov->iov_len;
 		unsigned char __user *from = iov->iov_base;
 
 		iov++;