| Message ID | 20211116174242.32681-1-jonathan.davies@nutanix.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | cf9acc90c80ecbee00334aa85d92f4e74014bcff |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,net] net: virtio_net_hdr_to_skb: count transport header in UFO | expand |
On Tue, Nov 16, 2021 at 6:43 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> Reviewed-by: Willem de Bruijn <willemb@google.com> > --- > 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.
Hello: This patch was applied to netdev/net.git (master) by David S. Miller <davem@davemloft.net>: On Tue, 16 Nov 2021 17:42:42 +0000 you 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 > > [...] Here is the summary with links: - [v2,net] net: virtio_net_hdr_to_skb: count transport header in UFO https://git.kernel.org/netdev/net/c/cf9acc90c80e You are awesome, thank you!
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; + /* Too small packets are not really GSO ones. */ - if (skb->len - p_off > gso_size) { + if (skb->len - nh_off > gso_size) { shinfo->gso_size = gso_size; shinfo->gso_type = gso_type;
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> --- Changes in v2: - refactor to use variable for readability --- include/linux/virtio_net.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)