quoted

 Also, this doesn't help those drivers that that can offload TCP and
UDP for IPv6 but only if there are no extension headers, in those
case the driver needs to look at the packet to see if it is a
"simple" UDP/TCP packet.

Hm, are such devices even permitted to set NETIF_F_IPV6_CSUM?

Apparently this may be a problem in ixgbe. See "[net-next 05/19]
ixgbe: Add support for UDP-encapsulated tx checksum offload" thread.

quoted

AFAIK, the only non UDP/TCP transport IP checksum in the stack is GRE
checksum which as I pointed out we don't attempt to offload. So the
only way to trip the bug that you are seeing is probably through a
userspace packet interface like in the test code. I think this
actually might expose a much more serious issue. Looking at tun.c, I
don't see anything that validates that the csum_start and csum_offset
provided by userspace actually refers to a sane checksum offset.

That's handled in skb_partial_csum_set().

That only checks that start and offset are within skb_headlen. It
doesn't check that checksum offset refers to TCP/UDP/GRE/ICMP
checksum, or whether to the first two bytes of the IP destination
address. Maybe there's something later in the path that would catch
this, but I didn't readily see it.

quoted

Not only is this a way to ask the stack to perform checksums for non
TCP/UDP, but it actually seems like the interface could be used by a
malicious application to have a device arbitrarily overwrite two
bytes anywhere in the packet with it's own data far below the stack,
netfilter, routing. To really fix this we should probably be doing
validation in tun, if the checksum isn't for TCP or UDP then call
skb_checksum_help before sending the packet into the stack.

So... if it's never valid to ask for a hardware checksum on anything
but TCP or UDP, why do we bother with NETIF_F_GEN_CSUM at all? Should
we just be removing it entirely? That seems like something of a
retrograde step.

No, we want to do the opposite! In your example the request to
checksum is being generated from outside the stack so we need to
verify that for sanity-- requests generated by the stack would be
trusted. Presumably, within the stack we want a generic checksum
offload for new protocols, new extension headers (I am almost certain
that segment routing exthdr would break some NETIF_F_IPV6_CSUM), and
new flavors of encapsulation. NETIF_F_IP_CSUM and NETIF_F_IPV6_CSUM
are not generic and are becoming impediments to protocol development--
drivers moving to NETIF_F_HW_CSUM is the answer.

Perhaps a better solution would be a bit in the skbuff which indicates
that it *is* a TCP or UDP checksum. That would be set by our UDP and
TCP sockets, cleared by encapsulation, also set if appropriate by
skb_partial_csum_set().

Yes I agree. What I have been thinking to do is steal two bits from
csum_offset that would indicate that the checksum is IPv4 or IPv6
(specifically that the checksum value is seeded with an IPv4 or IPv6
pseudo header). This information plus the csum_offset would be
sufficient for drivers to identify the checksum as UDP/TCP-IPv4/IPv6.
The other case that needs special handling is inner vs. outer
checksum, but that can be deduced by comparing (inner of outer)
transport offset to checksum start. With this and a couple of utility
functions we should be able to start deprecating NETIF_F_IP_CSUM and
NETIF_F_IPV6_CSUM.

Thanks,
Tom

And then the check in can_checksum_protocol() is trivial and clearly
correct.

--
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation