Message-ID: <CALx6S35VXGfCeYtXm9z2APgt=kAwR2qJ5v7psN+Pg17LcDKasw@mail.gmail.com>
Date:	Mon, 28 Sep 2015 12:13:07 -0700
From:	Tom Herbert <tom@...bertland.com>
To:	David Woodhouse <dwmw2@...radead.org>
Cc:	Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [RFC PATCH] Fix false positives in can_checksum_protocol()

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

Powered by blists - more mailing lists