Re: [PATCH net] net: virtio_net_hdr_to_skb: count transport header in UFO

[Jonathan Davies]

On 15/11/2021 16:34, Willem de Bruijn wrote:
> On Mon, Nov 15, 2021 at 4:16 PM Jonathan Davies
> <jonathan.davies@xxxxxxxxxxx> 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@xxxxxxxxxxx>
>
> Thanks for the fix, and the detailed explanation of the bug.
>
> Reviewed-by: Willem de Bruijn <willemb@xxxxxxxxxx>
>
> > ---
> >   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;

Thanks for the suggestion. I agree that improves readability. v2 posted.

Jonathan

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