Michael S. Tsirkin wrote:
> As reported first by Paulos Yibelo <[email protected]> (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.
>
> Reported-by: Paulos Yibelo <[email protected]>
> Reported-by: Chris Arges <[email protected]>
> Tested-by: Chris Arges <[email protected]>
> Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
> Suggested-by: Willem de Bruijn <[email protected]>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
A few comments inline. All but one cosmetic. Feel free to ignore
those. Please do review the comment from an LLM about the tunnel.
Aside from that LGTM.
> +static inline struct iphdr *virtio_net_ip4_parse(struct sk_buff *skb,
> + unsigned int nhoff,
> + unsigned int *thoff)
> +{
> + struct iphdr *iph;
> +
> + if (!pskb_may_pull(skb, nhoff + sizeof(struct iphdr)))
> + return NULL;
> +
> + iph = (struct iphdr *)(skb->data + nhoff);
> + if (iph->version != 4 ||
> + iph->ihl < sizeof(struct iphdr) / sizeof(u32))
> + return NULL;
> +
> + *thoff = nhoff + iph->ihl * sizeof(u32);
> + /* Pull the options as well: the transport header offset
> + * returned above must be within the linear data. iph goes
> + * stale across the pull.
> + */
> + if (!pskb_may_pull(skb, *thoff))
perhaps a local var instead of dereference the pointer here.
though the compiler probably optimizes that anyway (?)
> + return NULL;
> +
> + return (struct iphdr *)(skb->data + nhoff);
> +}
> +
> +/* Walk the L3 header at @nhoff: where does its transport header
> + * start, and which protocol is it? Returns false if the header
> + * is truncated, malformed or fragmented.
> + */
> +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;
Since ipv6_find_hdr is called with target -1, any non extension
header might pass. Do we want to restrict here to IPPROTO_TCP and
IPPROTO_UDP again? No other transport layer requests csum offload.
> static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> const struct virtio_net_hdr *hdr,
> bool little_endian)
> {
> - return __virtio_net_hdr_to_skb(skb, hdr, little_endian, hdr->gso_type);
> + typeof(skb->transport_header) old_transport = skb->transport_header;
> + int ret = __virtio_net_hdr_to_skb(skb, hdr, little_endian,
> + hdr->gso_type);
> +
> + if (ret < 0)
> + return ret;
> +
> + if (skb->ip_summed != CHECKSUM_PARTIAL ||
> + virtio_net_csum_partial_ok(skb, hdr->gso_type))
> + return 0;
This is the common hotpath, but default branch prediction is to
predict if without else as not-taken. Probably not very important on
modern server CPUs, but optionally add likely. Same below in the
tunnel code.
> +
> + /*
> + * Here we do not trust the checksum metadata.
> + * __virtio_net_hdr_to_skb set the transport header to csum_start
> + * from virtio hdr, restore it to a known good value.
> + */
> + skb->transport_header = old_transport;
> +
> + return __virtio_net_resolve_csum_partial_mismatch(skb);
> }
>
> static inline int
> virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
> const struct virtio_net_hdr_v1_hash_tunnel *vhdr,
> @@ -293,6 +558,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
> unsigned int inner_nh, outer_th, inner_th;
> unsigned int inner_l3min, outer_l3min;
> u8 gso_inner_type, gso_tunnel_type;
> + __be16 inner_proto;
> bool outer_isv6, inner_isv6;
> int ret;
>
> @@ -335,9 +601,12 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
> /* Let the basic parsing deal with plain GSO features. */
> ret = __virtio_net_hdr_to_skb(skb, hdr, true,
> hdr->gso_type & ~gso_tunnel_type);
> - if (ret)
> + if (ret < 0)
> return ret;
>
> + if (!virtio_net_tnl_offsets_ok(skb, outer_isv6, outer_th))
> + return -EINVAL;
> +
> /* In case of USO, the inner protocol is still unknown and
> * `inner_isv6` is just a guess, additional parsing is needed.
> * The previous validation ensures that accessing an ipv4 inner
> @@ -352,8 +621,8 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
> return -EINVAL;
> }
>
> - skb_set_inner_protocol(skb, inner_isv6 ? htons(ETH_P_IPV6) :
> - htons(ETH_P_IP));
> + inner_proto = inner_isv6 ? htons(ETH_P_IPV6) : htons(ETH_P_IP);
> + skb_set_inner_protocol(skb, inner_proto);
> if (hdr->flags & VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM) {
> if (!tnl_csum_negotiated)
> return -EINVAL;
> @@ -368,7 +637,20 @@ 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;
> - return 0;
> +
> + if (!skb_is_gso(skb))
> + skb_shinfo(skb)->gso_type &= ~(SKB_GSO_UDP_TUNNEL_CSUM |
> + SKB_GSO_UDP_TUNNEL);
> +
> + /*
> + * We know VIRTIO_NET_HDR_F_NEEDS_CSUM was set so
> __virtio_net_hdr_to_skb
> + * set CHECKSUM_PARTIAL.
> + */
> + if (virtio_net_csum_partial_ok_at_offset(skb, gso_inner_type, inner_nh,
> + inner_proto))
> + return 0;
> +
> + return __virtio_net_resolve_csum_partial_mismatch(skb);
Allowing `!skb_is_gso(skb)` packets through `virtio_net_hdr_tnl_to_skb()`
has a few issues:
1. When `!skb_is_gso(skb)` and `virtio_net_csum_partial_ok_at_offset()`
returns false, `__virtio_net_resolve_csum_partial_mismatch()` completes
the checksum in software and lets the packet pass, but
`skb->encapsulation`,
`skb->inner_protocol`, `skb->inner_transport_header`,
`skb->inner_network_header`, and `skb->inner_mac_header` have already
been populated with the unvalidated `inner_th` (`csum_start`) and
`inner_nh` offsets (unlike `virtio_net_hdr_to_skb()`, which restores
`skb->transport_header`).
2. If `!skb_is_gso(skb)` and `VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM` is set,
clearing `SKB_GSO_UDP_TUNNEL_CSUM` drops the outer UDP checksum request,
since a non-GSO skb can only carry a single `CHECKSUM_PARTIAL` offset
(at `inner_th`).
3. On RX, a non-GSO UDP packet with `CHECKSUM_PARTIAL` at `inner_th` causes
`skb_csum_unnecessary()` (`skb_checksum_start_offset(skb) >= 0`) to treat
the outer UDP checksum as already validated.
Unlike plain GSO (commit 7c6d2ecbda83), UDP tunnel offload is modern-only
and
explicitly requires GSO (`/* The UDP tunnel must carry a GSO packet, but no
UFO. */`).
Why not reject `!skb_is_gso(skb)` right after `__virtio_net_hdr_to_skb()`,
and
return `-EINVAL` directly if `!virtio_net_csum_partial_ok_at_offset()`?
