>> 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.
>> 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. >> 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.woodho...@intel.com Intel Corporation > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
