On Thu, May 29, 2025 at 7:55 PM Paolo Abeni [off-list ref] wrote:

On 5/26/25 6:40 AM, Jason Wang wrote:

quoted

On Wed, May 21, 2025 at 6:34 PM Paolo Abeni [off-list ref] wrote:

quoted

+       if (!gso_inner_type || gso_inner_type == VIRTIO_NET_HDR_GSO_UDP)
+               return -EINVAL;
+
+       /* Relay on csum being present. */
+       if (!(hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM))
+               return -EINVAL;
+
+       /* Validate offsets. */
+       outer_isv6 = gso_tunnel_type & VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV6;
+       inner_l3min = virtio_l3min(gso_inner_type == VIRTIO_NET_HDR_GSO_TCPV6);
+       outer_l3min = ETH_HLEN + virtio_l3min(outer_isv6);
+
+       tnl = ((void *)hdr) + tnl_hdr_offset;
+       inner_th = __virtio16_to_cpu(little_endian, hdr->csum_start);
+       inner_nh = __virtio16_to_cpu(little_endian, tnl->inner_nh_offset);
+       outer_th = __virtio16_to_cpu(little_endian, tnl->outer_th_offset);
+       if (outer_th < outer_l3min ||
+           inner_nh < outer_th + sizeof(struct udphdr) ||
+           inner_th < inner_nh + inner_l3min)
+               return -EINVAL;

I wonder if kernel has already had helpers to validate the tunnel
headers

Not that I know of.

quoted

or if the above check is sufficient here.

AFAICS yes. Syzkaller is out there just to prove me wrong...

quoted

quoted

+
+       /* Let the basic parsing deal with plain GSO features. */
+       ret = __virtio_net_hdr_to_skb(skb, hdr, little_endian,
+                                     hdr->gso_type & ~gso_tunnel_type);
+       if (ret)
+               return ret;
+
+       skb_set_inner_protocol(skb, outer_isv6 ? htons(ETH_P_IPV6) :
+                                                htons(ETH_P_IP));

The outer_isv6 is somehow misleading here, I think we'd better rename
it as inner_isv6?

There is bug above, thanks for spotting it. I should not use the
`outer_isv6` variable, instead I should compute separately `inner_isv6`

quoted

quoted

+       if (hdr->flags & VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM) {
+               if (!tnl_csum_negotiated)
+                       return -EINVAL;
+
+               skb_shinfo(skb)->gso_type |= SKB_GSO_UDP_TUNNEL_CSUM;
+       } else {
+               skb_shinfo(skb)->gso_type |= SKB_GSO_UDP_TUNNEL;
+       }
+
+       skb->inner_transport_header = inner_th + skb_headroom(skb);

I may miss something but using skb_headroom() means the value depends
on the geometry of the skb and the headroom might vary depending on
the size of the packet and other factors.  (see receive_buf())

Yes, that is correct: the actual inner_transport_header value depends on
the skb geometry, because the (inner) transport header is located at
skb->head + skb->inner_transport_header.

Right, I see. Btw, is skb_set_inner_transport_header() considered to
be better here?

quoted

quoted

+       skb->inner_network_header = inner_nh + skb_headroom(skb);
+       skb->inner_mac_header = inner_nh + skb_headroom(skb);

This actually equals to inner_network_header, is this intended?

Yes. AFAICS the inner mac header field is used only for GSO/TSO.

At this point we don't know if the inner mac header is actually present
nor it's len (could include vlan tag).

Still the above allows correct segmentation by the GSO stage because the
inner mac header is not copied verbatim in the segmented packets, alike
the tunnel header.

With the above code, the inner mac header if really present will be
logically considered part of the tunnel header by the GSO stage.

Note that some devices restrict the TSO capability to some fixed values
of the UDP tunnel sizes and inner mac header. In such cases, they will
fallback to S/W GSO.

Ok.

quoted

quoted

