Re: [PATCH net] gro: do not GRO GSO packets with inconsistent payload

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Eric Dumazet wrote:
> Le jeu. 29 oct. 2026 à 00:33, Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> a écrit :
> >
> > Eric Dumazet wrote:
> > > From: Kyle Zeng <kylebot@openai.com>
> > >
> > > tcp_gro_receive() excludes empty non-GSO packets with the unsigned
> > > len - 1 check, but its GSO branch only checks that gso_size matches
> > > the held packet's MSS. A trusted TCP GSO skb can reach GRO again with
> > > no payload after TC pedit increases its TCP data offset.
> > >
> > > If the skb is fully linear, skb_gro_receive() then enters its fragment
> > > transfer path with nr_frags == 0. The do-while loop copies skb_frag_t
> > > entries backwards beyond both fragment arrays. An unprivileged process
> > > using its own user and network namespaces can trigger a KASAN
> > > slab-out-of-bounds write and panic through veth, pedit and GRE.
> > >
> > > More generally, the payload of such a packet can be shrunk to any
> > > value, making it inconsistent with its gso_segs, and a GSO packet
> > > with gso_segs == 0 contributes no segment to the GRO packet.
> > >
> > > Do not aggregate GSO packets with gso_segs == 0, and require the
> > > payload of GSO packets to be consistent with gso_segs and gso_size,
> > > the last segment being allowed to be short. This rejects the empty
> > > skb before either TCP merge helper, and covers both IPv4 and IPv6.
> > >
> > > Fixes: 5eddb24901ee ("gro: add support of (hw)gro packets to gro stack")
> > > Signed-off-by: Kyle Zeng <kylebot@openai.com>
> > > [edumazet: check payload against gso_segs, refuse gso_segs == 0]
> > > Signed-off-by: Eric Dumazet <edumazet@kernel.org>
> > > ---
> > >  net/core/gro.c         |  5 +++--
> > >  net/ipv4/tcp_offload.c | 12 +++++++++---
> > >  2 files changed, 12 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/net/core/gro.c b/net/core/gro.c
> > > index ff828a3dce3ca649e149a840bd6e3dfd9ddaf0c4..1b033855a20db6b8b996179f0b81e38393d3864a 100644
> > > --- a/net/core/gro.c
> > > +++ b/net/core/gro.c
> > > @@ -513,9 +513,10 @@ static enum gro_result dev_gro_receive(struct gro_node *gro,
> > >       NAPI_GRO_CB(skb)->count = 1;
> > >       if (unlikely(skb_is_gso(skb))) {
> > >               NAPI_GRO_CB(skb)->count = skb_shinfo(skb)->gso_segs;
> > > -             /* Only support TCP and non DODGY users. */
> > > +             /* Only support TCP and non DODGY users, with a sane gso_segs. */
> > >               if (!skb_is_gso_tcp(skb) ||
> > > -                 (skb_shinfo(skb)->gso_type & SKB_GSO_DODGY))
> > > +                 (skb_shinfo(skb)->gso_type & SKB_GSO_DODGY) ||
> > > +                 !NAPI_GRO_CB(skb)->count)
> >
> > Or make tc_pedit set SKB_GSO_DODGY? BPF already does.
> >
> > In pedit_l4_skb_offset and pedit_l4_skb_offset.
> 
> Good idea, this would also make sure gso_segs is recomputed if
> such packets are later segmented.
> 
> I would still keep the !count check: ixgbe and libwx RSC packets reach
> GRO with gso_segs == 0, and the payload check in tcp_gro_receive()
> relies on count being non zero.

Should those get fixed instead (eventually).

> pedit is also not the only way to change the TCP header of a GSO
> packet (nft payload on older kernels, NFQUEUE with
> NFQA_CFG_F_GSO, bpf_skb_store_bytes()), so I prefer GRO to stay
> robust anyway.

Argh. Good point. In that case:

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

> pedit can be changed later in net-next to set SKB_GSO_DODGY on
> GSO packets it modifies. Note this needs skb_unclone(), which can
> fail. pedit does not drop packets on allocation errors today, so
> it would probably leave the packet unmarked, another reason for
> GRO to not trust these packets.