netdev - Re: Oops in 2.6.22.1: skb_copy_and_csum_datagram

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20070905205554.GA17326@gondor.apana.org.au>
Date:	Thu, 6 Sep 2007 04:55:54 +0800
From:	Herbert Xu <herbert@...dor.apana.org.au>
To:	Neil Brown <neilb@...e.de>, "David S. Miller" <davem@...emloft.net>
Cc:	Chuck Ebbert <cebbert@...hat.com>, netdev@...r.kernel.org
Subject: Re: Oops in 2.6.22.1: skb_copy_and_csum_datagram_iovec()

Hi Neil:

On Wed, Sep 05, 2007 at 01:50:21PM +0100, Neil Brown wrote:
>
> > iov == NULL used to work.

Well it used to work mostly.  In fact, it still does work
mostly too.

> > I think it stopped working at 
> >    commit 759e5d006462d53fb708daa8284b4ad909415da1
> > 
> > Previously, as len==0, MSG_TRUNC would get set, so copy_only would get
> > set, so skb_copy_datagram_iovec would get called, and that handles a
> > len of 0.
> > 
> > Now, skb_copy_and_csum_datagram_iovec gets called unless
> > skb_csum_unnecessary(skb), which now kills us.

Not quite.  If MSG_TRUNC is set, then we'll just call
udp_lib_checksum_complete.  If it fails we'll bail, if
it succeeds then skb_csum_unnecessary(skb) is guaranteed
to be true.

So in this respect the new code behaves identically with
the old code.

> However earlier there is:
> 	ulen = skb->len - sizeof(struct udphdr);
> 	copied = len;
> 	if (copied > ulen)
> 		copied = ulen;
> 
> so if the 'len' (of the iovec) is too small, we end up with "copied == ulen", 

Nope, if len (== copied) is too small, we simply set MSG_TRUNC
as we did before.

Where this all falls apart is when the UDP payload is
empty (ulen == 0).  In that case MSG_TRUNC is not set
and we end up calling skb_copy_and_csum_datagram_iovec
which oopses.  I've looked up the original crash and
indeed %edi there which corresponds to the UDP payload
length is zero.

However, I can see where you're coming from in that we
shouldn't dereference msg_iov at all if msg_iovlen is 0.
This never happens with sys_recvmsg so we never bothered
checking it.  The following patch should fix the problem.

[NET]: Do not dereference iov if length is zero

When msg_iovlen is zero we shouldn't try to dereference
msg_iov.  Right now the only thing that tries to do so
is skb_copy_and_csum_datagram_iovec.  Since the total
length should also be zero if msg_iovlen is zero, it's
sufficient to check the total length there and simply
return if it's zero.

Signed-off-by: Herbert Xu <herbert@...dor.apana.org.au>

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@...dor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
diff --git a/net/core/datagram.c b/net/core/datagram.c
--- a/net/core/datagram.c
+++ b/net/core/datagram.c
@@ -450,6 +450,9 @@ int skb_copy_and_csum_datagram_iovec(str
 	__wsum csum;
 	int chunk = skb->len - hlen;

+	if (!chunk)
+		return 0;
+
 	/* Skip filled elements.
 	 * Pretty silly, look at memcpy_toiovec, though 8)
 	 */
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html