[PATCH v4] hw/net: fix e1000e ip_len inflation by Ethernet minimum-frame padding

Sanjeeva Yerrapureddy posted 1 patch 3 weeks, 6 days ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20260628-net-tx-pkt-ip-length-padding-v4-1-1794063064fa@gmail.com
Maintainers: Dmitry Fleytman <dmitry.fleytman@gmail.com>, Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>, Jason Wang <jasowang@redhat.com>
hw/net/e1000e_core.c |  8 +++++++-
hw/net/net_tx_pkt.c  | 17 +++++++++++++++++
hw/net/net_tx_pkt.h  | 13 +++++++++++++
3 files changed, 37 insertions(+), 1 deletion(-)
[PATCH v4] hw/net: fix e1000e ip_len inflation by Ethernet minimum-frame padding
Posted by Sanjeeva Yerrapureddy 3 weeks, 6 days ago
When a guest transmits a short Ethernet frame, iov_size() returns the
padded wire length including any bytes added to reach the Ethernet
minimum frame size of 60 bytes.  net_tx_pkt_rebuild_payload() uses
this inflated size as payload_len.  In the non-TSO IXSM path,
net_tx_pkt_update_ip_hdr_checksum() then overwrites the IPv4 Total
Length field with payload_len + l3_hdr_len, inflating it by the
padding bytes.  The receiver interprets Ethernet padding as IP
payload, producing a malformed packet.

Fix the IXSM path in e1000e by adding a new function,
net_tx_pkt_update_ip_hdr_checksum_only(), that recomputes the IPv4
header checksum without modifying ip_len.  For non-TSO packets the
guest has already set ip_len correctly; only the checksum needs to be
recomputed after the header is reassembled into the linear buffer.
A new common-layer function is needed because struct NetTxPkt is
opaque to device code and the IP header bytes required by
net_raw_checksum() are inaccessible from e1000e_core.c.

The TSO path is unaffected: it continues to call
net_tx_pkt_update_ip_checksums() which correctly writes ip_len from
payload_len (ip_len=0 per Intel 82574 datasheet §7.3.4 for TSO
super-packets that the host must segment).

Signed-off-by: Sanjeeva Yerrapureddy <y.sanjeevreddy@gmail.com>
---
Thank you Akihiko for your review. I agree that device-specific logic
should stay in device code and not in net_tx_pkt_rebuild_payload(). v4
takes a different approach that avoids any changes to the common
parsing path.

Changes in v4:
- Revert all changes to net_tx_pkt_rebuild_payload(); it is unchanged
  from upstream.
- Add net_tx_pkt_update_ip_hdr_checksum_only() to the net_tx_pkt
  common layer: identical to net_tx_pkt_update_ip_hdr_checksum() but
  skips the ip_len write. A new common-layer function is necessary
  because struct NetTxPkt is opaque to device code — the IP header
  bytes needed by net_raw_checksum() are inaccessible from e1000e_core.c.
- Change the IXSM (non-TSO) path in e1000e_setup_tx_offloads() to call
  net_tx_pkt_update_ip_hdr_checksum_only(). The guest has already set
  ip_len correctly; we only need to recompute the checksum.
- The TSO path is unaffected: it continues to call
  net_tx_pkt_update_ip_checksums() which correctly writes ip_len from
  payload_len (ip_len=0 per §7.3.4 for TSO super-packets).
- Link to v3: https://lore.kernel.org/qemu-devel/20260627-net-tx-pkt-ip-length-padding-v3-1-e860dabadd3f@gmail.com

Changes in v3:
- Architectural redesign: three-case fallback logic in
  net_tx_pkt_rebuild_payload() plus net_tx_pkt_set_payload_len() API;
  e1000e pre-sets payload_len from PAYLEN descriptor field.
- Link to v2: https://lore.kernel.org/qemu-devel/20260625-net-tx-pkt-ip-length-padding-v2-1-287d0ba59ec1@gmail.com

Changes in v2:
- Fix iov_copy() to use raw_payload_len and net_tx_pkt_get_total_len()
  to use iov_size() to preserve correct wire frame size.
- Link to v1: https://lore.kernel.org/qemu-devel/20260624-net-tx-pkt-ip-length-padding-v1-1-7a22fe9b4e40@gmail.com

