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]

Date:   Sun, 11 Oct 2020 13:32:35 -0700
From:   Xie He <xie.he.0141@...il.com>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     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 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.