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

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 <redacted>
---
 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) {
 			shinfo->gso_size = gso_size;
 			shinfo->gso_type = gso_type;
 

-- 
2.9.3

On Mon, Nov 15, 2021 at 4:16 PM Jonathan Davies
[off-list ref] 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 <redacted>

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

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

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

On Mon, 15 Nov 2021 15:16:17 +0000 Jonathan Davies wrote:

+		if (gso_type & SKB_GSO_UDP && skb->len - p_off + thlen > gso_size ||

Compilers don't like mixing && and || without bracketing, and will warn
here, at least with W=1. Please add explicit brackets.

On 15/11/2021 16:34, Willem de Bruijn wrote:

On Mon, Nov 15, 2021 at 4:16 PM Jonathan Davies
[off-list ref] wrote:

quoted

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 <redacted>

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

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

quoted

---
  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

quoted

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

--
2.9.3