On 10/29/26 12:47, Alexander H Duyck wrote:
On Wed, 2026-10-28 at 13:02 -0400, Matt Vollrath wrote:
When an skb arrives at the driver and the header length exceeds the u8
hdr_len, tucss and tucso fields in e1000_context_desc, wrapped values
are sent to the card. If TSO is enabled, some header lengths will cause
the card to hang.
This was reproduced on an I218-V with an unprivileged Python script.
Can you elaborate on what the script did to trigger this? I am
wondering if it can even get there with a valid frame supported by the
interface since most of these e1000e parts don't support tunnels and
such.
I will send you the script OOB. To be clear, this is a security fix,
not a correctness fix for rarely used paths. The rules say I have to
post the fix publicly because an LLM found it in a
function-by-function scan.
Also did it have separate cases for the CSUM vs the GSO? I would
suspect this is more a CSUM issue than a GSO issue.
I have only reproduced hardware hang when TSO is enabled on the nic.
Otherwise it likely just mangles the packet, but I have not confirmed
that.
Fix it by disabling GSO and checksum offload when the header length (and
checksum offset) would exceed the field widths. Compensate for a
software-inserted VLAN tag.
Because of a quirk in the ordering of ndo_features_check, the skb's
transport_header field may be stale. If the skb carries
CHECKSUM_PARTIAL, use csum_start to find the header. Otherwise, there's
no reliable interface, so don't offload it.
Fixes: bc7f75fa9788 ("[E1000E]: New pci-express e1000 driver (currently for ICH9
devices only)")
I am pretty sure this Fixes tag is bogus. This is the introduction of
the e1000e driver which was in 2007. The ndo_features_check didn't
exist until 2014 sometime after that if I am not mistaken.
My understanding is the Fixes tag should point at the commit where the
bug was introduced. A comment on the Cc: stable tag indicates how far
back the fix can backport. I didn't bother because this patch should
apply back to 5.0 (skb_is_gso_tcp()) and the oldest LTS is 5.10.
Much of the complexity that enabled frames to build up massive headers
wasn't until much later when the network tunnels and OVS started being
added.
The repro script should work back to 4.10.
Cc: [email protected]
Assisted-by: LLM
Signed-off-by: Matt Vollrath <[email protected]>
---
drivers/net/ethernet/intel/e1000e/netdev.c | 34 +++++++++++++++++++++-
1 file changed, 33 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c
b/drivers/net/ethernet/intel/e1000e/netdev.c
index 6f85dab5c55c5..370776657d100 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -7355,6 +7355,38 @@ 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)
+{
+ /* Do not offload packets whose header lengths exceed the u8 fields
+ * in the descriptor.
+ */
+ unsigned int hdr_len = 0;
+
+ if (skb->ip_summed == CHECKSUM_PARTIAL) {
+ const struct tcphdr *th = (const void *)skb_checksum_start(skb);
+
+ hdr_len = skb_checksum_start_offset(skb);
+ if (skb_is_gso(skb) && skb_is_gso_tcp(skb))
+ hdr_len += __tcp_hdrlen(th);
+ else
+ hdr_len += skb->csum_offset;
+ } else if (skb_is_gso(skb)) {
+ /* Work around untrustworthy header offsets in this case. */
+ return features & ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK);
+ }
Most drivers currently assume if the CHECKSUM_PARTIAL isn't set then
the frame must not be TCP/UDP GSO since for those normally the checksum
and offsets should be populated. Have you identified cases where we are
seeing TCP/UDP GSO w/o the checksum being set up?
It looks like this particular case is never reached by normal traffic.
It has to be GSO without NEEDS_CSUM. This is only produced by a buggy,
hostile or niche setup.
When CHECKSUM_PARTIAL is not set, the csum_start/csum_offset fields are
used for csum instead, or left zeroed.
The transport_header field is unreliable when a packet is being routed,
so you would need to parse the packet from the top to find where the
transport header ends.
Rather than walk all the headers for hdr_len, it seemed prudent to just
categorically deny offload for these exceptional cases. I could be
convinced otherwise.
From what I can tell it looks like the only valid cases for it not to
be set and instead it is set to CHECKSUM_UNNECESSARY on a GSO frame are
FCoE and SCTP (GSO_FRAGS) which are both non-TCP/UDP specialty cases
anyway.
FCoE was switched to CHECKSUM_PARTIAL in 253aab0597d9e
+
+ if (skb_vlan_tag_present(skb) &&
+ !vlan_hw_offload_capable(features, skb->vlan_proto))
+ hdr_len += VLAN_HLEN;
+
+ if (hdr_len > U8_MAX)
+ features &= ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK);
+
+ return features;
+}
+
static const struct net_device_ops e1000e_netdev_ops = {
.ndo_open = e1000e_open,
.ndo_stop = e1000e_close,
@@ -7374,7 +7406,7 @@ static const struct net_device_ops e1000e_netdev_ops = {
#endif
.ndo_set_features = e1000_set_features,
.ndo_fix_features = e1000_fix_features,
- .ndo_features_check = passthru_features_check,
+ .ndo_features_check = e1000_features_check,
.ndo_hwtstamp_get = e1000e_hwtstamp_get,
.ndo_hwtstamp_set = e1000e_hwtstamp_set,
};
base-commit: 7076f684d5132e6a6145e1da2fa74771ecd052e5