On the relationship to the short-packet receive-path discussion
(Peter Maydell):
https://lore.kernel.org/qemu-devel/CAFEAcA_UhmCxJc16CHE=4ZR1+PA0=4-29=fEe+d+iUmhLTzpuQ@mail.gmail.com/

The linked thread concerns the receive path. This patch addresses only
the transmit path: the IXSM offload path was overwriting a correctly
set guest ip_len with an inflated value that included Ethernet padding.
No changes to padding behaviour or the receive path are made.
---
 hw/net/e1000e_core.c |  8 +++++++-
 hw/net/net_tx_pkt.c  | 17 +++++++++++++++++
 hw/net/net_tx_pkt.h  | 13 +++++++++++++
 3 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
index 46e156a5dd..3c0347caf8 100644
--- a/hw/net/e1000e_core.c
+++ b/hw/net/e1000e_core.c
@@ -614,7 +614,13 @@ e1000e_setup_tx_offloads(E1000ECore *core, struct e1000e_tx *tx)
     }
 
     if (tx->sum_needed & E1000_TXD_POPTS_IXSM) {
-        net_tx_pkt_update_ip_hdr_checksum(tx->tx_pkt);
+        /*
+         * Guest already set ip_len correctly; only recompute the checksum.
+         * net_tx_pkt_update_ip_hdr_checksum() would overwrite ip_len with
+         * payload_len + l3_hdr_len, inflating it by any Ethernet
+         * minimum-frame padding bytes present in payload_len.
+         */
+        net_tx_pkt_update_ip_hdr_checksum_only(tx->tx_pkt);
     }
 
     return true;
diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c
index 903238dca2..c2eae2f74b 100644
--- a/hw/net/net_tx_pkt.c
+++ b/hw/net/net_tx_pkt.c
@@ -102,6 +102,23 @@ void net_tx_pkt_update_ip_hdr_checksum(struct NetTxPkt *pkt)
     pkt->l3_hdr.ip.ip_sum = cpu_to_be16(csum);
 }
 
