netdev - Re: [PATCH net] 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]

Message-ID: <20221029104131.07fbc6cf@blondie>
Date:   Sat, 29 Oct 2022 10:41:31 +0300
From:   Shmulik Ladkani <shmulik.ladkani@...il.com>
To:     Jiri Benc <jbenc@...hat.com>, willemb@...gle.com
Cc:     netdev@...r.kernel.org, Eric Dumazet <eric.dumazet@...il.com>,
        Tomas Hruby <tomas@...era.io>,
        Jeremi Piotrowski <jpiotrowski@...ux.microsoft.com>,
        alexanderduyck@...a.com, Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH net] net: gso: fix panic on frag_list with mixed head
 alloc types

On Thu, 27 Oct 2022 10:20:56 +0200
Jiri Benc <jbenc@...hat.com> wrote:

> 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. That
> particular case was with vmxnet3; looking at the driver, it indeed uses
> different skb allocation strategies based on the packet size. The last
> packet in frag_list can thus be kmalloced if it is sufficiently small.
> And there's nothing preventing drivers from mixing things even more
> freely.

Hi Jiri,

One of my early attempts to fix the original BUG was to also detect:

> - some frag in the frag_list has a linear part that is NOT head_frag,
>   or length not equal to the requested gso_size

See [0], see skb_is_nonlinear_equal_frags() there

(Note that your current suggestion implements the "some frag in the
 frag_list has a linear part that is NOT head_frag" condition, but not
 "length not equal to the requested gso_size")

As a response, Willem suggested:

> My suggestion only tested the first frag_skb length. If a list can be
> created where the first frag_skb is head_frag but a later one is not,
> it will fail short. I kind of doubt that.

See [1]

So we eventually concluded testing just
  !list_skb->head_frag && skb_headlen(list_skb)
and not every frag in frag_list.

Maybe Willem can elaborate on that.


[0] https://lore.kernel.org/netdev/20190903185121.56906d31@pixies/
[1] https://lore.kernel.org/netdev/CA+FuTScE=pyopY=3f5E4JGx1zyGqT+XS+8ss13UN4if4TZ2NbA@mail.gmail.com/