netdev - veth regression with "don’t modify ip_summed; doing so treats packets with bad checksums as good."

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-next>] [day] [month] [year] [list]

Message-ID: <56F463D6.7080406@candelatech.com>
Date:	Thu, 24 Mar 2016 15:01:58 -0700
From:	Ben Greear <greearb@...delatech.com>
To:	netdev <netdev@...r.kernel.org>, ej@...njones.ca, vijayp@...ayp.ca
Cc:	Cong Wang <cwang@...pensource.com>
Subject: veth regression with "don’t modify ip_summed; doing so treats packets with bad checksums as good."

I have an application that creates two pairs of veth devices.

a <-> b       c <-> d

b and c have a raw packet socket opened on them and I 'bridge' frames
between b and c to provide network emulation (ie, configurable delay).

I put IP 1.1.1.1/24 on a, 1.1.1.2/24 on d, and then create a UDP connection
(using policy based routing to ensure frames are sent on the appropriate
interfaces).

This is user-space only app, and kernel in this case is completely unmodified.

The commit below breaks this feature:  UDP frames are sniffed on both a and d ports
(in both directions), but the UDP socket does not receive frames.

Using normal ethernet ports, this network emulation feature works fine, so it is
specific to VETH.

A similar test with just sending UDP between a single veth pair:  e <-> f
works fine.  Maybe it has something to do with raw packets?

The patch below is the culprit:

[greearb@...-dt3 linux-2.6]$ git bisect bad
ce8c839b74e3017996fad4e1b7ba2e2625ede82f is the first bad commit
commit ce8c839b74e3017996fad4e1b7ba2e2625ede82f
Author: Vijay Pandurangan <vijayp@...ayp.ca>
Date:   Fri Dec 18 14:34:59 2015 -0500

     veth: don’t modify ip_summed; doing so treats packets with bad checksums as good.

     Packets that arrive from real hardware devices have ip_summed ==
     CHECKSUM_UNNECESSARY if the hardware verified the checksums, or
     CHECKSUM_NONE if the packet is bad or it was unable to verify it. The
     current version of veth will replace CHECKSUM_NONE with
     CHECKSUM_UNNECESSARY, which causes corrupt packets routed from hardware to
     a veth device to be delivered to the application. This caused applications
     at Twitter to receive corrupt data when network hardware was corrupting
     packets.

...

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 0ef4a5a..ba21d07 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -117,12 +117,6 @@ static netdev_tx_t veth_xmit(struct sk_buff *skb, struct net_device *dev)
                 kfree_skb(skb);
                 goto drop;
         }
-       /* don't change ip_summed == CHECKSUM_PARTIAL, as that
-        * will cause bad checksum on forwarded packets
-        */
-       if (skb->ip_summed == CHECKSUM_NONE &&
-           rcv->features & NETIF_F_RXCSUM)
-               skb->ip_summed = CHECKSUM_UNNECESSARY;

         if (likely(dev_forward_skb(rcv, skb) == NET_RX_SUCCESS)) {
                 struct pcpu_vstats *stats = this_cpu_ptr(dev->vstats);

Any suggestions for how to fix this so that I get the old working behaviour and
the bug this patch was trying to fix is also resolved?

Thanks,
Ben

-- 
Ben Greear <greearb@...delatech.com>
Candela Technologies Inc  http://www.candelatech.com