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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAF=yD-JSwz0FvB2RAZ34_8T8CjPLAW=ut0RyTKWJGnGQMjOpUg@mail.gmail.com>
Date:   Mon, 14 Jan 2019 10:38:55 -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 Mon, Jan 14, 2019 at 10:15 AM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> On Mon, Jan 14, 2019 at 9:51 AM Nicolas Dichtel
> <nicolas.dichtel@...nd.com> wrote:
> >
> > Le 12/01/2019 à 19:00, Willem de Bruijn a écrit :
> > > On Fri, Jan 11, 2019 at 4:29 PM Willem de Bruijn
> > > <willemdebruijn.kernel@...il.com> wrote:
> > >>
> > >> On Fri, Jan 11, 2019 at 9:44 AM Nicolas Dichtel
> > >> <nicolas.dichtel@...nd.com> wrote:
> > >>>
> > >>> Since commit cb9f1b783850, scapy (which uses an AF_PACKET socket in
> > >>> SOCK_RAW mode) is unable to send a basic icmp packet over a sit tunnel:
> > >>>
> > >>> Here is a example of the setup:
> > >>> $ ip link set ntfp2 up
> > >>> $ ip addr add 10.125.0.1/24 dev ntfp2
> > >>> $ ip tunnel add tun1 mode sit ttl 64 local 10.125.0.1 remote 10.125.0.2 dev ntfp2
> > >>> $ ip addr add fd00:cafe:cafe::1/128 dev tun1
> > >>> $ ip link set dev tun1 up
> > >>> $ ip route add fd00:200::/64 dev tun1
> > >>> $ scapy
> > >>>>>> p = []
> > >>>>>> p += IPv6(src='fd00:100::1', dst='fd00:200::1')/ICMPv6EchoRequest()
> > >>>>>> send(p, count=1, inter=0.1)
> > >>>>>> quit()
> > >>> $ ip -s link ls dev tun1 | grep -A1 "TX.*errors"
> > >>>     TX: bytes  packets  errors  dropped carrier collsns
> > >>>     0          0        1       0       0       0
> > >>>
> > >>> The problem is that the network offset is set to the hard_header_len of the
> > >>> output device (tun1, ie 14 + 20) and in our case, because the packet is
> > >>> small (48 bytes) the pskb_inet_may_pull() fails (it tries to pull 40 bytes
> > >>> (ipv6 header) starting from the network offset). Let's reset the network
> > >>> offset so that it points to the beginning of the L3 data, ie skb->data.
> > >>
> > >> This only just landed in my inbox, sorry for the delay. So far I'm not
> > >> reproducing the issue, but I'm trying and am having a look.
> > >>
> > >> Which pskb_inet_may_pull() is failing?
> > >
> > > Okay. I see what's going on.
> > >
> > > Commit cb9f1b783850 converted a pskb_may_pull ipv6hdr, pulling from
> > > skb->data, to pskb_network_may_pull, pulling from skb->network_header.
> > >
> > > Normally packet sockets with SOCK_RAW write a fixed size hardware
> > > header and place skb->network_header directly after that. Dropping
> > > these in sit_tunnel_xmit if they fail pskb_inet_may_pull is correct.
> > >
> > > But with devices with variable length hardware headers like sit, it is
> > > possible to cook packets that are shorter than the upper bound hhlen.
> > > In that case, we have no way of knowing where the variable length
> > > header ends (short of device specific parsing if implemented, see also
> > > header_ops.validate), so we set the network header to the same as the
> > > mac header as of commit 993675a3100b1.
> > >
> > > This is not a good thing to do in general, and not needed in the
> > > common case.
> > Sorry, but I fail to understand what we try to achieve here. In this case, the
> > packet socket builds a packet without a hard header:
> >
> > <-------48B--------->
> > <--40B--->
> > +--------+----------+
> > |IPv6 hdr|ICMPv6 hdr|
> > +--------+----------+
> > ^ skb->data
> >       ^ nh offset: 34 (ie dev->hard_header_len, ie ethernet hdr + ipv4 hdr)
> >
> > The nh offset does not match any data in the packet.
> >
> > dev_validate_header() checks that datalen (ie 48) is >= dev->hard_header_len (ie
> > 34). Still no hard header here.
> >
> > Then, dev->xmit (ie sit_tunnel_xmit()) is called. This function calls
> > pskb_inet_may_pull() which tries to pull 40B from the nh offset (34). But the nh
> > offset still points somewhere in the ipv6 hdr, thus the test fails (34 + 40 > 48).
> > I don't understand why it is wrong to set the nh offset to 0 (skb->data), ie
> > exactly where the ipv6 header starts.
>
> 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.
>
> 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)