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

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

[Matt Vollrath]

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.

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)")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Matt Vollrath <tactii@gmail.com>
---
 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);
+	}
+
+	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
-- 
2.43.0

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

[Alexander H Duyck]

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.

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.

> 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.

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.

> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: Matt Vollrath <tactii@gmail.com>
> ---
>  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?

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.

> +
> +	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

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

[Matt Vollrath]

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: stable@vger.kernel.org
>> Assisted-by: LLM
>> Signed-off-by: Matt Vollrath <tactii@gmail.com>
>> ---
>>   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

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

[Alexander Duyck]

On Fri, Oct 30, 2026 at 12:00 PM Matt Vollrath <tactii@gmail.com> wrote:
>
> 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.

I got the script, will try to take a look at it over this weekend.

> > 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.

I get that, but at the same time the fixes tag is pointing at the
driver when what more likely happened is that the stack evolved past
what the driver can handle as evidenced by the fact that the fix would
never be able to backport that far as the function it was adding was
introduced sometime after the additional stack complexity was added.

> > 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.

Okay. However, that still only goes back as far as 2017 if I am not
mistaken. The diff you are blaming is from 2007. I suspect we should
be looking at blaming whatever introduced the ability to push the
headers this far as the driver itself isn't likely the root cause for
the issue, but instead a header change was.

> >> Cc: stable@vger.kernel.org
> >> Assisted-by: LLM
> >> Signed-off-by: Matt Vollrath <tactii@gmail.com>
> >> ---
> >>   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.

The point I was trying to get at is that the stack shouldn't be
allowing those sort of frames through in the first place. Basically if
checksum partial isn't set the stack should be rejecting the frame for
UDP/TCP GSO, not the drivers.

> >  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

This is part of the problem. It pushed through the ability for
CHECKSUM_UNECESSARY to be valid in an outgoing TCP frame, and then was
reverted back to using CHECKSUM_PARTIAL. That is what is partially
what is allowing the non-partial checksum through for GSO frames.

I will probably look at submitting a diff for this next week to look
at limiting this as there are a number of drivers that already expect
all TSO/USO frames to have CHECKSUM_PARTIAL always set before they
start processing the frames.

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

[Matt Vollrath]

On 10/31/26 12:53, Alexander Duyck wrote:
> On Fri, Oct 30, 2026 at 12:00 PM Matt Vollrath <tactii@gmail.com> wrote:
>>
>> 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.
> 
> I got the script, will try to take a look at it over this weekend.
> 
>>> 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.
> 
> I get that, but at the same time the fixes tag is pointing at the
> driver when what more likely happened is that the stack evolved past
> what the driver can handle as evidenced by the fact that the fix would
> never be able to backport that far as the function it was adding was
> introduced sometime after the additional stack complexity was added.

The Fixes tag seems correct to me. To confirm it I reproduced the hanging
hdr_len values against 2.6.24 on top of Fedora 8. It was more or less the
same script, just with different headers. I can't confirm that it hangs
the limited set of hardware supported in the first driver commit. Maybe
Intel could test it in their lab.

It's worth finding new core complexities causing problems in drivers, but
in this case the mechanism predates the driver. It really has been a
security bug for 19 years. The switch was enabling TSO6 when e1000e was
spawned from e1000.

The fix not being able to backport all the way to the Fixes tag was a
choice. It has nothing to do with the bug not being a bug that far back.
I'm not spending time supporting Linux 2.6 or any pre-LTS kernel, nor is
anybody at Intel or kernel.org, as far as I know.

The attack surface and details of exploit have changed over time, but
that's not a story that a single Fixes tag can tell. It's the story of
IPv6 headers.
>>> 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.
> 
> Okay. However, that still only goes back as far as 2017 if I am not
> mistaken. The diff you are blaming is from 2007. I suspect we should
> be looking at blaming whatever introduced the ability to push the
> headers this far as the driver itself isn't likely the root cause for
> the issue, but instead a header change was.
> 
>>>> Cc: stable@vger.kernel.org
>>>> Assisted-by: LLM
>>>> Signed-off-by: Matt Vollrath <tactii@gmail.com>
>>>> ---
>>>>    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.
> 
> The point I was trying to get at is that the stack shouldn't be
> allowing those sort of frames through in the first place. Basically if
> checksum partial isn't set the stack should be rejecting the frame for
> UDP/TCP GSO, not the drivers.

I agree that this patch demonstrates how obtuse it is for an
ndo_features_check to reliably measure header length, which is something
that several drivers need to do in the hook. The issues being worked
around should be fixed in core.

GSO without NEEDS_CSUM is the biggest offender, no driver is checking for
that in their implementation of ndo_features_check.

Using a potentially stale transport header is next in line. Several
drivers are getting this wrong. I'm working on a fix, a simple reorder.

