netdev - Re: ipv6: Fix tcp_v6_send

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] [thread-next>] [day] [month] [year] [list]

Message-Id: <20100421.142841.191672280.davem@davemloft.net>
Date:	Wed, 21 Apr 2010 14:28:41 -0700 (PDT)
From:	David Miller <davem@...emloft.net>
To:	herbert@...dor.apana.org.au
Cc:	netdev@...r.kernel.org, cratiu@...acom.com
Subject: Re: ipv6: Fix tcp_v6_send_response checksum

From: Herbert Xu <herbert@...dor.apana.org.au>
Date: Wed, 21 Apr 2010 21:09:20 +0800

> On Wed, Apr 21, 2010 at 01:58:23AM -0700, David Miller wrote:
>> 
>> If we're going to use CHECKSUM_PARTIAL for these things (which we are
>> since commit 2e8e18ef52e7dd1af0a3bd1f7d990a1d0b249586 "tcp: Set
>> CHECKSUM_UNNECESSARY in tcp_init_nondata_skb"), we can't be setting
>> buff->csum as we always have been here in tcp_v6_send_response.  We
>> need to leave it at zero.
>> 
>> Kill that line and checksums are good again.
>> 
>> Signed-off-by: David S. Miller <davem@...emloft.net>
> 
> That line is needed for the CHECKSUM_NONE case.  In fact, if we're
> in the CHECKSUM_PARTIAL case, then that buff->csum calculation
> should be a noop because it immediately gets overwritten when we
> subsequently set csum_start/csum_offset.

Ok, but the checksums were broken.  Can you see why?

I think we changed semantics here when we did this:

-	t1->check = csum_ipv6_magic(&fl.fl6_src, &fl.fl6_dst,
-				    tot_len, IPPROTO_TCP,
-				    buff->csum);
+	__tcp_v6_send_check(buff, &fl.fl6_src, &fl.fl6_dst);

which changes what ->csum needs to be.  It used to need to be computed
over the header as well as any data afterwards, but now it has to
cover only data.  Because now it evaluates to:

		th->check = tcp_v6_check(skb->len, saddr, daddr,
					 csum_partial(th, th->doff << 2,
						      skb->csum));

See?  We were computing the checksum over the TCP header twice now :-)
That's why my patch fixed things for dataless packets, whose
->csum would evaluate to zero.

So to make things work fully as-is, we would need to compute ->csum
over the post-header data only.

But an even easier change is to just be consistent with the rest
of the TCP packet generation code and use CHECKSUM_PARTIAL here.
And that's how I'll fix this.

What confused me here last night is the fact that CHECKSUM_NONE
is zero and the implicit zero initialization of new SKBs givs us
that :-)

Anyways, thanks for pointing this out!
--
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