Re: [RFC PATCH] Fix false positives in can_checksum_protocol()

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 2898 bytes --]

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.Woodhouse@intel.com                              Intel Corporation


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]