netdev - Re: [PATCH 1/2 net] ip_gre: add validation for csum

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

Date:   Wed, 1 Sep 2021 17:39:45 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Ido Schimmel <idosch@...sch.org>
Cc:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        Shreyansh Chouhan <chouhan.shreyansh630@...il.com>,
        davem@...emloft.net, yoshfuji@...ux-ipv6.org, dsahern@...nel.org,
        kuba@...nel.org, pshelar@...ira.com, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        syzbot+ff8e1b9f2f36481e2efc@...kaller.appspotmail.com,
        Edward Cree <ecree@...arflare.com>
Subject: Re: [PATCH 1/2 net] ip_gre: add validation for csum_start

On Wed, Sep 1, 2021 at 11:53 AM Ido Schimmel <idosch@...sch.org> wrote:
>
> Thanks for the quick reply, Willem.
>
> On Wed, Sep 01, 2021 at 09:46:48AM -0400, Willem de Bruijn wrote:
> > Thanks for the detailed report, Ido.
> >
> > This is a gre tunnel device with csum/ocsum enabled, correct?
>
> Correct.
>
> >
> > How was this packet generated: does it come from the local stack or is
> > it a custom packet injected from userspace, e.g., with a packet socket
> > with vnet_hdr?
>
> The packet is received by a physical port and injected to the kernel's
> Rx path by mlxsw (which does not support checksumming). The IPv4 routing
> code then forwards the packet to the GRE tunnel.
>
> I was able to reproduce the issue using veth pairs and a packet socket
> [1]. Running the reproducer with the debug patch from before, I get the
> following output [2].

Thanks for that device independent repro.

As expected, the following fixes it for these packets:

-       if (csum && skb_checksum_start(skb) < skb->data)
+       if (csum && skb->ip_summed == CHECKSUM_PARTIAL &&
+           skb_checksum_start(skb) < skb->data)

The question is whether we're doing the right thing when CHECKSUM_PARTIAL
is set.

Local checksum offload allows for cheap calculation of outer checksums, by
relying on the fact that the inner packet with the checksum field filled in will
sum to zero. It relies on checksum offload to compute this inner checksum,
so expects csum_start and csum_off to point after the GRE header.

If so, then the existing fix won't break correctly configured skbs as it only
drops packets for which this does not hold.