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

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

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.