| Message ID | 20211115151618.126875-1-jonathan.davies@nutanix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: virtio_net_hdr_to_skb: count transport header in UFO | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Single patches do not need cover letters |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | fail | Errors and warnings before: 8 this patch: 30 |
| netdev/cc_maintainers | success | CCed 5 of 5 maintainers |
| netdev/build_clang | fail | Errors and warnings before: 0 this patch: 66 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | fail | Problems with Fixes tag: 1 |
| netdev/build_allmodconfig_warn | fail | Errors and warnings before: 8 this patch: 30 |
| netdev/checkpatch | warning | WARNING: line length of 84 exceeds 80 columns |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Mon, Nov 15, 2021 at 4:16 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> Thanks for the fix, and the detailed explanation of the bug. Reviewed-by: Willem de Bruijn <willemb@google.com> > --- > 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 > <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> > > Thanks for the fix, and the detailed explanation of the bug. > > Reviewed-by: Willem de Bruijn <willemb@google.com> > >> --- >> 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 >>
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;
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> --- include/linux/virtio_net.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)