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

[Paolo Abeni <pabeni@redhat.com>]

On 9/16/26 15:05, netdev-bot+sashiko@kernel.org wrote:
> 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.

Given the last paragraph all the above is IMHO material for eventual future
cleanups.

/P