[PATCH] net: validate checksum start before software fallback on transmit

[PATCH] net: validate checksum start before software fallback on transmit

[Dmitrii Iakunin]

Commit 6785011f8b16 ("ipv4: validate checksum_start before completing
checksum") added checks to prevent malformed checksum metadata from
causing software checksum completion to corrupt the IPv4 header.

skb_csum_hwoffload_help() can also complete checksums in software when
the available device features cannot satisfy the checksum request.
This path was not covered by the earlier validation.

For IPv4 packets, ensure that the fixed network header is accessible
and validate the checksum start before calling skb_checksum_help().
Return -EINVAL on failure so callers discard the packet rather than
complete the checksum using invalid metadata.

Leave hardware-offloaded checksums and SCTP CRC handling unchanged.

Fixes: 6785011f8b16 ("ipv4: validate checksum_start before completing checksum")
Signed-off-by: Dmitrii Iakunin <zeil@nebius.com>
---
 net/core/dev.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index 3ac700d37e59..77c40e914e5f 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4130,6 +4130,11 @@ int skb_csum_hwoffload_help(struct sk_buff *skb,
 	}
 
 sw_checksum:
+	if (vlan_get_protocol(skb) == htons(ETH_P_IP)) {
+		if (!pskb_network_may_pull(skb, sizeof(struct iphdr)) ||
+		    ip_check_csum_start(skb))
+			return -EINVAL;
+	}
 	return skb_checksum_help(skb);
 }
 EXPORT_SYMBOL(skb_csum_hwoffload_help);
-- 
2.52.0

Re: [PATCH] net: validate checksum start before software fallback on transmit

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH] net: validate checksum start before software fallback on transmit

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 2 · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The new check reads the IPv4 header from a different place than
  the one it used to decide the packet is IPv4.
- [High] net: missing iph->ihl >= 5 check in skb_csum_hwoffload_help()
- [Medium] The patch is narrower than its subject.
- [Medium] On the PPP channel-bridge path, the new guard drops packets
  that used to be forwarded.

--

net: validate checksum start before software fallback on transmit

This patch extends the IPv4 csum_start validation from 6785011f8b16 to the
sw_checksum path of skb_csum_hwoffload_help(). For IPv4 packets it pulls
the fixed IPv4 header and calls ip_check_csum_start(). On failure it
returns -EINVAL so the caller drops the packet.

> diff --git a/net/core/dev.c b/net/core/dev.c
> index f32353b0467cd..3e737ba094968 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4130,6 +4130,11 @@ int skb_csum_hwoffload_help(struct sk_buff *skb,
>  	}
>  
>  sw_checksum:
> +	if (vlan_get_protocol(skb) == htons(ETH_P_IP)) {

[Severity: High]

Can the protocol check and the header read disagree here?

vlan_get_protocol() walks every in-band 802.1Q/802.1AD tag and returns the
innermost EtherType. pskb_network_may_pull() and ip_check_csum_start()
both read at skb->network_header instead:

include/net/ip.h:ip_check_csum_start() {
	if (unlikely(skb->csum_start < skb->network_header + ip_hdrlen(skb)))
		return -EINVAL;
	...
}

The sites added by 6785011f8b16 gate on skb->protocol or on the netfilter
family. For example, nf_queue_checksum_help() checks
entskb->protocol == htons(ETH_P_IP). With those checks, the protocol and
network_header always agree.

They seem to diverge for a QinQ frame that arrives with CHECKSUM_PARTIAL
(from a tap/virtio guest with NEEDS_CSUM, or from a veth peer) and is then
redirected by tc mirred or bpf_redirect:

__netif_receive_skb_core()
  skb_vlan_untag()
    pops only the outer tag
    skb_reset_network_header()   <- now points at the inner vlan_hdr
  sch_handle_ingress()
    tcf_mirred_to_dev() or bpf_redirect   <- pushes mac_len only
      __dev_queue_xmit()
        validate_xmit_skb()
          vlan_features_check()   <- drops NETIF_F_IP_CSUM, multi-tagged
          skb_csum_hwoffload_help()
            sw_checksum:
              vlan_get_protocol() == ETH_P_IP
              ip_check_csum_start()   <- ihl read from the inner TCI

At that point ip_hdr(skb)->ihl is the low nibble of the first byte of the
inner TCI, which is VID bits 11..8.

Take a correctly formed TCP packet with csum_start == network_header + 24.
If the inner VID is 0x700 or higher, ihl is 7 or more. ip_check_csum_start()
then fails and validate_xmit_skb() drops a packet that skb_checksum_help()
used to complete correctly. Was this drop intended?

The reverse also seems possible. With an inner VID that gives ihl 0 or 1,
a csum_start pointing into the real IPv4 header passes the check, and the
header overwrite still happens.

The bridge path looks unaffected, because br_dev_queue_push_xmit() resets
network_header to the VLAN depth for CHECKSUM_PARTIAL frames. The tc and
bpf redirect paths have no such fix-up.

Would it help to gate on skb->protocol like the other sites? Another
option is to locate the IPv4 header at the depth returned by
vlan_get_protocol_and_depth().

> +		if (!pskb_network_may_pull(skb, sizeof(struct iphdr)) ||

[Severity: Medium]

What happens here when skb_network_offset() is negative?

Take L2TP over UDP/IPv4 on the PPP channel-bridge path. ip_rcv() sets
skb->protocol to ETH_P_IP and points network_header at the outer IPv4
header. The outer IPv4, UDP and L2TP headers (and optionally FF03) are
then pulled, and nothing resets network_header or protocol:

l2tp_udp_encap_recv()
  __skb_pull(skb, sizeof(struct udphdr))
  l2tp_recv_common()
    pppol2tp_recv()
      ppp_input()
        ppp_channel_bridge_input()   <- keeps CHECKSUM_PARTIAL when
                                        csum_start >= skb->data
          ppp_bridge_xmit()
            skb_csum_hwoffload_help(skb, 0)   <- !chan->xmit_offload

With features set to 0 this reaches sw_checksum, and vlan_get_protocol()
returns ETH_P_IP.

pskb_network_may_pull_reason() then does:

	return pskb_may_pull_reason(skb, skb_network_offset(skb) + len);

With an offset of -34 or less and len of 20, the unsigned sum wraps to
about 0xFFFFFFF2. On CONFIG_DEBUG_NET kernels this trips
DEBUG_NET_WARN_ON_ONCE(len > INT_MAX) in pskb_may_pull_reason(). The
function returns PKT_TOO_SMALL either way, so this path returns -EINVAL
and ppp_bridge_xmit() frees the skb.

Before this patch, skb_checksum_help() completed the inner checksum and
the packet was forwarded. Is dropping these packets expected?

Even if the pull succeeded, ip_hdr() would point at the outer header that
was already stripped, not at the header of the packet being checksummed.
PPTP-received packets bridged to L2TP or PPTP channels look affected in
the same way.

> +		    ip_check_csum_start(skb))
> +			return -EINVAL;
> +	}
>  	return skb_checksum_help(skb);
>  }
>  EXPORT_SYMBOL(skb_csum_hwoffload_help);

