netdev - Re: 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>] [day] [month] [year] [list]

Message-ID: <56F470DF.2060002@candelatech.com>
Date:	Thu, 24 Mar 2016 15:57:35 -0700
From:	Ben Greear <greearb@...delatech.com>
To:	Vijay Pandurangan <vijayp@...ayp.ca>
Cc:	Cong Wang <cwang@...pensource.com>,
	Linux Kernel Network Developers <netdev@...r.kernel.org>,
	ej@...njones.ca
Subject: Re: veth regression with "don’t modify ip_summed; doing so treats packets with bad checksums as good."

On 03/24/2016 03:49 PM, Vijay Pandurangan wrote:
> Hmm that's troubling. We tested various UDP configurations but I don't think we tested this setup. Do you have code or more specific instructions we can use to
> replicate this bug?

The user-space app is not open source, and the routing setup is
a bit tricky as well.

I'm not certain it is specific to UDP, probably it is not.

I'm using sendmmsg to transmit frames on the packet socket, so
possibly something in that path is key.

Truth is, I can debug this with printk and see what is going
on if you have no immediate ideas what is going wrong.

Thanks,
Ben


>
> On Mar 24, 2016 6:02 PM, "Ben Greear" <greearb@...delatech.com <mailto:greearb@...delatech.com>> wrote:
>
>     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 <http://1.1.1.1/24> on a, 1.1.1.2/24 <http://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 <mailto: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 <mailto:greearb@...delatech.com>>
>     Candela Technologies Inc http://www.candelatech.com
>


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