netdev - Re: [Patch net v2] ip_gre: set dev->hard_header_len and dev->needed

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: <CA+FuTSe7QxHUJfyh3Wr3nb+dG=mrE5MCETCdSiVu9ZCXnFqAag@mail.gmail.com>
Date:   Sun, 11 Oct 2020 17:06:27 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Xie He <xie.he.0141@...il.com>
Cc:     Cong Wang <xiyou.wangcong@...il.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        syzbot <syzbot+4a2c52677a8a1aa283cb@...kaller.appspotmail.com>,
        William Tu <u9012063@...il.com>,
        Willem de Bruijn <willemdebruijn.kernel@...il.com>
Subject: Re: [Patch net v2] ip_gre: set dev->hard_header_len and
 dev->needed_headroom properly

On Sun, Oct 11, 2020 at 4:42 PM Xie He <xie.he.0141@...il.com> wrote:
>
> On Sun, Oct 11, 2020 at 12:11 PM Cong Wang <xiyou.wangcong@...il.com> wrote:
> >
> > GRE tunnel has its own header_ops, ipgre_header_ops, and sets it
> > conditionally. When it is set, it assumes the outer IP header is
> > already created before ipgre_xmit().
> >
> > This is not true when we send packets through a raw packet socket,
> > where L2 headers are supposed to be constructed by user. Packet
> > socket calls dev_validate_header() to validate the header. But
> > GRE tunnel does not set dev->hard_header_len, so that check can
> > be simply bypassed, therefore uninit memory could be passed down
> > to ipgre_xmit(). Similar for dev->needed_headroom.
> >
> > dev->hard_header_len is supposed to be the length of the header
> > created by dev->header_ops->create(), so it should be used whenever
> > header_ops is set, and dev->needed_headroom should be used when it
> > is not set.
>
> Hi, thanks for attempting to fix this tunnel. Are we still considering
> removing header_ops->create?
>
> As said in my email sent previously today, I want to remove
> header_ops->create because 1) this keeps the un-exposed headers of GRE
> devices consistent with those of GRETAP devices, and 2) I think the
> GRE header (and the headers before the GRE header) is not actually the
> L2 header of the tunnel (the Wikipedia page for "Generic Routing
> Encapsulation" doesn't consider this protocol to be at L2 either).
>
> I'm not sure if you still agree to remove header_ops->create. Do you
> still agree but think it'd be better to do that in a separate patch?
>
> Removing header_ops->create would simplify the fixing of the issue you
> are trying to fix, too, because that way we would no longer need to
> use header_ops or hard_header_len. Also, I'm worried that changing
> hard_header_len (or needed_headroom) in ipgre_link_update would have
> racing issues. If we remove header_ops, we no longer need to use
> hard_header_len and we can just set needed_headroom to the maximum
> value, so that we no longer need to update them in ipgre_link_update.

Our messages crossed.

It seems there are legacy expectations that sendto/recvfrom packet
sockets allow writing/reading the outer IP address, as of commit
6a5f44d7a048 ("[IPV4] ip_gre: sendto/recvfrom NBMA address"). That is
the express purpose of that commit.

The behavior is inconsistent with other tunnels, as you also point
out, and probably only rarely used if at all. I would love to get rid
of it, but given that we cannot be certain that it is unused, I'm afraid
that we have to continue to support this special case.