On Wed, Mar 20, 2024 at 6:20 PM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote:
>
> The virtio spec says:
> If one of the VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options have
> been negotiated:
> If the VIRTIO_NET_F_GUEST_HDRLEN feature has been negotiated,
> and gso_type differs from VIRTIO_NET_HDR_GSO_NONE, the driver
> MUST set hdr_len to a value equal to the length of the headers,
> including the transport header.
>
> If the VIRTIO_NET_F_GUEST_HDRLEN feature has not been negotiated,
> or gso_type is VIRTIO_NET_HDR_GSO_NONE, the driver SHOULD set
> hdr_len to a value not less than the length of the headers,
> including the transport header.
>
> So if the VIRTIO_NET_F_GUEST_HDRLEN feature has been negotiated, the
> hdr->hdr_len should be eth header + ip header + tcp/udp header.
>
> But now:
> hdr->hdr_len = __cpu_to_virtio15(little_endian, skb_headlen(skb));
>
> The skb_headlen() returns the linear space of the skb, not the header
> size that only match the case the VIRTIO_NET_F_GUEST_HDRLEN feature has
> not been negotiated, or gso_type is VIRTIO_NET_HDR_GSO_NONE.
>
> We do not check the feature of the device. This function is a common
> function used by many places. So we do more stricter work whatever
> the features is negotiated.
>
> For the case skb_is_gso(skb) is false, if none of the
> VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options have been negotiated,
> the spec not define the action of setting hdr_len. Here I set it to
> skb_headlen(). If one of the above features have been negotiated, we
> should set a value not less than the length of "eth header + ip header +
> tcp/udp header". So the skb_headlen() also is a valid value.
>
> For the case skb_is_gso(skb) is true, it implies that one of
> VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options MUST have been
> negotiated. If the VIRTIO_NET_F_GUEST_HDRLEN is negotiated, we MUST set
> it to the length of "eth header + ip header + tcp/udp header".
> If the VIRTIO_NET_F_GUEST_HDRLEN is not negotiated, that still be a
> valid value.
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx>
> Reported-by: Spike Du <spiked@xxxxxxxxxx>
> ---
> include/linux/virtio_net.h | 41 ++++++++++++++++++++++++++++----------
> 1 file changed, 31 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index 4dfa9b69ca8d..51d93f9762d7 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -201,24 +201,45 @@ static inline int virtio_net_hdr_from_skb(const struct sk_buff *skb,
>
> if (skb_is_gso(skb)) {
> struct skb_shared_info *sinfo = skb_shinfo(skb);
> + u32 hdrlen;
>
> - /* This is a hint as to how much should be linear. */
> - hdr->hdr_len = __cpu_to_virtio16(little_endian,
> - skb_headlen(skb));
> - hdr->gso_size = __cpu_to_virtio16(little_endian,
> - sinfo->gso_size);
> - if (sinfo->gso_type & SKB_GSO_TCPV4)
> + if (sinfo->gso_type & SKB_GSO_TCPV4) {
> hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
> - else if (sinfo->gso_type & SKB_GSO_TCPV6)
> + hdrlen = tcp_hdrlen(skb) + skb_transport_offset(skb);
So could evil guests give us a 0 hdrlen through this. If yes, it seems
much more dangerous than headlen or we need harden the value as
9181d6f8a2bb32d158de66a84164fac05e3ddd18 did.
> +
> + } else if (sinfo->gso_type & SKB_GSO_TCPV6) {
> hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
> - else if (sinfo->gso_type & SKB_GSO_UDP_L4)
> + hdrlen = tcp_hdrlen(skb) + skb_transport_offset(skb);
> +
> + } else if (sinfo->gso_type & SKB_GSO_UDP_L4) {
> hdr->gso_type = VIRTIO_NET_HDR_GSO_UDP_L4;
> - else
> + hdrlen = sizeof(struct udphdr) + skb_transport_offset(skb);
> +
> + } else {
> return -EINVAL;
> + }
> +
> + /* One of VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options MUST
> + * have been negotiated. If the VIRTIO_NET_F_GUEST_HDRLEN is
> + * negotiated, we MUST set it to the length of "eth header + ip
> + * header + tcp/udp header". If the VIRTIO_NET_F_GUEST_HDRLEN
> + * is not negotiated, that still be a valid value.
> + */
I'd stick the headlen for deivce without GUEST_HDRLEN. It seems much
more safe as we don't want to break legacy devices.
Thanks