[Severity: High]
Does ip_check_csum_start() protect the header here when ihl itself has
not been validated?

ip_check_csum_start() takes its lower bound from the packet:

	skb->csum_start < skb->network_header + ip_hdrlen(skb)

ip_hdrlen() is ip_hdr(skb)->ihl * 4. The sites added by 6785011f8b16,
ip_do_fragment() and xfrm_output(), only see headers that ip_rcv_core()
or the IPv4 output path already checked for ihl >= 5. Nothing on the
paths into skb_csum_hwoffload_help() does that. pskb_network_may_pull()
only makes 20 bytes readable and does not look at ihl.

Packets built by AF_PACKET with PACKET_VNET_HDR (packet_snd(),
tpacket_snd()) or by tap_get_user() with VIRTIO_NET_HDR_F_NEEDS_CSUM
reach this helper with an ihl chosen by the sender. They can arrive via
validate_xmit_skb(), ieee80211_tx_skb_fixup() or the openvswitch
queue_userspace_packet() path.

For VIRTIO_NET_HDR_GSO_NONE, __virtio_net_hdr_to_skb() only requires

	skb_transport_offset(skb) >= sizeof(struct iphdr)

That is measured from skb->data, which still points at the L2 header.
The lowest accepted csum_start therefore depends on the tagging:

  - untagged Ethernet: 6 bytes into the IPv4 header
  - one 802.1Q tag: 2 bytes in
  - QinQ: the network header itself

With ihl between 0 and 4, ip_hdrlen() is 0 to 16. A csum_start inside
the fixed 20-byte header then passes ip_check_csum_start().
skb_checksum_help() writes the folded checksum at
csum_start + csum_offset, which still lands inside struct iphdr.

In the QinQ case, csum_start == network_header with csum_offset == 0
overwrites the version/ihl byte itself. That is the header overwrite
the commit message says this change prevents.

Should this path reject ihl < 5 the way ip_rcv_core() does? Another
option is to compare against max(ip_hdrlen(skb), sizeof(struct iphdr)).
If the helper in include/net/ip.h were changed instead, the existing
callers should not notice, since they already guarantee ihl >= 5.

[Severity: Medium]

The subject says:

  net: validate checksum start before software fallback on transmit

However, the only new check is at the sw_checksum label of
skb_csum_hwoffload_help(). Are the other transmit-side software
completion sites meant to stay unvalidated?

skb_csum_hwoffload_help() returns 0 before sw_checksum, without looking
at csum_start, when the device has NETIF_F_HW_CSUM:

	if (features & NETIF_F_HW_CSUM)
		return 0;

The same early return happens when the device has NETIF_F_IP_CSUM and
csum_offset is 16 or 6.

Several drivers then fall back to software anyway:

  - wireguard encrypt_packet() calls skb_checksum_help() for
    CHECKSUM_PARTIAL skbs.
  - igb_tx_csum() calls skb_checksum_help() in its default case. Other
    Intel drivers follow the same pattern.
  - tg3_start_xmit() calls skb_checksum_help() for frames whose
    skb->protocol is ETH_P_8021Q or ETH_P_8021AD. That is the in-band
    VLAN case this patch handles with vlan_get_protocol().

Some core-stack callers also complete the checksum directly and set
CHECKSUM_NONE, so the skb never reaches this helper afterwards:

  - xsk_skb_metadata() with tx_sw_csum only checks
    csum_start + csum_offset + 2 <= desc->len, which is an upper bound.
  - netem_enqueue() on the corruption path.

On each of these paths, skb_checksum_help() can still write the folded
checksum into the IPv4 header when csum_start points inside it.
6785011f8b16 itself names input-side validation as the eventual complete
fix.

Should these sites be covered too? Otherwise, could the subject and commit
message say the change only covers skb_csum_hwoffload_help()?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102140633.42564-1-zeil%40nebius.com