>>>   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
> 
> This is part of the problem. It pushed through the ability for
> CHECKSUM_UNECESSARY to be valid in an outgoing TCP frame, and then was
> reverted back to using CHECKSUM_PARTIAL. That is what is partially
> what is allowing the non-partial checksum through for GSO frames.
> 
> I will probably look at submitting a diff for this next week to look
> at limiting this as there are a number of drivers that already expect
> all TSO/USO frames to have CHECKSUM_PARTIAL always set before they
> start processing the frames.

That would be great.

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

[Alexander Duyck]

On Mon, Nov 2, 2026 at 8:32 PM Matt Vollrath <tactii@gmail.com> wrote:
>
> On 10/31/26 12:53, Alexander Duyck wrote:
> > On Fri, Oct 30, 2026 at 12:00 PM Matt Vollrath <tactii@gmail.com> wrote:
> >>
> >> 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.
> >
> > I got the script, will try to take a look at it over this weekend.
> >
> >>> 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.
> >
> > I get that, but at the same time the fixes tag is pointing at the
> > driver when what more likely happened is that the stack evolved past
> > what the driver can handle as evidenced by the fact that the fix would
> > never be able to backport that far as the function it was adding was
> > introduced sometime after the additional stack complexity was added.
>
> The Fixes tag seems correct to me. To confirm it I reproduced the hanging
> hdr_len values against 2.6.24 on top of Fedora 8. It was more or less the
> same script, just with different headers. I can't confirm that it hangs
> the limited set of hardware supported in the first driver commit. Maybe
> Intel could test it in their lab.
>
> It's worth finding new core complexities causing problems in drivers, but
> in this case the mechanism predates the driver. It really has been a
> security bug for 19 years. The switch was enabling TSO6 when e1000e was
> spawned from e1000.
>
> The fix not being able to backport all the way to the Fixes tag was a
> choice. It has nothing to do with the bug not being a bug that far back.
> I'm not spending time supporting Linux 2.6 or any pre-LTS kernel, nor is
> anybody at Intel or kernel.org, as far as I know.
>
> The attack surface and details of exploit have changed over time, but
> that's not a story that a single Fixes tag can tell. It's the story of
> IPv6 headers.

The thing is this has had cycles where you could do it and where you
couldn't. My thought is that it would have made more sense to tag:
Fixes: f2701b185e05 ("e1000e: Enable TSO for stacked VLAN")

Basically there were a number of efforts to fix this in the 20-teens
as all the extra headers for tunnels is when this started to really
become an issue. In addition the driver ended up being moved at some
point between so this likely went from being fixed to being broken
again in a few cycles over the years.

> >>> 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.
> >
> > Okay. However, that still only goes back as far as 2017 if I am not
> > mistaken. The diff you are blaming is from 2007. I suspect we should
> > be looking at blaming whatever introduced the ability to push the
> > headers this far as the driver itself isn't likely the root cause for
> > the issue, but instead a header change was.
> >
> >>>> Cc: stable@vger.kernel.org
> >>>> Assisted-by: LLM
> >>>> Signed-off-by: Matt Vollrath <tactii@gmail.com>
> >>>> ---
> >>>>    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.
> >
> > The point I was trying to get at is that the stack shouldn't be
> > allowing those sort of frames through in the first place. Basically if
> > checksum partial isn't set the stack should be rejecting the frame for
> > UDP/TCP GSO, not the drivers.
>
> I agree that this patch demonstrates how obtuse it is for an
> ndo_features_check to reliably measure header length, which is something
> that several drivers need to do in the hook. The issues being worked
> around should be fixed in core.
>
> GSO without NEEDS_CSUM is the biggest offender, no driver is checking for
> that in their implementation of ndo_features_check.

The point is that it shouldn't need to. It looks like this is
something that was broken over time to first enable FCoE and then
GSO_FRAGLIST. The way it used to work is the code required
CHECSUM_PARTIAL only.

> Using a potentially stale transport header is next in line. Several
> drivers are getting this wrong. I'm working on a fix, a simple reorder.
>
> >>>   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
> >
> > This is part of the problem. It pushed through the ability for
> > CHECKSUM_UNECESSARY to be valid in an outgoing TCP frame, and then was
> > reverted back to using CHECKSUM_PARTIAL. That is what is partially
> > what is allowing the non-partial checksum through for GSO frames.
> >
> > I will probably look at submitting a diff for this next week to look
> > at limiting this as there are a number of drivers that already expect
> > all TSO/USO frames to have CHECKSUM_PARTIAL always set before they
> > start processing the frames.
>
> That would be great.

The one thing that it should change about your patch is that you won't
need to do the clearing of CHECKSUM_PARTIAL. Basically if that isn't
set you can assume there will be no offload requested as GSO will not
allow it through unless GSO_FRAGLIST

I will try to push my patch out sometime today and try to remember to
CC you on it.