netdev - Re: BUG_ON in skb_segment, after bpf_skb_change

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: <CA+FuTSfVsgNDi7c=GUU8nMg2hWxF2SjCNLXetHeVPdnxAW5K-w@mail.gmail.com>
Date:   Sun, 1 Sep 2019 16:05:48 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Shmulik Ladkani <shmulik.ladkani@...il.com>
Cc:     Daniel Borkmann <daniel@...earbox.net>,
        Eric Dumazet <eric.dumazet@...il.com>,
        netdev <netdev@...r.kernel.org>,
        Alexander Duyck <alexander.duyck@...il.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Yonghong Song <yhs@...com>,
        Steffen Klassert <steffen.klassert@...unet.com>,
        Shmulik Ladkani <shmulik@...anetworks.com>,
        eyal@...anetworks.com
Subject: Re: BUG_ON in skb_segment, after bpf_skb_change_proto was applied

On Thu, Aug 29, 2019 at 8:22 AM Shmulik Ladkani
<shmulik.ladkani@...il.com> wrote:
>
> On Tue, 27 Aug 2019 14:10:35 +0200
> Daniel Borkmann <daniel@...earbox.net> wrote:
>
> > Given first point above wrt hitting rarely, it would be good to first get a
> > better understanding for writing a reproducer. Back then Yonghong added one
> > to the BPF kernel test suite [0], so it would be desirable to extend it for
> > the case you're hitting. Given NAT64 use-case is needed and used by multiple
> > parties, we should try to (fully) fix it generically.
> >
>
> Thanks Daniel.
>
> Managed to write a reproducer which mimics the skb we see on prodction,
> that hits the exact same BUG_ON.
>
> Submitted as a separate RFC PATCH to bpf-next.

Thanks for the reproducer.

One quick fix is to disable sg and thus revert to copying in this
case. Not ideal, but better than a kernel splat:

@@ -3714,6 +3714,9 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
        sg = !!(features & NETIF_F_SG);
        csum = !!can_checksum_protocol(features, proto);

+       if (list_skb && skb_headlen(list_skb) && !list_skb->head_frag)
+               sg = false;
+

It could perhaps be refined to avoid in the special case where
skb_headlen(list_skb) == len and nskb aligned to start of list_skb.
And needs looking into effect on GSO_BY_FRAGS.

I also looked into trying to convert a kmalloc'ed skb->head into a
headfrag. But even if possible, that conversion is non-trivial and
easy to have bugs of its own.

@@ -3849,8 +3885,8 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
                                if (!skb_headlen(list_skb)) {
                                        BUG_ON(!nfrags);
                                } else {
-                                       BUG_ON(!list_skb->head_frag);
-
+                                       BUG_ON(!list_skb->head_frag &&
+
!skb_to_headfrag(list_skb, GFP_ATOMIC));