MIME-Version: 1.0
References: <20211115151618.126875-1-jonathan.davies@nutanix.com>
In-Reply-To: <20211115151618.126875-1-jonathan.davies@nutanix.com>
From:   Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date:   Mon, 15 Nov 2021 17:34:17 +0100
Message-ID: <CA+FuTScqWToamoOqAWkf1VbzYnjoM-y+-rQe_wEkPmBsOZbsLA@mail.gmail.com>
Subject: Re: [PATCH net] net: virtio_net_hdr_to_skb: count transport header in UFO
To:     Jonathan Davies <jonathan.davies@nutanix.com>
Cc:     netdev@vger.kernel.org, Florian Schmidt <flosch@nutanix.com>,
        Thilak Raj Surendra Babu <thilakraj.sb@nutanix.com>,
        "Michael S. Tsirkin" <mst@redhat.com>,
        Jason Wang <jasowang@redhat.com>,
        "David S. Miller" <davem@davemloft.net>,
        virtualization@lists.linux-foundation.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Nov 15, 2021 at 4:16 PM Jonathan Davies
<jonathan.davies@nutanix.com> wrote:
>
> virtio_net_hdr_to_skb does not set the skb's gso_size and gso_type
> correctly for UFO packets received via virtio-net that are a little over
> the GSO size. This can lead to problems elsewhere in the networking
> stack, e.g. ovs_vport_send dropping over-sized packets if gso_size is
> not set.
>
> This is due to the comparison
>
>   if (skb->len - p_off > gso_size)
>
> not properly accounting for the transport layer header.
>
> p_off includes the size of the transport layer header (thlen), so
> skb->len - p_off is the size of the TCP/UDP payload.
>
> gso_size is read from the virtio-net header. For UFO, fragmentation
> happens at the IP level so does not need to include the UDP header.
>
> Hence the calculation could be comparing a TCP/UDP payload length with
> an IP payload length, causing legitimate virtio-net packets to have
> lack gso_type/gso_size information.
>
> Example: a UDP packet with payload size 1473 has IP payload size 1481.
> If the guest used UFO, it is not fragmented and the virtio-net header's
> flags indicate that it is a GSO frame (VIRTIO_NET_HDR_GSO_UDP), with
> gso_size = 1480 for an MTU of 1500.  skb->len will be 1515 and p_off
> will be 42, so skb->len - p_off = 1473.  Hence the comparison fails, and
> shinfo->gso_size and gso_type are not set as they should be.
>
> Instead, add the UDP header length before comparing to gso_size when
> using UFO. In this way, it is the size of the IP payload that is
> compared to gso_size.
>
> Fixes: 6dd912f8 ("net: check untrusted gso_size at kernel entry")
> Signed-off-by: Jonathan Davies <jonathan.davies@nutanix.com>

Thanks for the fix, and the detailed explanation of the bug.

Reviewed-by: Willem de Bruijn <willemb@google.com>

> ---
>  include/linux/virtio_net.h | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index b465f8f..bea56af 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -122,8 +122,11 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
>                 u16 gso_size = __virtio16_to_cpu(little_endian, hdr->gso_size);
>                 struct skb_shared_info *shinfo = skb_shinfo(skb);
>
> -               /* Too small packets are not really GSO ones. */
> -               if (skb->len - p_off > gso_size) {
> +               /* Too small packets are not really GSO ones.
> +                * UFO may not include transport header in gso_size.
> +                */
> +               if (gso_type & SKB_GSO_UDP && skb->len - p_off + thlen > gso_size ||
> +                   skb->len - p_off > gso_size) {

Perhaps for readability instead something like

  unsigned int nh_off = p_off;

  if (gso_type & SKB_GSO_UDP)
    nh_off -= thlen;


>                         shinfo->gso_size = gso_size;
>                         shinfo->gso_type = gso_type;
>
> --
> 2.9.3
>