Re: [PATCH v2 net] net: skbuff: do not leave stale header offsets after pskb_carve()

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] Incomplete invalidation of checksum metadata in the new
  skb_carve_reset_headers() (net/core/skbuff.c:6842).

--

net: skbuff: do not leave stale header offsets after pskb_carve()

The two pskb_carve() helpers used to call skb_headers_offset_update(skb, 0),
which left mac/network/transport offsets describing bytes that were just
removed. This patch adds skb_carve_reset_headers() to unset the mac and
transport headers, reset the network header, clear mac_len, zero the inner
header state, and downgrade CHECKSUM_PARTIAL to CHECKSUM_NONE.

Closes: https://lore.kernel.org/netdev/6aa3e9d3.f2639fcc.29487d.0028.GAE@google.com/

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index cc3b4b70288b4..609f2c7f4a47a 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -6832,6 +6832,34 @@ struct sk_buff *alloc_skb_with_frags(unsigned long header_len,
>  }
>  EXPORT_SYMBOL(alloc_skb_with_frags);
>  
> +/* pskb_carve_inside_header() and pskb_carve_inside_nonlinear()
> + * remove the first bytes of a packet and reallocate skb->head.
> + *
> + * Whatever headers were present before the operation are gone,
> + * we must not leave stale offsets, otherwise users of this skb
> + * (skb_dump(), drop_monitor, taps, ...) would read or pull garbage.
> + */
> +static void skb_carve_reset_headers(struct sk_buff *skb)
> +{
> +	skb_unset_mac_header(skb);
> +	skb_unset_transport_header(skb);
> +	skb_reset_network_header(skb);
> +	skb->mac_len = 0;
> +
> +	/* Inner offsets have no "unset" marker, zero them so that
> +	 * skb_inner_network_header_was_set() becomes false and no
> +	 * consumer mistakes them for a real (and long gone) header.
> +	 */
> +	skb->inner_mac_header = 0;
> +	skb->inner_network_header = 0;
> +	skb->inner_transport_header = 0;
> +	skb->inner_protocol = 0;
> +	skb->encapsulation = 0;
> +
> +	if (skb->ip_summed == CHECKSUM_PARTIAL)
> +		skb->ip_summed = CHECKSUM_NONE;
> +}

[Severity: Medium]
The changelog says the goal is that "all the header state is invalidated in
one place".  Is the checksum state fully invalidated here?

skb_carve_reset_headers() only rewrites ip_summed, while skb->csum,
skb->csum_valid, skb->csum_complete_sw, skb->csum_level and
skb->csum_not_inet keep the values they had before the bytes were removed.

For the CHECKSUM_PARTIAL case, the syzbot dump quoted in the changelog shows
the incoming skb already has valid=1:

  csum(0x100120 start=288 offset=16 ip_summed=3 complete_sw=0 valid=1 level=0)

After the downgrade this becomes ip_summed=CHECKSUM_NONE with csum_valid
still 1, and skb_csum_unnecessary() in include/linux/skbuff.h returns true on
csum_valid alone:

  static inline int skb_csum_unnecessary(const struct sk_buff *skb)
  {
	return ((skb->ip_summed == CHECKSUM_UNNECESSARY) ||
		skb->csum_valid ||
		(skb->ip_summed == CHECKSUM_PARTIAL &&
		 skb_checksum_start_offset(skb) >= 0));
  }

Can a consumer of the carved skb then believe the payload was already
verified, even though the bytes the checksum covered are gone?  A leftover
csum_not_inet would similarly still claim CRC32c semantics, and skb_dump()
would print the contradictory "ip_summed=0 ... valid=1".

Should csum_valid, csum_complete_sw, csum_level and csum_not_inet be cleared
in the same helper?

The CHECKSUM_COMPLETE case looks untouched as well.  Unlike
skb_pull_rcsum()/skb_postpull_rcsum(), the carve path never subtracts the off
bytes removed from the front from skb->csum, and pskb_extract() trims the tail
with plain pskb_trim() rather than pskb_trim_rcsum():

  net/core/skbuff.c:pskb_extract() {
	...
	if (pskb_carve(clone, off, gfp) < 0 ||
	    pskb_trim(clone, to_copy)) {
	...
  }

So a CHECKSUM_COMPLETE skb keeps a whole-packet checksum that no longer
matches its contents.  Would it make sense to also drop or fix up
CHECKSUM_COMPLETE in skb_carve_reset_headers(), or to use pskb_trim_rcsum()
in pskb_extract()?

The only in-tree user of pskb_extract() is RDS, which copies the queued data
to userspace and never calls a checksum validation helper, so there is no
functional failure visible today; the concern is the trust bits left on a
generic exported helper.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915130423.3956471-1-edumazet%40google.com