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

Message-ID: <CAM_iQpXpqVgweZFw438v_b7No8U8YtqHSt0x0+HZ+oSimdCyfg@mail.gmail.com>
Date:	Fri, 25 Mar 2016 09:32:23 -0700
From:	Cong Wang <xiyou.wangcong@...il.com>
To:	Ben Greear <greearb@...delatech.com>
Cc:	Vijay Pandurangan <vijayp@...ayp.ca>,
	netdev <netdev@...r.kernel.org>, Evan Jones <ej@...njones.ca>,
	Cong Wang <cwang@...pensource.com>,
	Tom Herbert <tom@...bertland.com>
Subject: Re: veth regression with "don’t modify ip_summed; doing so treats packets with bad checksums as good."

On Fri, Mar 25, 2016 at 9:10 AM, Ben Greear <greearb@...delatech.com> wrote:
> I am suspicious that this will break at least some drivers.  I grepped
> around
> for ip_summed, and found this, for instance:
>
> davicom/dm9000.c
>
>         /* The DM9000 is not smart enough to leave fragmented packets alone.
> */
>         if (dm->ip_summed != ip_summed) {
>                 if (ip_summed == CHECKSUM_NONE)
>                         iow(dm, DM9000_TCCR, 0);
>                 else
>                         iow(dm, DM9000_TCCR, TCCR_IP | TCCR_UDP | TCCR_TCP);
>                 dm->ip_summed = ip_summed;
>         }
>
>
> It is taking action based on ip_summed == CHECKSUM_NONE, and your change
> will probably break that.
>
> I would suggest that we try to make any fix specific only to veth,
> at least for now.  A tree-wide audit of drivers is probably required
> to safely make the kind of change you propose above.
>
> So, unless you can explain why your change is safe, then I do not plan
> to test it.

I just blindly trust the comments there:

 * CHECKSUM_UNNECESSARY:
 *
 *   This has the same meaning on as CHECKSUM_NONE for checksum offload on
 *   output.

Let's Cc Tom who wrote this comment.

On the other hand, hyperv got this correctly:

        if ((skb->ip_summed == CHECKSUM_NONE) ||
            (skb->ip_summed == CHECKSUM_UNNECESSARY))
                goto do_send;

So I believe dm9000 needs to fix.

Thanks.