[net-next PATCH] IPv6: Use a 16 bit length field when computing a IPv6 UDP checksum

[net-next PATCH] IPv6: Use a 16 bit length field when computing a IPv6 UDP checksum

[Alexander Duyck]

This change makes it so that we only use a 16 bit length field instead of a
32 bit length field when computing a UDP checksum for IPv6.

This fixes an issue found with UDP tunnels over IPv6 where the total size
exceeded 65536 for a frame that was to be segmented.  As a result the
checksum being computed didn't match the frame data so we ended up being
off by 1 for the final checksum value since we didn't cancel out the upper
16 bits of the length.

The reasoning behind this is that RFC2460 states that for protocols such as
UDP that carry their own length field we should use that when computing the
checksum for the pseudo-header.  As such we should be using a 16 bit value,
not a 32 bit as is currently occurring when computing the UDP checksum for
IPv6.

Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
---

I don't have the highest of confidence that this patch is the correct
solution for this, however the way I see it we only have a few
alternatives.  For now I am using this patch to get the conversation
started.

There ends up really being two issues here.

The first issue is that IPv4 and IPv6 GSO frames are being assembled that
have a length that is greater than 65535 and the truncated value is being
placed in the length fields.  The problem as I see it is that I am not sure
there is any definitive way to prevent this without reducing the maximum
GSO size for all sources within a given network namespace.  In addition I
am not sure what the impact on performance would be if we were to implement
such an approach.

The second issue is that if we cannot prevent the first issue we really
need to have a consistent way of doing the IPv4 and IPv6 checksums.  The
IPv4 checksum passes the length value as a short, while the IPv6 passes
this value as an integer.  As a result we get different behavior between
the two which makes it difficult for us to put together a generic routine
for handling checksums for segmentation at higher levels.

 include/net/ip6_checksum.h |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/net/ip6_checksum.h b/include/net/ip6_checksum.h
index 1a49b73f7f6e..c55529ac12cf 100644
--- a/include/net/ip6_checksum.h
+++ b/include/net/ip6_checksum.h
@@ -95,6 +95,15 @@ static inline __sum16 udp_v6_check(int len,
 				   const struct in6_addr *daddr,
 				   __wsum base)
 {
+	/* RFC 2460 states that for protocols such as UDP which include
+	 * a length in their header we should use that value when computing
+	 * the pseudo-header instead of the total payload length minus
+	 * extension headers.  Since UDP has a length field that is only
+	 * 16 bits in length we need to cap the length field to 16 bits to
+	 * match what will be present in the header.
+	 */
+	len &= 0xFFFF;
+
 	return csum_ipv6_magic(saddr, daddr, len, IPPROTO_UDP, base);
 }
 

Re: [net-next PATCH] IPv6: Use a 16 bit length field when computing a IPv6 UDP checksum

[David Miller]

From: Alexander Duyck <aduyck@mirantis.com>
Date: Thu, 25 Feb 2016 19:10:59 -0800

> This change makes it so that we only use a 16 bit length field instead of a
> 32 bit length field when computing a UDP checksum for IPv6.
> 
> This fixes an issue found with UDP tunnels over IPv6 where the total size
> exceeded 65536 for a frame that was to be segmented.  As a result the
> checksum being computed didn't match the frame data so we ended up being
> off by 1 for the final checksum value since we didn't cancel out the upper
> 16 bits of the length.
> 
> The reasoning behind this is that RFC2460 states that for protocols such as
> UDP that carry their own length field we should use that when computing the
> checksum for the pseudo-header.  As such we should be using a 16 bit value,
> not a 32 bit as is currently occurring when computing the UDP checksum for
> IPv6.
> 
> Signed-off-by: Alexander Duyck <aduyck@mirantis.com>

What a can of worms. :-/

Reading RFC2460 over a few times, indeed using the truncated 16-bit length
is the thing to do for the pseudo-header checksum.

We have this mistake in a few places, for example ip6_compute_pseudo()
unconditionally uses skb->len, yet is used by UDP on receive.

Can you do a little audit and fix as many of these cases as you can find
and wrap them all into this patch?

Thanks!

Re: [net-next PATCH] IPv6: Use a 16 bit length field when computing a IPv6 UDP checksum

[Alexander Duyck]

