Paulos Yibelo wrote:
> __virtio_net_hdr_to_skb() checks a minimum network-header length for
> CHECKSUM_PARTIAL packets. Its checksum start is relative to skb->data,
> but some callers have not established skb->network_header when they
> convert the virtio header.
>
> Pass the data-relative L3 origin explicitly. Ethernet receive paths
> parse the frame and nested VLAN headers without changing skb state.
> AF_PACKET uses the frame's actual L3 origin even when the socket
> protocol is ETH_P_IP and the raw frame carries VLAN tags. Non-Ethernet
> AF_PACKET devices retain their established skb network offset.
>
> Also pass the actual L3 protocol so IPv6 packets use the 40-byte base
> header minimum even without TCPv6 GSO. IFF_TUN obtains that protocol
> from the packet before skb->protocol is set. Name the Ethernet parser
> accordingly, use the same origin for tunnel validation, and propagate
> conversion failures in UML.
>
> The bound remains a minimum; fragmentation paths separately validate
> the parsed IPv4 or IPv6 header length before completing a checksum.
>
> Fixes: 49d14b54a527 ("net: test for not too small csum_start in
> virtio_net_hdr_to_skb()")
> Fixes: a2fb4bc4e2a6 ("net: implement virtio helpers to handle UDP GSO
> tunneling.")
> Reported-by: Paulos Yibelo <[email protected]>
> Link:
> https://lore.kernel.org/netdev/[email protected]/
> Cc: [email protected]
> Assisted-by: LLM
> Signed-off-by: Paulos Yibelo <[email protected]>
> ---
> arch/um/drivers/vector_transports.c | 13 ++++-
> drivers/net/tun_vnet.h | 52 ++++++++++++++++-
> drivers/net/virtio_net.c | 10 +++-
> include/linux/virtio_net.h | 87 ++++++++++++++++++++++++-----
> net/packet/af_packet.c | 24 +++++++-
> 5 files changed, 163 insertions(+), 23 deletions(-)
The fix may still miss the case IPv4 packets have options.
This version is a very large patch.
Untested shorter first suggestion by bot, which looks plausible as a
starting point for discussion.
--- a/drivers/net/tun_vnet.h
+++ b/drivers/net/tun_vnet.h
@@ -152,6 +152,9 @@ static inline int tun_vnet_hdr_to_skb(unsigned int
flags, struct sk_buff *skb,
const struct virtio_net_hdr *hdr)
{
+ if ((flags & TUN_TYPE_MASK) == IFF_TUN)
+ skb_reset_network_header(skb);
+
return virtio_net_hdr_to_skb(skb, hdr, tun_is_little_endian(flags));
}
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -71,9 +71,27 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff
*skb,
if (!pskb_may_pull(skb, needed))
return -EINVAL;
+ if (!skb_network_header_was_set(skb) ||
+ (skb->dev && skb->dev->type == ARPHRD_ETHER)) {
+ int nhoff = ETH_HLEN;
+
+ if (unlikely(start < ETH_HLEN + nh_min_len))
+ return -EINVAL;
+ __vlan_get_protocol(skb, eth_hdr(skb)->h_proto, &nhoff);
+ nh_min_len += nhoff;
+ } else {
+ nh_min_len += skb_network_offset(skb);
+ }
+ if (unlikely(start < nh_min_len))
+ return -EINVAL;
+
+ const struct iphdr *iph = (void *)(skb->data + nh_min_len -
sizeof(*iph));
+ if (iph->version == 4)
+ nh_min_len += max_t(u32, iph->ihl * 4, sizeof(*iph)) -
sizeof(*iph);
+ else if (iph->version == 6)
/* ..here I don't trust the initial bug output, but the branch
is clear.. */
+ if (unlikely(start < nh_min_len))
+ return -EINVAL;
+
if (!skb_partial_csum_set(skb, start, off))
return -EINVAL;
- if (skb_transport_offset(skb) < nh_min_len)
- return -EINVAL;
}
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -772,7 +772,9 @@ int ip_do_fragment(struct net *net, struct sock *sk,
struct sk_buff *skb,
if (skb->ip_summed == CHECKSUM_PARTIAL &&
(!(dev->features & NETIF_F_CSUM_MASK) ||
- skb_checksum_help(skb)))
+ skb_checksum_start_offset(skb) < (int)(iph->ihl * 4) ||
+ skb_checksum_help(skb)))
goto fail;
And the summary of the problem:
### 1. What the Problem Is
When userspace (AF_PACKET, TUN/TAP, vhost-net) or a device (virtio_net, UML)
injects a CHECKSUM_PARTIAL packet using struct virtio_net_hdr,
virtio_net.h:47-119 validates hdr->csum_start:
// include/linux/virtio_net.h:107 (added by commit 49d14b54a527)
if (skb_transport_offset(skb) < nh_min_len)
return -EINVAL;
This validation has three bugs:
1. Missing L2 + VLAN offset: skb_transport_offset(skb) (hdr->csum_start) is
an offset from skb->data, which points to the L2 header (14
bytes for Ethernet + 4 * n bytes for 802.1Q/802.1ad VLAN tags) on Ethernet
callers (virtio_net, IFF_TAP, PACKET_SOCK_DGRAM/SOCK_RAW).
Comparing csum_start < 20 allows csum_start to land inside the L2 VLAN tags
(14..21) or inside the L3 header (22..33).
2. Wrong nh_min_len for IPv6 non-GSO: nh_min_len defaults to sizeof(struct
iphdr) (20) and is only raised to sizeof(struct ipv6hdr) (40)
when gso_type is VIRTIO_NET_HDR_GSO_TCPV6. A non-GSO or GSO_UDP_L4 IPv6
packet is only checked against 20 bytes.
3. Ignores IPv4 options (iph->ihl > 5): Even without an L2 header (IFF_TUN),
an IPv4 header with options can be up to 60 bytes (ihl = 15),
allowing csum_start = 20 to point 40 bytes inside the IPv4 options area.
This causes two kernel bugs downstream:
• Bug A (WARN_ONCE / panic_on_warn in skb_checksum_help): With 2 VLAN tags
(22 bytes L2), csum_start = 20 passes 20 >= 20. Once
eth_type_trans() + skb_vlan_untag() pull 22 bytes,
skb_checksum_start_offset(skb) (csum_start - skb_headroom(skb)) becomes -2. In
dev.c:3645, offset >= skb_headlen(skb) promotes signed -2 to 0xfffffffeU,
firing WARN_ONCE(1, ...) and crashing panic_on_warn=1 hosts.
• Bug B (TOCTOU L3 Header Corruption -> OOB Read in ip_do_fragment): With
csum_start = 20, csum_offset = 0 on TAP/AF_PACKET, csum_start
lands at byte 6 of struct iphdr (frag_off) or byte 0 (version/ihl on
double-VLAN frames). In ip_output.c:774 and nf_conntrack_bridge.c:42,
skb_checksum_help(skb) runs before hlen = iph->ihl * 4 is read. The 16-bit
checksum write corrupts iph->ihl (e.g. from 5 [20B] to 15 [60B])
after ip_rcv_core() already validated it, causing ip_do_fragment() to read 60
bytes out-of-bounds from skb->data.
