netdev - Re: [PATCH net] ip_gre: validate csum_start only if CHECKSUM

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]

Message-ID: <CA+FuTSe+vJgTVLc9SojGuN-f9YQ+xWLPKE_S4f=f+w+_P2hgUg@mail.gmail.com>
Date:   Sun, 5 Sep 2021 23:02:23 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Alexander Duyck <alexander.duyck@...il.com>
Cc:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        Netdev <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Ido Schimmel <idosch@...sch.org>,
        chouhan.shreyansh630@...il.com
Subject: Re: [PATCH net] ip_gre: validate csum_start only if CHECKSUM_PARTIAL

On Sun, Sep 5, 2021 at 11:53 AM Alexander Duyck
<alexander.duyck@...il.com> wrote:
>
> On Sun, Sep 5, 2021 at 8:24 AM Willem de Bruijn
> <willemdebruijn.kernel@...il.com> wrote:
> >
> > On Sat, Sep 4, 2021 at 7:47 PM Alexander Duyck
> > <alexander.duyck@...il.com> wrote:
>
> <snip>
>
> > > You can do it since you have essentially already written half the code.. :)
> >
> > Sent, but only the ipv4 patch.
> >
> > I actually do not see an equivalent skb_pull path in ip6_gre.c. Will
> > take a closer look later, but don't have time for that now.
> >
> > https://patchwork.kernel.org/project/netdevbpf/patch/20210905152109.1805619-1-willemdebruijn.kernel@gmail.com/
>
> So does that mean that commit 9cf448c200ba ("ip6_gre: add validation
> for csum_start") is adding overhead that doesn't really address
> anything, and is introducing false positives? If so, should we just
> revert it?

I agree.

I didn't immediately understand how there can be an ip6gre_header
analogous to ipgre_header (for headers_ops.create), but no skb_pull
in ip6gre_tunnel_xmit equivalent to the one in ipgre_xmit.

One difference between the two of them is the return value, which
returns an offset: ip6gre_header returns t->hlen. ipgre_header returns
t->hlen + sizeof(*iph). This extra offset was introduced in commit
77a482bdb2e6 ("ip_gre: fix ipgre_header to return correct offset") as
a fix to previous commit c544193214 ("GRE: Refactor GRE tunneling
code."). Before the latter commit the IPv4 header was included in
t->hlen, similar to IPv6. After that commit it is no longer. The latter
patch is also the one that introduces the skb_pull in ipgre_xmit.

I can't say that I fully understand how this works for IPv6.

But I think that is sufficient to understand that this skb_pull issue
does not affect the ip6_gre path and we best just revert that commit.