On Tue, Mar 1, 2016 at 12:09 PM, David Miller <davem@davemloft.net> wrote:
> From: Alexander Duyck <aduyck@mirantis.com>
> Date: Thu, 25 Feb 2016 19:10:59 -0800
>
>> This change makes it so that we only use a 16 bit length field instead of a
>> 32 bit length field when computing a UDP checksum for IPv6.
>>
>> This fixes an issue found with UDP tunnels over IPv6 where the total size
>> exceeded 65536 for a frame that was to be segmented.  As a result the
>> checksum being computed didn't match the frame data so we ended up being
>> off by 1 for the final checksum value since we didn't cancel out the upper
>> 16 bits of the length.
>>
>> The reasoning behind this is that RFC2460 states that for protocols such as
>> UDP that carry their own length field we should use that when computing the
>> checksum for the pseudo-header.  As such we should be using a 16 bit value,
>> not a 32 bit as is currently occurring when computing the UDP checksum for
>> IPv6.
>>
>> Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
>
> What a can of worms. :-/
>
> Reading RFC2460 over a few times, indeed using the truncated 16-bit length
> is the thing to do for the pseudo-header checksum.
>
> We have this mistake in a few places, for example ip6_compute_pseudo()
> unconditionally uses skb->len, yet is used by UDP on receive.
>
> Can you do a little audit and fix as many of these cases as you can find
> and wrap them all into this patch?
>
> Thanks!

So I have been thinking about it and limiting the length to 16 bits
might restrict us in terms of ipv6 jumbograms (RFC 2675), though it
doesn't appear we support them but if we ever did then we might be
painted into a bit of a corner.  At the same time it isn't as if the
frames are exactly valid anyway since we have exceeded the 64K limit
without really taking any approach to resolve it as defined in RFC
2675.  In addition we have the same issue for IPv4 which isn't even
supposed to support frames larger than 64K.

I was wondering what your thoughts would be about widening the size of
the length field that we pass into csum_tcpudp_magic from a 16 bit to
a 24 or 32 bit value?  The general idea would be to shift tunnels over
to uniformly using skb->len instead of a mix of 16 bit or 32 bit
lengths.  My thought is it might add a bit of security since it would
invalidate the outer header checksum for the case where length has
exceeded 65535 resulting in uh->len field being invalid anyway.

- Alex

Re: [net-next PATCH] IPv6: Use a 16 bit length field when computing a IPv6 UDP checksum

[David Miller]

From: Alexander Duyck <alexander.duyck@gmail.com>
Date: Tue, 1 Mar 2016 13:19:28 -0800

> I was wondering what your thoughts would be about widening the size of
> the length field that we pass into csum_tcpudp_magic from a 16 bit to
> a 24 or 32 bit value?  The general idea would be to shift tunnels over
> to uniformly using skb->len instead of a mix of 16 bit or 32 bit
> lengths.  My thought is it might add a bit of security since it would
> invalidate the outer header checksum for the case where length has
> exceeded 65535 resulting in uh->len field being invalid anyway.

Hmmm, but wait, what is uh->len supposed to be for an ipv6 jumbogram
anyways?

It just gets truncated and the the ipv6 header payload length field
trumps whatever is in the UDP header length field right?

If that's what happens then we should uniformly use the truncated
length for the pseudo-header calculations as you originally suggested.

How UDP jumbograms as supposed to be handled wrt. udp_hdr->len should
guide our implementation.

Re: [net-next PATCH] IPv6: Use a 16 bit length field when computing a IPv6 UDP checksum

[Alexander Duyck]

On Tue, Mar 1, 2016 at 1:35 PM, David Miller <davem@davemloft.net> wrote:
> From: Alexander Duyck <alexander.duyck@gmail.com>
> Date: Tue, 1 Mar 2016 13:19:28 -0800
>
>> I was wondering what your thoughts would be about widening the size of
>> the length field that we pass into csum_tcpudp_magic from a 16 bit to
>> a 24 or 32 bit value?  The general idea would be to shift tunnels over
>> to uniformly using skb->len instead of a mix of 16 bit or 32 bit
>> lengths.  My thought is it might add a bit of security since it would
>> invalidate the outer header checksum for the case where length has
>> exceeded 65535 resulting in uh->len field being invalid anyway.
>
> Hmmm, but wait, what is uh->len supposed to be for an ipv6 jumbogram
> anyways?

In a true UDP jumbogram the value for uh->len and ipv6->paylen should
be 0 and the length should be derived based on the actual payload
length which is placed in an IPv6 hop-by-hop extension header with a
jumbo payload option present since paylen is only a 16 bit field in
the IPv6 header.

> It just gets truncated and the the ipv6 header payload length field
> trumps whatever is in the UDP header length field right?

Right.  The RFC 2675 says the value for uh->len should be set to 0 and
instead the total payload length should be used.

> If that's what happens then we should uniformly use the truncated
> length for the pseudo-header calculations as you originally suggested.
>
> How UDP jumbograms as supposed to be handled wrt. udp_hdr->len should
> guide our implementation.

That is kind of what I was thinking.  The only problem is I also have
to sort out IPv4 and IPv6 and hopefully do so in such a way that I
don't end up having to impact the drivers too much.  That is why I was
wondering if we could look at widening out the approach so that it
could be applied to IPv4 as well as IPv6.

One other thought I am having is that maybe I should take a look at
the true scope of this.  For example I know Windows allows for doing
TSO with frames larger than 64K.  Should we maybe start looking at
supporting something like that as well for GRO/GSO?

- Alex