Re: [PATCH v2 net] net: virtio_net_hdr_to_skb: count transport header in UFO
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: 2021-11-16 21:43:26
Also in:
lkml, virtualization
On Tue, Nov 16, 2021 at 6:43 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>Reviewed-by: Willem de Bruijn <willemb@google.com>
quoted hunk ↗ jump to hunk
--- Changes in v2: - refactor to use variable for readability --- include/linux/virtio_net.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h index b465f8f..04e87f4b 100644 --- a/include/linux/virtio_net.h +++ b/include/linux/virtio_net.h@@ -120,10 +120,15 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb, if (hdr->gso_type != VIRTIO_NET_HDR_GSO_NONE) { u16 gso_size = __virtio16_to_cpu(little_endian, hdr->gso_size); + unsigned int nh_off = p_off; struct skb_shared_info *shinfo = skb_shinfo(skb); + /* UFO may not include transport header in gso_size. */ + if (gso_type & SKB_GSO_UDP) + nh_off -= thlen;
Subtracting from an unsigned int always has the chance of negative overflow.
This case is safe, as all three paths that lead here have a p_off >= thlen.
I just noticed a more obscure fourth path:
if (hdr->gso_type != VIRTIO_NET_HDR_GSO_NONE) {
switch (hdr->gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
We do not explicitly check against hdr->gso_type ==
VIRTIO_NET_HDR_GSO_ECN. An obviously bogus value. That leaves p_off 0.
But it also leaves th_len 0, so it is safe.
Negative overflow is also safe in this case.