netdev - Re: [PATCH net v2] net: gso: fix panic on frag

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:   Wed, 2 Nov 2022 13:11:22 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Jiri Benc <jbenc@...hat.com>
Cc:     netdev@...r.kernel.org, Shmulik Ladkani <shmulik@...anetworks.com>,
        Eric Dumazet <eric.dumazet@...il.com>,
        Tomas Hruby <tomas@...era.io>,
        Jeremi Piotrowski <jpiotrowski@...ux.microsoft.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Willem de Bruijn <willemdebruijn.kernel@...il.com>
Subject: Re: [PATCH net v2] net: gso: fix panic on frag_list with mixed head
 alloc types

On Wed, Nov 2, 2022 at 12:55 PM Jiri Benc <jbenc@...hat.com> wrote:
>
> Since commit 3dcbdb134f32 ("net: gso: Fix skb_segment splat when
> splitting gso_size mangled skb having linear-headed frag_list"), it is
> allowed to change gso_size of a GRO packet. However, that commit assumes
> that "checking the first list_skb member suffices; i.e if either of the
> list_skb members have non head_frag head, then the first one has too".
>
> It turns out this assumption does not hold. We've seen BUG_ON being hit
> in skb_segment when skbs on the frag_list had differing head_frag with
> the vmxnet3 driver. This happens because __netdev_alloc_skb and
> __napi_alloc_skb can return a skb that is page backed or kmalloced
> depending on the requested size. As the result, the last small skb in
> the GRO packet can be kmalloced.
>
> There are three different locations where this can be fixed:
>
> (1) We could check head_frag in GRO and not allow GROing skbs with
>     different head_frag. However, that would lead to performance
>     regression on normal forward paths with unmodified gso_size, where
>     !head_frag in the last packet is not a problem.
>
> (2) Set a flag in bpf_skb_net_grow and bpf_skb_net_shrink indicating
>     that NETIF_F_SG is undesirable. That would need to eat a bit in
>     sk_buff. Furthermore, that flag can be unset when all skbs on the
>     frag_list are page backed. To retain good performance,
>     bpf_skb_net_grow/shrink would have to walk the frag_list.
>
> (3) Walk the frag_list in skb_segment when determining whether
>     NETIF_F_SG should be cleared. This of course slows things down.
>
> This patch implements (3). To limit the performance impact in
> skb_segment, the list is walked only for skbs with SKB_GSO_DODGY set
> that have gso_size changed. Normal paths thus will not hit it.
>
> We could check only the last skb but since we need to walk the whole
> list anyway, let's stay on the safe side.
>
> Fixes: 3dcbdb134f32 ("net: gso: Fix skb_segment splat when splitting gso_size mangled skb having linear-headed frag_list")
> Signed-off-by: Jiri Benc <jbenc@...hat.com>

Reviewed-by: Willem de Bruijn <willemb@...gle.com>