netdev - Re: [PATCH net] af_packet: fix raw sockets over 6in4 tunnel

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:   Tue, 15 Jan 2019 11:20:52 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Nicolas Dichtel <nicolas.dichtel@...nd.com>
Cc:     David Miller <davem@...emloft.net>,
        Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH net] af_packet: fix raw sockets over 6in4 tunnel

On Tue, Jan 15, 2019 at 4:49 AM Nicolas Dichtel
<nicolas.dichtel@...nd.com> wrote:
>
> Le 14/01/2019 à 16:38, Willem de Bruijn a écrit :
> > On Mon, Jan 14, 2019 at 10:15 AM Willem de Bruijn
> > <willemdebruijn.kernel@...il.com> wrote:
> [snip]
> >> It is wrong because for other devices l2 header length is not zero, so
> >> this incorrectly sets skb->network_header to the start of the link
> >> layer header on all those devices.
> Ok, thank you for the details.
>
> >>
> >> A one line variant of the above would be
> >>
> >> -               if (len < reserve)
> >> +               if (len < reserve + sizeof(struct ipv6hdr))
> >
> > and exclude the majority of devices with fixed hard header len. Those
> > require len to be >= reserve, so this workaround does not apply to
> > them:
> >
> > if (len < reserve + sizeof(struct ipv6hdr) &&
> >     dev->min_header_len != dev->hard_header_len)
> >
> And what about:
> - if (len < reserve)
> + if (dev->min_header_len != dev->hard_header_len)
>         skb_reset_network_header(skb);
> ?

As a fix for the regression introduced by cb9f1b783850 ("ip: validate
header length on virtual device xmit"), the test I proposed is a bit
more narrow by only applying this ugly workaround in cases where that
test might have started failing.

By limiting to short packets we also avoid in the common case reading
dev->min_header_len, which may not be cached (btw, we should use
reserve instead of dev->hard_header_len for the same reason).

But the length restriction does look rather arbitrary, so there is
something to say for your suggestion to apply this uniformly to
devices with variable length.

Note also the concurrent discussion in
http://patchwork.ozlabs.org/patch/1024489/ about extending header_ops
with a hard header parser which may return the exist header length.
That is only for net-next, but it will allow us to set
skb->network_header correctly even for these devices where
(dev->min_header_len != dev->hard_header_len).