netdev - Re: [RFC PATCH net] net-gro: restore check for NULL skb in napi_gro

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]

Date: Wed, 16 Aug 2023 18:46:27 +0100
From: Edward Cree <ecree.xilinx@...il.com>
To: Eric Dumazet <edumazet@...gle.com>, edward.cree@....com
Cc: davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
 netdev@...r.kernel.org, Martin Habets <habetsm.xilinx@...il.com>
Subject: Re: [RFC PATCH net] net-gro: restore check for NULL skb in
 napi_gro_frags

On 02/08/2023 11:22, Eric Dumazet wrote:
> On Wed, Aug 2, 2023 at 11:42 AM <edward.cree@....com> wrote:
>> An sfc customer has encountered this panic in the wild; we're still
>>  investigating exactly how it happened (we have a reproducer) but it
>>  seems wise to have the core handle this check rather than requiring
>>  it in every driver.
> 
> An ethernet driver feeding non-ethernet packets to the upper stacks
> seems weird to me,
...
> Not sure why a napi_gro_frags() enabled driver would be allowed to
> cook arbitrary packets with length <  ETH_HLEN
...
> Mixed feelings here
Fwiw we now have some more understanding of what caused this: the
 device produced an RX descriptor with a zero in its buffer length
 field (there was actually data in the buffer, but a — we think —
 firmware bug caused the length to be stored in the wrong place).
And the driver, blithely trusting the device, attached the RX page
 to the skb from napi_get_frags, passing the buffer length it had
 straight to skb_fill_page_desc without checking it.  (After all,
 the device had told us through RX event flags that the packet was
 TCP, so it had to have seen a complete set of headers.)
This certainly can be fixed in the driver, by adding a length
 check before calling napi_gro_frags(), but I see two reasons to
 put it in the core instead.
1) The same issue is likely to be present in any napi_gro_frags()
   driver; I've just looked at e1000 and mlx4 (as examples) and it
   looks like they could both be susceptible if hw misbehaved in a
   similar way.
2) The core can recycle the SKB, but drivers can't because
   napi_reuse_skb is static.  Instead, if they've already called
   napi_get_frags when they discover the problem, they have to do
	kfree_skb(skb);
	napi->skb = NULL;
   which is not pretty.  Of course we could always export
   napi_reuse_skb...
Another open question is whether, if we do put this in the core,
 we should do the same for the other RX entry points
 (napi_gro_receive, eth_type_trans) that could see something
 similar.

Anyway, I'll post v2, with unlikely() per Tyler's analysis, and you
 can ack or nack as the mood takes you.

-ed