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 <[email protected]>
---
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
<[email protected]> 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 <[email protected]>
Thanks for the fix, and the detailed explanation of the bug.
Reviewed-by: Willem de Bruijn <[email protected]>
> ---
> 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
>
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
> <[email protected]> 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 <[email protected]>
>
> Thanks for the fix, and the detailed explanation of the bug.
>
> Reviewed-by: Willem de Bruijn <[email protected]>
>
>> ---
>> 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
>>