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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sat, 29 Oct 2022 10:10:03 -0400 From: Willem de Bruijn <willemdebruijn.kernel@...il.com> To: Shmulik Ladkani <shmulik.ladkani@...il.com> Cc: Jiri Benc <jbenc@...hat.com>, 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 Sat, Oct 29, 2022 at 3:41 AM Shmulik Ladkani <shmulik.ladkani@...il.com> wrote: > > 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. Clearly I was wrong :) If a device has different allocation strategies depending on packet size, and GRO coalesces those using a list, then indeed this does not have to hold. GRO requires the same packet size and thus allocation strategy to coalesce -- except for the last segment. I don't see any allocation in vmxnet3 that uses a head frag, though. There is a small packet path (rxDataRingUsed), but both small and large allocate using a kmalloc-backed skb->data as far as I can tell. In any case, iterating over all frags is more robust. This is an edge case, fine to incur the cost there.
Powered by blists - more mailing lists