Re: KASAN: use-after-free Read in ipv6_gso_pull_exthdrs

[Willem de Bruijn]

On Sun, Jul 8, 2018 at 6:58 PM Willem de Bruijn
<willemdebruijn.kernel@xxxxxxxxx> wrote:
>
> On Fri, Jul 6, 2018 at 6:16 PM Willem de Bruijn
> <willemdebruijn.kernel@xxxxxxxxx> wrote:
> >
> > On Fri, Jul 6, 2018 at 1:55 PM syzbot
> > <syzbot+7b9ed9872dab8c32305d@xxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
> > >
> > > syzbot has found a reproducer for the following crash on:
> > >
> > > HEAD commit:    70ba5b6db96f ipv4: Return EINVAL when ping_group_range sys..
> > > git tree:       net
> > > console output: https://syzkaller.appspot.com/x/log.txt?x=13cd2970400000
> > > kernel config:  https://syzkaller.appspot.com/x/.config?x=2ca6c7a31d407f86
> > > dashboard link: https://syzkaller.appspot.com/bug?extid=7b9ed9872dab8c32305d
> > > compiler:       gcc (GCC) 8.0.1 20180413 (experimental)
> > > syzkaller repro:https://syzkaller.appspot.com/x/repro.syz?x=15dfb748400000
> > > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=12a1050c400000
> > >
> > > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > > Reported-by: syzbot+7b9ed9872dab8c32305d@xxxxxxxxxxxxxxxxxxxxxxxxx
> > >
> > > IPv6: ADDRCONF(NETDEV_UP): veth0: link is not ready
> > > IPv6: ADDRCONF(NETDEV_CHANGE): veth0: link becomes ready
> > > IPv6: ADDRCONF(NETDEV_CHANGE): bond0: link becomes ready
> > > 8021q: adding VLAN 0 to HW filter on device team0
> > > ==================================================================
> > > BUG: KASAN: use-after-free in ipv6_gso_pull_exthdrs+0x57a/0x5f0
> > > net/ipv6/ip6_offload.c:45
> > > Read of size 1 at addr ffff8801ce17f3a9 by task syz-executor655/4567
> >
> > This is an 8 byte packet with a virtio_net_hdr describing it as
> > VIRTIO_NET_HDR_GSO_TCPV4, but with protocol ETH_P_NSH. That
> > matches the occurrence of nsh_gso_segment in the stack trace.
> >
> > This pulls the struct nshhdr of 8B, passing a packet with skb->len 0
> > to skb_mac_gso_segment. That is the only GRO function that
> > unconditionally calls _skb_pull without first checking pskb_may_pull.
> > Adding that check does catch this:
> >
> > +       if (unlikely(!pskb_may_pull(skb, vlan_depth)))
> > +               return ERR_PTR(-EINVAL);
>
> vlan_depth is 65528 when entering skb_mac_gso_segment due to
> overflow of skb->mac_len in nsh_gso_segment. For this packet, the
> outer mac len is zero, so skb->data == skb_mac_header(skb). Then
>
>     skb_reset_network_header(skb);
>     [...]
>     __skb_pull(skb, nsh_len);
>     skb_reset_mac_header(skb);    // now mac hdr starts nsh_len == 8B
> after net hdr
>     skb_reset_mac_len(skb);          // mac len = net hdr - mac hdr ==
> (u16) -8 == 65528
>     [..]
>     skb_mac_gso_segment(skb, ..)
>
> Setting skb->mac_len to 0, similar to mpls_gs_segment,
> is sufficient if the encapsulated packet is not ETH_P_TEB.
>
> If the packet is encapsulated at L2, __skb_pull(skb, vlan_depth)
> has to pull the inner mac header before passing to l3 handlers like
> inet_gso_segment.
>
> If that header includes VLAN tags, skb_network_protocol will
> parse then and update the mac length in vlan_depth. So
> hardcoding to ETH_HLEN should be fine:
>
> @@ -104,7 +95,7 @@ static struct sk_buff *nsh_gso_segment(struct sk_buff *skb,
>         __skb_pull(skb, nsh_len);
>
>         skb_reset_mac_header(skb);
> -       skb_reset_mac_len(skb);
> +       skb->mac_len = proto == ETH_P_TEB ? ETH_HLEN : 0;

Well, htons(ETH_P_TEB)

but after this patch the reproducer no longer triggers.
ipv6_gso_segment will catch the zero length inner packet with
pskb_may_pull.

skb_mac_gso_segment itself does not strictly need a
pskb_may_pull, as skb_network_protocol calls pskb_may_pull
when processing ETH_P_TEB and VLAN headers.

Still, adding this would help catch bugs in callers sooner:

-       if (unlikely(!type))
+       if (unlikely(!type || !pskb_may_pull(skb, vlan_depth)))
                return ERR_PTR(-EINVAL);