+void net_tx_pkt_update_ip_hdr_checksum_only(struct NetTxPkt *pkt)
+{
+    uint16_t csum;
+    assert(pkt);
+
+    /*
+     * Recompute the IP header checksum without touching ip_len.
+     * Used for non-TSO packets where the guest already set ip_len
+     * correctly; overwriting it would inflate it by any Ethernet
+     * minimum-frame padding present in payload_len.
+     */
+    pkt->l3_hdr.ip.ip_sum = 0;
+    csum = net_raw_checksum(pkt->l3_hdr.octets,
+        pkt->vec[NET_TX_PKT_L3HDR_FRAG].iov_len);
+    pkt->l3_hdr.ip.ip_sum = cpu_to_be16(csum);
+}
+
 void net_tx_pkt_update_ip_checksums(struct NetTxPkt *pkt)
 {
     uint16_t csum;
diff --git a/hw/net/net_tx_pkt.h b/hw/net/net_tx_pkt.h
index 0a716e74a5..d2840f38e8 100644
--- a/hw/net/net_tx_pkt.h
+++ b/hw/net/net_tx_pkt.h
@@ -133,6 +133,19 @@ bool net_tx_pkt_update_sctp_checksum(struct NetTxPkt *pkt);
  */
 size_t net_tx_pkt_get_total_len(struct NetTxPkt *pkt);
 
+/**
+ * recompute the IPv4 header checksum without modifying ip_len
+ *
+ * Use for non-TSO packets where the guest already set ip_len correctly.
+ * Calling net_tx_pkt_update_ip_hdr_checksum() instead would overwrite
+ * ip_len with payload_len + l3_hdr_len, inflating it by any Ethernet
+ * minimum-frame padding present in payload_len.
+ *
+ * @pkt:            packet
+ *
+ */
+void net_tx_pkt_update_ip_hdr_checksum_only(struct NetTxPkt *pkt);
+
 /**
  * get packet type
  *

---
base-commit: b83371668192a705b878e909c5ae9c1233cbd5fb
change-id: 20260628-net-tx-pkt-ip-length-padding-951e75fa97b4

Best regards,
--  
Sanjeeva Yerrapureddy <y.sanjeevreddy@gmail.com>


Re: [PATCH v4] hw/net: fix e1000e ip_len inflation by Ethernet minimum-frame padding
Posted by Akihiko Odaki 3 weeks, 6 days ago
On 2026/06/29 0:33, Sanjeeva Yerrapureddy wrote:
> When a guest transmits a short Ethernet frame, iov_size() returns the
> padded wire length including any bytes added to reach the Ethernet
> minimum frame size of 60 bytes.  net_tx_pkt_rebuild_payload() uses
> this inflated size as payload_len.  In the non-TSO IXSM path,
> net_tx_pkt_update_ip_hdr_checksum() then overwrites the IPv4 Total
> Length field with payload_len + l3_hdr_len, inflating it by the
> padding bytes.  The receiver interprets Ethernet padding as IP
> payload, producing a malformed packet.
> 
> Fix the IXSM path in e1000e by adding a new function,
> net_tx_pkt_update_ip_hdr_checksum_only(), that recomputes the IPv4
> header checksum without modifying ip_len.  For non-TSO packets the
> guest has already set ip_len correctly; only the checksum needs to be
> recomputed after the header is reassembled into the linear buffer.
> A new common-layer function is needed because struct NetTxPkt is
> opaque to device code and the IP header bytes required by
> net_raw_checksum() are inaccessible from e1000e_core.c.

Now we have a better picture of the issue. I think 
net_tx_pkt_update_ip_hdr_checksum() should not modify ip_len but only 
update checksum. The function has three callers:

- e1000e_setup_tx_offloads()
- igb_setup_tx_offloads()
- net_tx_pkt_update_ip_checksums()

igb has effectively the same requirement with e1000e, so 
net_tx_pkt_update_ip_checksums() is the exceptional one that needs 
modifying ip_len. So we can move the logic to modify ip_len to 
net_tx_pkt_update_ip_checksums(), and avoid having somewhat confusing 
two similar functions: net_tx_pkt_update_ip_hdr_checksum() and 
net_tx_pkt_update_ip_hdr_checksum_only(). Eventually we will have both 
e1000e and igb corrected just with a change of a few lines.

Regards,
Akihiko Odaki

> 
> The TSO path is unaffected: it continues to call
> net_tx_pkt_update_ip_checksums() which correctly writes ip_len from
> payload_len (ip_len=0 per Intel 82574 datasheet §7.3.4 for TSO
> super-packets that the host must segment).
> 
> Signed-off-by: Sanjeeva Yerrapureddy <y.sanjeevreddy@gmail.com>
> ---
> Thank you Akihiko for your review. I agree that device-specific logic
> should stay in device code and not in net_tx_pkt_rebuild_payload(). v4
> takes a different approach that avoids any changes to the common
> parsing path.
> 
> Changes in v4:
> - Revert all changes to net_tx_pkt_rebuild_payload(); it is unchanged
>    from upstream.
> - Add net_tx_pkt_update_ip_hdr_checksum_only() to the net_tx_pkt
>    common layer: identical to net_tx_pkt_update_ip_hdr_checksum() but
>    skips the ip_len write. A new common-layer function is necessary
>    because struct NetTxPkt is opaque to device code — the IP header
>    bytes needed by net_raw_checksum() are inaccessible from e1000e_core.c.
> - Change the IXSM (non-TSO) path in e1000e_setup_tx_offloads() to call
>    net_tx_pkt_update_ip_hdr_checksum_only(). The guest has already set
>    ip_len correctly; we only need to recompute the checksum.
> - The TSO path is unaffected: it continues to call
>    net_tx_pkt_update_ip_checksums() which correctly writes ip_len from
>    payload_len (ip_len=0 per §7.3.4 for TSO super-packets).
> - Link to v3: https://lore.kernel.org/qemu-devel/20260627-net-tx-pkt-ip-length-padding-v3-1-e860dabadd3f@gmail.com
> 
> Changes in v3:
> - Architectural redesign: three-case fallback logic in
>    net_tx_pkt_rebuild_payload() plus net_tx_pkt_set_payload_len() API;
>    e1000e pre-sets payload_len from PAYLEN descriptor field.
> - Link to v2: https://lore.kernel.org/qemu-devel/20260625-net-tx-pkt-ip-length-padding-v2-1-287d0ba59ec1@gmail.com
> 
> Changes in v2:
> - Fix iov_copy() to use raw_payload_len and net_tx_pkt_get_total_len()
>    to use iov_size() to preserve correct wire frame size.
> - Link to v1: https://lore.kernel.org/qemu-devel/20260624-net-tx-pkt-ip-length-padding-v1-1-7a22fe9b4e40@gmail.com
> 
> On the relationship to the short-packet receive-path discussion
> (Peter Maydell):
> https://lore.kernel.org/qemu-devel/CAFEAcA_UhmCxJc16CHE=4ZR1+PA0=4-29=fEe+d+iUmhLTzpuQ@mail.gmail.com/
> 
> The linked thread concerns the receive path. This patch addresses only
> the transmit path: the IXSM offload path was overwriting a correctly
> set guest ip_len with an inflated value that included Ethernet padding.
> No changes to padding behaviour or the receive path are made.
> ---
>   hw/net/e1000e_core.c |  8 +++++++-
>   hw/net/net_tx_pkt.c  | 17 +++++++++++++++++
>   hw/net/net_tx_pkt.h  | 13 +++++++++++++
>   3 files changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
> index 46e156a5dd..3c0347caf8 100644
> --- a/hw/net/e1000e_core.c
> +++ b/hw/net/e1000e_core.c
> @@ -614,7 +614,13 @@ e1000e_setup_tx_offloads(E1000ECore *core, struct e1000e_tx *tx)
>       }
>   
>       if (tx->sum_needed & E1000_TXD_POPTS_IXSM) {
> -        net_tx_pkt_update_ip_hdr_checksum(tx->tx_pkt);
> +        /*
> +         * Guest already set ip_len correctly; only recompute the checksum.
> +         * net_tx_pkt_update_ip_hdr_checksum() would overwrite ip_len with
> +         * payload_len + l3_hdr_len, inflating it by any Ethernet
> +         * minimum-frame padding bytes present in payload_len.
> +         */
> +        net_tx_pkt_update_ip_hdr_checksum_only(tx->tx_pkt);
>       }
>   
>       return true;
> diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c
> index 903238dca2..c2eae2f74b 100644
> --- a/hw/net/net_tx_pkt.c
> +++ b/hw/net/net_tx_pkt.c
> @@ -102,6 +102,23 @@ void net_tx_pkt_update_ip_hdr_checksum(struct NetTxPkt *pkt)
>       pkt->l3_hdr.ip.ip_sum = cpu_to_be16(csum);
>   }
>   
> +void net_tx_pkt_update_ip_hdr_checksum_only(struct NetTxPkt *pkt)
> +{
> +    uint16_t csum;
> +    assert(pkt);
> +
> +    /*
> +     * Recompute the IP header checksum without touching ip_len.
> +     * Used for non-TSO packets where the guest already set ip_len
> +     * correctly; overwriting it would inflate it by any Ethernet
> +     * minimum-frame padding present in payload_len.
> +     */
> +    pkt->l3_hdr.ip.ip_sum = 0;
> +    csum = net_raw_checksum(pkt->l3_hdr.octets,
> +        pkt->vec[NET_TX_PKT_L3HDR_FRAG].iov_len);
> +    pkt->l3_hdr.ip.ip_sum = cpu_to_be16(csum);
> +}
> +
>   void net_tx_pkt_update_ip_checksums(struct NetTxPkt *pkt)
>   {
>       uint16_t csum;
> diff --git a/hw/net/net_tx_pkt.h b/hw/net/net_tx_pkt.h
> index 0a716e74a5..d2840f38e8 100644
> --- a/hw/net/net_tx_pkt.h
> +++ b/hw/net/net_tx_pkt.h
> @@ -133,6 +133,19 @@ bool net_tx_pkt_update_sctp_checksum(struct NetTxPkt *pkt);
>    */
>   size_t net_tx_pkt_get_total_len(struct NetTxPkt *pkt);
>   
> +/**
> + * recompute the IPv4 header checksum without modifying ip_len
> + *
> + * Use for non-TSO packets where the guest already set ip_len correctly.
> + * Calling net_tx_pkt_update_ip_hdr_checksum() instead would overwrite
> + * ip_len with payload_len + l3_hdr_len, inflating it by any Ethernet
> + * minimum-frame padding present in payload_len.
> + *
> + * @pkt:            packet
> + *
> + */
> +void net_tx_pkt_update_ip_hdr_checksum_only(struct NetTxPkt *pkt);
> +
>   /**
>    * get packet type
>    *
> 
> ---
> base-commit: b83371668192a705b878e909c5ae9c1233cbd5fb
> change-id: 20260628-net-tx-pkt-ip-length-padding-951e75fa97b4
> 
> Best regards,
> --
> Sanjeeva Yerrapureddy <y.sanjeevreddy@gmail.com>
>