+       skb->transport_header = outer_th + skb_headroom(skb);
+       skb->encapsulation = 1;
+       return 0;
+}
+
+static inline int virtio_net_chk_data_valid(struct sk_buff *skb,
+                                           struct virtio_net_hdr *hdr,
+                                           bool tun_csum_negotiated)

This is virtio_net.h so it's better to avoid using "tun". Btw, I
wonder why this needs to be called by the virtio-net instead of being
called by hdr_to_skb helpers.

I can squash into virtio_net_hdr_tnl_to_skb(), I kept them separated to
avoid extra long argument lists, but we are dropping an argument from
virtio_net_hdr_tnl_to_skb(), so should be ok.

quoted

quoted

+{
+       if (!(hdr->gso_type & VIRTIO_NET_HDR_GSO_UDP_TUNNEL)) {
+               if (!(hdr->flags & VIRTIO_NET_HDR_F_DATA_VALID))
+                       return 0;
+
+               skb->ip_summed = CHECKSUM_UNNECESSARY;
+               if (!(hdr->flags & VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM))
+                       return 0;
+
+               /* tunnel csum packets are invalid when the related
+                * feature has not been negotiated
+                */
+               if (!tun_csum_negotiated)
+                       return -EINVAL;

Should we move this check above VIRTIO_NET_HDR_F_DATA_VALID check?

It could break existing setups. We can safely do extra validation only
when we know that the UDP tunnel features have been negotiated.

You are right.

quoted

quoted

+               skb->csum_level = 1;
+               return 0;
+       }
+
+       /* DATA_VALID is mutually exclusive with NEEDS_CSUM,

I may miss something but I think we had a discussion about this, and
the conclusion is it's too late to fix as it may break some legacy
devices?

I'm not sure what should be fixed here? This check implements exactly
restriction you asked for while discussing the spec. We can't have a
similar check for non UDP tunneled packets, because it could break
existing setup.

Right.

quoted

quoted

and GSO
+        * over UDP tunnel requires the latter
+        */
+       if (hdr->flags & VIRTIO_NET_HDR_F_DATA_VALID)
+               return -EINVAL;
+       return 0;
+}
+
+static inline int virtio_net_hdr_tnl_from_skb(const struct sk_buff *skb,
+                                             struct virtio_net_hdr *hdr,
+                                             unsigned int tnl_offset,
+                                             bool little_endian,
+                                             int vlan_hlen)
+{
+       struct virtio_net_hdr_tunnel *tnl;
+       unsigned int inner_nh, outer_th;
+       int tnl_gso_type;
+       int ret;
+
+       tnl_gso_type = skb_shinfo(skb)->gso_type & (SKB_GSO_UDP_TUNNEL |
+                                                   SKB_GSO_UDP_TUNNEL_CSUM);
+       if (!tnl_gso_type)
+               return virtio_net_hdr_from_skb(skb, hdr, little_endian, false,
+                                              vlan_hlen);
+
+       /* Tunnel support not negotiated but skb ask for it. */
+       if (!tnl_offset)
+               return -EINVAL;

Should we do BUG_ON here?

I don't think so. BUG_ON()s are explicitly discouraged to avoid crashing
the kernel on exceptional/unexpected situation.

The caller will emit rate limited warns with the relevant info, if this
is hit. The BUG_ON() stack trace will add little value.

Ok.

quoted

quoted

+
+       /* Let the basic parsing deal with plain GSO features. */
+       skb_shinfo(skb)->gso_type &= ~tnl_gso_type;
+       ret = virtio_net_hdr_from_skb(skb, hdr, little_endian, false,
+                                     vlan_hlen);
+       skb_shinfo(skb)->gso_type |= tnl_gso_type;
+       if (ret)
+               return ret;

Could we do the plain GSO after setting inner flags below to avoid
masking and unmasking tnl_gso_type?

virtio_net_hdr_from_skb() will still receive a skb with UDP tunnel GSO
type and will error out.

The masking coudl be avoided factoring out a __virtio_net_hdr_from_skb()
helper receiving an explicit gso_type argument. I can do that if it's
preferred.

That would be fine.

Thanks

/P