Michael S. Tsirkin wrote:
As reported first by Paulos Yibelo [off-list ref] (after which
others, credited below, found more ways the bug can affect users) a
virtio net header can place CHECKSUM_PARTIAL at any in-packet offset.
If that state is retained, later checksum completion can modify the
packet after protocol processing has made decisions from it.
Let's fix that: let's validate csum_start and csum_offset and only keep
CHECKSUM_PARTIAL when csum_start and csum_offset match a parsed TCP or
UDP header, while handling VLAN headers, IPv4 options and IPv6 extension
headers. Complete other non-GSO checksums immediately, before any
protocol processing runs.
We can't easily "complete the checksum" for GSO we don't recognize - GSO
assumes knowledge of the protocol. We could segment ignoring the
checksum then do whatever csum_start and csum_offset say, but that
seems like an overkill for something that shouldn't happen.
There's some risk here:
- non-gso packets which can't be parsed get checksum calculated in
software, adding performance overhead
- gso packets which can't be parsed get dropped
Note that for zero copy, userspace can change the data after the
checksum has been calculated. This is fundamental to zero copy by
design, and the only outcome is a bad checksum and a packet drop.
To mitigate the risks, the following patch will add a sysctl knob to
bypass the resolution. This is reasonable if the setup is such that
there is no protocol processing at all.
UDP tunnel headers get the same handling on the inner header.
Packets without VIRTIO_NET_HDR_F_NEEDS_CSUM are unaffected.
Note a fundamental limitation of this approach: an nf/BPF program
that tweaks the payload of a packet in flight may rely on the
checksum being fixed up later, assuming the packet carries
CHECKSUM_PARTIAL.
For metadata that fails validation the checksum
is now completed at input, so such edits leave a stale checksum
and the packet is dropped at the receiver. Protocols whose
transport header we cannot parse (anything but TCP and UDP) are
affected even when the metadata is self-consistent.
The sysctl added in the next patch is the escape hatch for these setups,
but it is a very blunt tool.
Reported-by: Paulos Yibelo <redacted>
Reported-by: Chris Arges <redacted>
Tested-by: Chris Arges <redacted>
Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
quoted hunk ↗ jump to hunk
+static inline bool
+virtio_net_l4_start(struct sk_buff *skb, unsigned int nhoff, __be16 proto,
+ unsigned int *thoff, int *ip_proto)
+{
+ if (proto == htons(ETH_P_IP)) {
+ const struct iphdr *iph = virtio_net_ip4_parse(skb, nhoff, thoff);
+
+ if (!iph || ip_is_fragment(iph))
+ return false;
+ *ip_proto = iph->protocol;
+ return true;
+ }
+
+ if (proto == htons(ETH_P_IPV6)) {
+ const struct ipv6hdr *iph;
+ int flags = 0;
+
+ if (!pskb_may_pull(skb, nhoff + sizeof(*iph)))
+ return false;
+ iph = (const void *)(skb->data + nhoff);
+ if (iph->version != 6)
+ return false;
+ *thoff = nhoff + sizeof(*iph);
+ *ip_proto = iph->nexthdr;
+ if (*ip_proto != IPPROTO_TCP && *ip_proto != IPPROTO_UDP) {
+ *thoff = nhoff;
+ /* Find the transport header behind any
+ * extension headers, AH included.
+ */
+ *ip_proto = ipv6_find_hdr(skb, thoff, -1, NULL,
+ &flags);
+ if (*ip_proto < 0 || flags & IP6_FH_F_FRAG)
+ return false;
Minor, not cause for respinning on its own.
`flags` should include `IP6_FH_F_AUTH` so `ipv6_find_hdr()` stops at
`NEXTHDR_AUTH` instead of skipping it:
- `CHECKSUM_PARTIAL` and GSO behind IPsec AH are invalid: AH authenticates
the inner L4 header including its checksum field
quoted hunk ↗ jump to hunk
+ }
+ return true;
+ }
+
+ return false;
+}
+
quoted hunk ↗ jump to hunk
@@ -368,6 +660,21 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
skb->inner_mac_header = inner_nh + skb_headroom(skb);
skb->transport_header = outer_th + skb_headroom(skb);
skb->encapsulation = 1;
+
+ if (!skb_is_gso(skb)) {
+ /* Too small to segment: complete the checksum in place
+ * instead of dropping - senders do tag small packets
+ * with GSO headers, e.g. packetdrill sets gso_type and
+ * gso_size with --mss, whatever the packet size.
+ * The outer UDP checksum is not calculated, but a zero
+ * one is fine: UDP over IPv4 allows a disabled
+ * checksum, and so do tunnels over IPv6 where the
+ * receiver opts in (RFC 6935, RFC 6936).
+ */
+ skb_shinfo(skb)->gso_type &= ~(SKB_GSO_UDP_TUNNEL_CSUM |
+ SKB_GSO_UDP_TUNNEL);
Maybe this carve-out for non-gso packet is best moved to its own
patch, as that is where the remaining issues (to merging the rest)
seem to be.
1. Moving `!tnl_csum_negotiated` below `virtio_net_csum_partial_ok_at_offset()`
accepts packets with an unnegotiated `VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM`
flag whenever inner validation falls back to
`__virtio_net_resolve_csum_partial_mismatch()`.
2. `__virtio_net_resolve_csum_partial_mismatch()` calls `skb_checksum_help()`,
which completes only the inner checksum at `hdr->csum_start` and sets
`skb->ip_summed = CHECKSUM_NONE`, leaving the outer UDP `uh->check` with
an uncompleted pseudo-header checksum seed.
Despite the comment, this block neither completes any checksum in place nor
is the outer UDP checksum zero when `VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM` is set:
- With `VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM`, the outer `uh->check` carries the
non-zero pseudo-header checksum seed (as expected by
`__skb_udp_tunnel_segment()`), not `0`.
- Clearing `SKB_GSO_UDP_TUNNEL_CSUM` on a `!skb_is_gso(skb)` packet leaves
`skb->ip_summed == CHECKSUM_PARTIAL` at `inner_th` with nothing completing
the outer `uh->check` on transmit/forwarding (`validate_xmit_skb()` only
completes/offloads the single inner checksum, emitting a corrupt outer UDP
checksum). Even zeroing `uh->check` would be invalid over IPv6 unless the
receiver explicitly opted into RFC 6936 (`UDP_NO_CHECK6_RX`).
+ }
+
return 0;
}
--
MST