Re: [PATCH net v2 2/2] net: gro: invalidate frag0 after a pull

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 (1) · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] net: gro: packet corruption in gro_pull_from_frag0() after
  failed pull

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  `napi_gro_frags()` (net/core/gro.c:783-794) never checks the result of…

--

net: gro: invalidate frag0 after a pull

This patch adds skb_gro_frag0_invalidate(), which clears
NAPI_GRO_CB(skb)->frag0_len. It is called before the slow pulls in
skb_gro_header_slow(), skb_gro_receive_list(), esp4_gro_receive(),
esp6_gro_receive() and udp_gro_receive_segment(). This keeps later GRO
header accesses from using a frag0 pointer that the pull may have
released or reallocated.

> diff --git a/include/net/gro.h b/include/net/gro.h
> index 00b49b6b9e74f..6f6affc8ab91c 100644
> --- a/include/net/gro.h
> +++ b/include/net/gro.h
> @@ -160,9 +160,19 @@ static inline bool skb_gro_may_pull(const struct sk_buff *skb,
>  	return likely(hlen <= NAPI_GRO_CB(skb)->frag0_len);
>  }
>  
> +/* A pull can release the page fragment frag0 points to, or reallocate
> + * skb->head, even if it fails. Call this before the pull, to make sure later
> + * skb_gro_header() and skb_gro_network_header() calls no longer use frag0.
> + */
> +static inline void skb_gro_frag0_invalidate(struct sk_buff *skb)
> +{
> +	NAPI_GRO_CB(skb)->frag0_len = 0;
> +}
> +
>  static inline void *skb_gro_header_slow(struct sk_buff *skb, unsigned int hlen,
>  					unsigned int offset)
>  {
> +	skb_gro_frag0_invalidate(skb);
>  	if (!pskb_may_pull(skb, hlen))
>  		return NULL;

[Severity: High]
Does clearing frag0_len here, while frag0 itself is left pointing at
the old location, break gro_pull_from_frag0() when the slow pull in
skb_gro_header_slow() fails?

Consider an skb that enters GRO with skb_headlen(skb) == 0. In that
case skb_gro_reset_offset() points frag0 at the start of frags[0],
which is packet offset 0.

Now add remote checksum offload nopartial: vxlan_gro_receive() or
vxlan_gpe_gro_receive() with VXLAN_F_REMCSUM_NOPARTIAL, or
gue_gro_receive() with FOU_F_REMCSUM_NOPARTIAL. There,
skb_gro_remcsum_process() records grc->offset and grc->delta through
the frag0 fast path. The inner eth_gro_receive() and
inet_gro_receive()/ipv6_gro_receive() handlers then advance
skb_gro_offset() through frag0 too, so skb_headlen() is still 0.

Suppose the next inner header does not fit in frag0_len, for instance
in tcp_gro_pull_header() or udp_gro_udphdr(). skb_gro_header_slow()
now sets frag0_len to 0 and calls pskb_may_pull(). On a truncated
packet where hlen > skb->len, this fails without pulling anything.
That leaves skb_headlen() at 0 and frag0_len at 0.

On the way out, skb_gro_flush_final_remcsum() calls
skb_gro_remcsum_cleanup(), which does:

	ptr = skb_gro_header(skb, plen, grc->offset);

Before this patch that was served from frag0. Now skb_gro_may_pull()
fails, and pskb_may_pull(skb, plen) really copies plen bytes into the
linear area. Here plen = grc->offset + sizeof(u16), which is below
skb_gro_offset(skb) once the inner headers were pulled. frags[0] is
advanced by plen, but NAPI_GRO_CB(skb)->frag0 still points at packet
offset 0.

Since the skb is flushed, dev_gro_receive() ends up in
gro_try_pull_from_frag0(). That computes
grow = skb_gro_offset(skb) - skb_headlen(skb) > 0 and calls
gro_pull_from_frag0():

	memcpy(skb_tail_pointer(skb), NAPI_GRO_CB(skb)->frag0, grow);

This copies packet bytes 0..grow to skb->data + plen, then advances
frags[0] by grow again. The skb handed up the stack as GRO_NORMAL
has its inner headers overwritten with outer header bytes, and the
real bytes at plen..skb_gro_offset() are lost. A remote sender can
trigger this with a short encapsulated packet.

gro_pull_from_frag0() assumes frag0 is the current start of frags[0],
which only holds while skb_headlen() is 0. Should it copy from
skb_frag_address(&pinfo->frags[0]) instead? Or should
skb_gro_remcsum_cleanup() and gro_try_pull_from_frag0() handle a
partially linearized skb in some other way?

[Severity: High]
This patch didn't introduce this, but since the failure path of
skb_gro_header_slow() is being changed here: what happens in
napi_gro_frags() when this pull fails for the Ethernet header?

In net/core/gro.c, napi_frags_skb() takes the slow path and returns
NULL when the pull fails:

net/core/gro.c:napi_frags_skb() {
    ...
	if (unlikely(!skb_gro_may_pull(skb, hlen))) {
		eth = skb_gro_header_slow(skb, hlen, 0);
		if (unlikely(!eth)) {
			net_warn_ratelimited("%s: dropping impossible skb from %s\n",
					     __func__, napi->dev->name);
			napi_reuse_skb(napi, skb);
			return NULL;
    ...
}

The caller doesn't check for NULL, though:

net/core/gro.c:napi_gro_frags() {
	struct sk_buff *skb = napi_frags_skb(napi);

	trace_napi_gro_frags_entry(skb);

	ret = napi_frags_finish(napi, skb, dev_gro_receive(&napi->gro, skb));
    ...
}

dev_gro_receive() starts with skb_get_hash_raw(skb) and then reads
skb->protocol. Can this be a NULL pointer dereference in NAPI softirq
context? One way to reach it would be skb->len < ETH_HLEN, where
pskb_may_pull() fails with PKT_TOO_SMALL. Another would be
__pskb_pull_tail() returning NULL.

napi_gro_frags() used to have an "if (!skb) return GRO_DROP;" check.
Commit 1d11fa696733 ("net-gro: remove GRO_DROP") removed it on the
grounds that NULL could only come from a failed napi_get_frags()
allocation. That reasoning doesn't cover the header pull failure return
shown above.

The drivers checked so far (tun, r8152, e1000) all enforce a minimum
frame length before calling napi_gro_frags(). The other callers have
not been checked.

Should napi_gro_frags() check for a NULL skb again?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261030125955.3107383-1-edumazet%40kernel.org