Re: [PATCH iwl-net] e1000e: fix Tx descriptor field wrapping

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] e1000e: missing skb_is_gso() and csum offset check in e1000_features_check()
--

commit a74dc3b4f983f63146f4b296827445229df8d08e
Author: Matt Vollrath <tactii@gmail.com>

e1000e: fix Tx descriptor field wrapping

This commit adds an ndo_features_check callback to e1000e to disable GSO and
checksum offload when the header length or checksum offset exceeds U8_MAX,
preventing 8-bit field wrapping in e1000_context_desc.

> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index 6f85dab5c55c5..11714032793ef 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -7355,6 +7355,27 @@ static int e1000_set_features(struct net_device *netdev,
>  	return 1;
>  }
>  
> +static netdev_features_t e1000_features_check(struct sk_buff *skb,
> +					      struct net_device *netdev,
> +					      netdev_features_t features)
> +{
> +	unsigned int hdr_len = 0;
> +
> +	if (skb_is_gso_tcp(skb))
> +		hdr_len = skb_tcp_all_headers(skb);
> +	else if (skb->ip_summed == CHECKSUM_PARTIAL)
> +		hdr_len = skb_checksum_start_offset(skb) + skb->csum_offset;

[Severity: High]
Should e1000_features_check() check skb_is_gso(skb) before calling
skb_is_gso_tcp(skb), and also verify the checksum offset rather than making
the two checks mutually exclusive?

In include/linux/skbuff.h, skb_is_gso_tcp() only inspects gso_type and notes
that the caller must check skb_is_gso(skb) first:

/* Note: Should be called only if skb_is_gso(skb) is true */
static inline bool skb_is_gso_tcp(const struct sk_buff *skb)
{
	return skb_shinfo(skb)->gso_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6);
}

Because net/ipv4/tcp_output.c:__tcp_transmit_skb() unconditionally sets
skb_shinfo(skb)->gso_type = sk->sk_gso_type on all outgoing TCP packets
(including non-GSO packets where gso_size is 0), every non-GSO TCP packet will
take the skb_is_gso_tcp(skb) branch and skip the CHECKSUM_PARTIAL branch:

1. For normal non-GSO TCP packets, e1000_tso() returns 0 and e1000_tx_csum()
   only writes css and css + csum_offset (16), but e1000_features_check()
   checks skb_tcp_all_headers(skb) and disables checksum offload early when
   the end of the TCP header exceeds U8_MAX even if tucso fits in u8.

2. If a non-GSO TCP skb is encapsulated in an outer header without setting
   skb->encapsulation (for example, L2TP over UDP in l2tp_xmit_core(), or an
   uncloned non-GSO TCP skb encrypted out-of-place by IPsec ESP in
   esp_output_head() / esp_output_tail()), skb->transport_header points to
   the outer 8-byte UDP or ESP header while gso_type still has
   SKB_GSO_TCPV4/V6 set. Calling skb_tcp_all_headers(skb) reads tcp_hdr(skb)->doff
   at offset 12 past the 8-byte header (which can read 4 bytes past skb->tail
   for ESP with a 0-length IV, or read a 0/1 nibble on UDP and allow
   css + csum_offset to wrap u8 tucso in e1000_tx_csum()).

In addition, when __netif_skb_features() invokes ndo_features_check prior to
GSO segmentation, skb_transport_header(skb) can be stale and differ from
skb_checksum_start(skb), as noted in net/core/dev.c:skb_gso_output_max_segs():

	/*
	 * The transport offset can be unset or stale, so hdr_len is only taken
	 * from a header at the checksum start.
	 */
	if (!skb_transport_header_was_set(skb) ||
	    skb_checksum_start(skb) != skb_transport_header(skb))
		return 0;

For forwarded IPv6 packets with extension headers (via ip6_rcv_core()) or
inline IPv6 extension header insertion (such as seg6_do_srh(),
ioam6_do_inline(), or rpl_do_srh_inline()), skb->transport_header points to
the first extension header immediately after struct ipv6hdr (offset 54),
whereas skb->csum_start points to the actual TCP header after the extension
headers (which may be past U8_MAX).

In that case, skb_tcp_all_headers(skb) evaluates to at most 54 + 60 = 114 and
skips the else-if branch, leaving NETIF_F_HW_CSUM and NETIF_F_TSO6 enabled.
Later in validate_xmit_skb() or ipv6_gso_segment(), the transport header is
updated to the real TCP header offset (> U8_MAX), allowing u8 css, tucso, or
hdr_len to wrap in e1000_tx_csum() or e1000_tso().

Would it make sense to guard the TSO check with skb_is_gso(skb) and check
skb->ip_summed == CHECKSUM_PARTIAL independently (or compute the TCP header
length from skb_checksum_start(skb) when skb_is_gso(skb) is true)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027012958.52524-1-tactii@gmail.com?part=1