On Mon, 2015-09-28 at 10:03 -0700, Tom Herbert wrote: > > + if ((((features & NETIF_F_V4_CSUM) && protocol == htons(ETH_P_IP)) > > || > > + ((features & NETIF_F_V6_CSUM) && protocol == > > htons(ETH_P_IPV6))) && > > + (sk_protocol == IPPROTO_TCP || sk_protocol == IPPROTO_UDP)) > > + return 1; > > + > Relying on skb->sk->sk_protocol is problematic. This is making the > assumption that the checksum being offloaded for the packet is the > same as that of the protocol for the socket-- this may not be the > case when we are offloading an outer checksum in encapsulation.
> Currently this wouldn't a be problem since we're probably only > offloading outer UDP checksums, but if we ever start trying to > offload other outer checksums (e.g. GRE) then this probably doesn't > work so well. That makes sense. > 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? > 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(). > 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. 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(). 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
smime.p7s
Description: S/MIME cryptographic signature
