netdev - Re: [PATCH net] ipv6: no CHECKSUM_PARTIAL on skbs with extension headers and recalc checksum during fragmentation

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: <1445885043.985660.420718105.55D9DB7D@webmail.messagingengine.com>
Date:	Mon, 26 Oct 2015 19:44:03 +0100
From:	Hannes Frederic Sowa <hannes@...essinduktion.org>
To:	Tom Herbert <tom@...bertland.com>
Cc:	Linux Kernel Network Developers <netdev@...r.kernel.org>,
	Eric Dumazet <edumazet@...gle.com>,
	Vlad Yasevich <vyasevich@...il.com>,
	Benjamin Coddington <bcodding@...hat.com>
Subject: Re: [PATCH net] ipv6: no CHECKSUM_PARTIAL on skbs with extension headers
 and recalc checksum during fragmentation



On Mon, Oct 26, 2015, at 15:19, Tom Herbert wrote:
> > We already concluded that drivers do have this problem and not the stack
> > above ip6_fragment. The places I am aware of I fixed in this patch. Also
> > IPv4 to me seems unaffected, albeit one can certainly clean up the logic
> > in net-next.
> >
> I don't understand why checksum for IP fragments is a driver problem.
> When fragments are sent to driver they should never have
> CHECKSUM_PARTIAL set (or maybe that is what you are seeing?).

Because either the drivers or the hardware does not correctly iterate
over the extension headers to fetch the final nexthdr field which is
used to compute the checksum. This is different from IPv4.

I can only guess e.g. from the e1000e driver:

        case cpu_to_be16(ETH_P_IPV6):
                /* XXX not handling all IPV6 headers */
                if (ipv6_hdr(skb)->nexthdr == IPPROTO_TCP)
                        cmd_len |= E1000_TXD_CMD_TCP;
                break;

> > Do you want to move the skb_checksum_help() check to the front of
> > ip_fragment in ipv4 now too?
> >
> Yes, it seems to me we should never fragment a packets with
> CHECKSUM_PARTIAL. This seems to currently be possible in IP output (v4
> & v6). I would imagine that we don't see this bug trip (too often?)
> because most uses of UDP got through the user space fragmentation
> code, and UDP packets sent through kernel path probably don't have a
> frag_list so they go through slow path.

I agree.

> Also, as Eric suggested, it looks like your patch is doing two things
> (fixing csum/fragmentation and disabling csum_partial for any
> extension headers)-- these should be separate patches.

I will now split the patches as a consensus seems reached here (and
write a commit message :) ). I audit the IPv4 paths, too.

Thanks,
Hannes
--
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