diff mbox series

[net-next,v7,2/3] virtio_net: add optional flow dissection in virtio_net_hdr_to_skb

Message ID 20210616203448.995314-3-tannerlove.kernel@gmail.com (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series virtio_net: add optional flow dissection in virtio_net_hdr_to_skb | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers warning 17 maintainers not CCed: vladimir.oltean@nxp.com dvyukov@google.com yhs@fb.com virtualization@lists.linux-foundation.org kpsingh@kernel.org bpf@vger.kernel.org wangqing@vivo.com masahiroy@kernel.org maheshb@google.com keescook@chromium.org wenxu@ucloud.cn john.fastabend@gmail.com eranbe@nvidia.com songliubraving@fb.com alobakin@pm.me lariel@mellanox.com jakub@cloudflare.com
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 3 this patch: 3
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch warning WARNING: line length of 89 exceeds 80 columns
netdev/build_allmodconfig_warn success Errors and warnings before: 3 this patch: 3
netdev/header_inline success Link

Commit Message

Tanner Love June 16, 2021, 8:34 p.m. UTC
From: Tanner Love <tannerlove@google.com>

Syzkaller bugs have resulted from loose specification of
virtio_net_hdr[1]. Enable execution of a BPF flow dissector program
in virtio_net_hdr_to_skb to validate the vnet header and drop bad
input.

Introduce a new sysctl net.core.flow_dissect_vnet_hdr controlling a
static key to decide whether to perform flow dissection. When the key
is false, virtio_net_hdr_to_skb computes as before.

A permissive specification of vnet headers is part of the ABI. Some
applications now depend on it. Still, many of these packets are bogus.
Give admins the option to interpret behavior more strictly. For
instance, verifying that a VIRTIO_NET_HDR_GSO_TCPV6 header matches a
packet with unencapsulated IPv6/TCP without extension headers, with
payload length exceeding gso_size and hdr_len exactly at TCP payload
offset.

BPF flow dissection implements protocol parsing in an safe way. And is
configurable, so can be as pedantic as the workload allows (e.g.,
dropping UFO altogether).

Vnet_header flow dissection is *not* a substitute for fixing bugs when
reported. But even if not enabled continuously, offers a quick path to
mitigating vulnerabilities.

[1] https://syzkaller.appspot.com/bug?id=b419a5ca95062664fe1a60b764621eb4526e2cd0

Changes
v4:
  - Expand commit message with rationale for bpf flow dissector based
    implementation
v3:
  - Move sysctl_flow_dissect_vnet_hdr_key definition to
    flow_dissector.c to fix CONFIG_SYSCTL warning when building UML

Suggested-by: Willem de Bruijn <willemb@google.com>
Signed-off-by: Tanner Love <tannerlove@google.com>
Reviewed-by: Willem de Bruijn <willemb@google.com>
---
 include/linux/virtio_net.h | 25 +++++++++++++++++++++----
 net/core/flow_dissector.c  |  3 +++
 net/core/sysctl_net_core.c |  9 +++++++++
 3 files changed, 33 insertions(+), 4 deletions(-)

Comments

Tanner Love June 16, 2021, 8:38 p.m. UTC | #1
We are still working on resolving the larger design comment
discussion in v4. These updates address specific implementation
feedback from v6. Thanks

On Wed, Jun 16, 2021 at 1:34 PM Tanner Love <tannerlove.kernel@gmail.com> wrote:
>
> From: Tanner Love <tannerlove@google.com>
>
> Syzkaller bugs have resulted from loose specification of
> virtio_net_hdr[1]. Enable execution of a BPF flow dissector program
> in virtio_net_hdr_to_skb to validate the vnet header and drop bad
> input.
>
> Introduce a new sysctl net.core.flow_dissect_vnet_hdr controlling a
> static key to decide whether to perform flow dissection. When the key
> is false, virtio_net_hdr_to_skb computes as before.
>
> A permissive specification of vnet headers is part of the ABI. Some
> applications now depend on it. Still, many of these packets are bogus.
> Give admins the option to interpret behavior more strictly. For
> instance, verifying that a VIRTIO_NET_HDR_GSO_TCPV6 header matches a
> packet with unencapsulated IPv6/TCP without extension headers, with
> payload length exceeding gso_size and hdr_len exactly at TCP payload
> offset.
>
> BPF flow dissection implements protocol parsing in an safe way. And is
> configurable, so can be as pedantic as the workload allows (e.g.,
> dropping UFO altogether).
>
> Vnet_header flow dissection is *not* a substitute for fixing bugs when
> reported. But even if not enabled continuously, offers a quick path to
> mitigating vulnerabilities.
>
> [1] https://syzkaller.appspot.com/bug?id=b419a5ca95062664fe1a60b764621eb4526e2cd0
>
> Changes
> v4:
>   - Expand commit message with rationale for bpf flow dissector based
>     implementation
> v3:
>   - Move sysctl_flow_dissect_vnet_hdr_key definition to
>     flow_dissector.c to fix CONFIG_SYSCTL warning when building UML
>
> Suggested-by: Willem de Bruijn <willemb@google.com>
> Signed-off-by: Tanner Love <tannerlove@google.com>
> Reviewed-by: Willem de Bruijn <willemb@google.com>
> ---
>  include/linux/virtio_net.h | 25 +++++++++++++++++++++----
>  net/core/flow_dissector.c  |  3 +++
>  net/core/sysctl_net_core.c |  9 +++++++++
>  3 files changed, 33 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index b465f8f3e554..b67b5413f2ce 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -25,10 +25,13 @@ static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,
>         return 0;
>  }
>
> +DECLARE_STATIC_KEY_FALSE(sysctl_flow_dissect_vnet_hdr_key);
> +
>  static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
>                                         const struct virtio_net_hdr *hdr,
>                                         bool little_endian)
>  {
> +       struct flow_keys_basic keys;
>         unsigned int gso_type = 0;
>         unsigned int thlen = 0;
>         unsigned int p_off = 0;
> @@ -78,13 +81,24 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
>                 p_off = skb_transport_offset(skb) + thlen;
>                 if (!pskb_may_pull(skb, p_off))
>                         return -EINVAL;
> -       } else {
> +       }
> +
> +       /* BPF flow dissection for optional strict validation.
> +        *
> +        * Admins can define permitted packets more strictly, such as dropping
> +        * deprecated UDP_UFO packets and requiring skb->protocol to be non-zero
> +        * and matching packet headers.
> +        */
> +       if (static_branch_unlikely(&sysctl_flow_dissect_vnet_hdr_key) &&
> +           !__skb_flow_dissect_flow_keys_basic(NULL, skb, &keys, NULL, 0, 0, 0,
> +                                               0, hdr, little_endian))
> +               return -EINVAL;
> +
> +       if (!(hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM)) {
>                 /* gso packets without NEEDS_CSUM do not set transport_offset.
>                  * probe and drop if does not match one of the above types.
>                  */
>                 if (gso_type && skb->network_header) {
> -                       struct flow_keys_basic keys;
> -
>                         if (!skb->protocol) {
>                                 __be16 protocol = dev_parse_header_protocol(skb);
>
> @@ -92,8 +106,11 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
>                                 if (protocol && protocol != skb->protocol)
>                                         return -EINVAL;
>                         }
> +
>  retry:
> -                       if (!skb_flow_dissect_flow_keys_basic(NULL, skb, &keys,
> +                       /* only if flow dissection not already done */
> +                       if (!static_branch_unlikely(&sysctl_flow_dissect_vnet_hdr_key) &&
> +                           !skb_flow_dissect_flow_keys_basic(NULL, skb, &keys,
>                                                               NULL, 0, 0, 0,
>                                                               0)) {
>                                 /* UFO does not specify ipv4 or 6: try both */
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 609e24ba98ea..046aa2a8c39d 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -35,6 +35,9 @@
>  #endif
>  #include <linux/bpf-netns.h>
>
> +DEFINE_STATIC_KEY_FALSE(sysctl_flow_dissect_vnet_hdr_key);
> +EXPORT_SYMBOL(sysctl_flow_dissect_vnet_hdr_key);
> +
>  static void dissector_set_key(struct flow_dissector *flow_dissector,
>                               enum flow_dissector_key_id key_id)
>  {
> diff --git a/net/core/sysctl_net_core.c b/net/core/sysctl_net_core.c
> index c8496c1142c9..c01b9366bb75 100644
> --- a/net/core/sysctl_net_core.c
> +++ b/net/core/sysctl_net_core.c
> @@ -36,6 +36,8 @@ static int net_msg_warn;      /* Unused, but still a sysctl */
>  int sysctl_fb_tunnels_only_for_init_net __read_mostly = 0;
>  EXPORT_SYMBOL(sysctl_fb_tunnels_only_for_init_net);
>
> +DECLARE_STATIC_KEY_FALSE(sysctl_flow_dissect_vnet_hdr_key);
> +
>  /* 0 - Keep current behavior:
>   *     IPv4: inherit all current settings from init_net
>   *     IPv6: reset all settings to default
> @@ -580,6 +582,13 @@ static struct ctl_table net_core_table[] = {
>                 .extra1         = SYSCTL_ONE,
>                 .extra2         = &int_3600,
>         },
> +       {
> +               .procname       = "flow_dissect_vnet_hdr",
> +               .data           = &sysctl_flow_dissect_vnet_hdr_key.key,
> +               .maxlen         = sizeof(sysctl_flow_dissect_vnet_hdr_key),
> +               .mode           = 0644,
> +               .proc_handler   = proc_do_static_key,
> +       },
>         { }
>  };
>
> --
> 2.32.0.272.g935e593368-goog
>
diff mbox series

Patch

diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index b465f8f3e554..b67b5413f2ce 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -25,10 +25,13 @@  static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,
 	return 0;
 }
 
+DECLARE_STATIC_KEY_FALSE(sysctl_flow_dissect_vnet_hdr_key);
+
 static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
 					const struct virtio_net_hdr *hdr,
 					bool little_endian)
 {
+	struct flow_keys_basic keys;
 	unsigned int gso_type = 0;
 	unsigned int thlen = 0;
 	unsigned int p_off = 0;
@@ -78,13 +81,24 @@  static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
 		p_off = skb_transport_offset(skb) + thlen;
 		if (!pskb_may_pull(skb, p_off))
 			return -EINVAL;
-	} else {
+	}
+
+	/* BPF flow dissection for optional strict validation.
+	 *
+	 * Admins can define permitted packets more strictly, such as dropping
+	 * deprecated UDP_UFO packets and requiring skb->protocol to be non-zero
+	 * and matching packet headers.
+	 */
+	if (static_branch_unlikely(&sysctl_flow_dissect_vnet_hdr_key) &&
+	    !__skb_flow_dissect_flow_keys_basic(NULL, skb, &keys, NULL, 0, 0, 0,
+						0, hdr, little_endian))
+		return -EINVAL;
+
+	if (!(hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM)) {
 		/* gso packets without NEEDS_CSUM do not set transport_offset.
 		 * probe and drop if does not match one of the above types.
 		 */
 		if (gso_type && skb->network_header) {
-			struct flow_keys_basic keys;
-
 			if (!skb->protocol) {
 				__be16 protocol = dev_parse_header_protocol(skb);
 
@@ -92,8 +106,11 @@  static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
 				if (protocol && protocol != skb->protocol)
 					return -EINVAL;
 			}
+
 retry:
-			if (!skb_flow_dissect_flow_keys_basic(NULL, skb, &keys,
+			/* only if flow dissection not already done */
+			if (!static_branch_unlikely(&sysctl_flow_dissect_vnet_hdr_key) &&
+			    !skb_flow_dissect_flow_keys_basic(NULL, skb, &keys,
 							      NULL, 0, 0, 0,
 							      0)) {
 				/* UFO does not specify ipv4 or 6: try both */
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 609e24ba98ea..046aa2a8c39d 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -35,6 +35,9 @@ 
 #endif
 #include <linux/bpf-netns.h>
 
+DEFINE_STATIC_KEY_FALSE(sysctl_flow_dissect_vnet_hdr_key);
+EXPORT_SYMBOL(sysctl_flow_dissect_vnet_hdr_key);
+
 static void dissector_set_key(struct flow_dissector *flow_dissector,
 			      enum flow_dissector_key_id key_id)
 {
diff --git a/net/core/sysctl_net_core.c b/net/core/sysctl_net_core.c
index c8496c1142c9..c01b9366bb75 100644
--- a/net/core/sysctl_net_core.c
+++ b/net/core/sysctl_net_core.c
@@ -36,6 +36,8 @@  static int net_msg_warn;	/* Unused, but still a sysctl */
 int sysctl_fb_tunnels_only_for_init_net __read_mostly = 0;
 EXPORT_SYMBOL(sysctl_fb_tunnels_only_for_init_net);
 
+DECLARE_STATIC_KEY_FALSE(sysctl_flow_dissect_vnet_hdr_key);
+
 /* 0 - Keep current behavior:
  *     IPv4: inherit all current settings from init_net
  *     IPv6: reset all settings to default
@@ -580,6 +582,13 @@  static struct ctl_table net_core_table[] = {
 		.extra1		= SYSCTL_ONE,
 		.extra2		= &int_3600,
 	},
+	{
+		.procname       = "flow_dissect_vnet_hdr",
+		.data           = &sysctl_flow_dissect_vnet_hdr_key.key,
+		.maxlen         = sizeof(sysctl_flow_dissect_vnet_hdr_key),
+		.mode           = 0644,
+		.proc_handler   = proc_do_static_key,
+	},
 	{ }
 };