netdev - Re: [RFC PATCH] Fix false positives in can_checksum

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1443464822.4674.109.camel@infradead.org>
Date:	Mon, 28 Sep 2015 19:27:02 +0100
From:	David Woodhouse <dwmw2@...radead.org>
To:	Tom Herbert <tom@...bertland.com>
Cc:	Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [RFC PATCH] Fix false positives in can_checksum_protocol()

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


Download attachment "smime.p7s" of type "application/x-pkcs7-signature" (5691 bytes)