| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHrNZNi-3u2r1z=nCyQyiCLVb6JWFfD3WP26q_Rg83zNzK3Png@mail.gmail.com>
Date: Wed, 16 Jun 2021 13:38:38 -0700
From: Tanner Love <tannerlove.kernel@...il.com>
To: Network Development <netdev@...r.kernel.org>
Cc: "David S . Miller" <davem@...emloft.net>,
Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Andrii Nakryiko <andrii@...nel.org>,
Eric Dumazet <edumazet@...gle.com>,
Willem de Bruijn <willemb@...gle.com>,
Petar Penkov <ppenkov@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
"Michael S . Tsirkin" <mst@...hat.com>,
Jason Wang <jasowang@...hat.com>,
Martin KaFai Lau <kafai@...com>,
Tanner Love <tannerlove@...gle.com>
Subject: Re: [PATCH net-next v7 2/3] virtio_net: add optional flow dissection
in virtio_net_hdr_to_skb
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@...il.com> wrote:
>
> From: Tanner Love <tannerlove@...gle.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@...gle.com>
> Signed-off-by: Tanner Love <tannerlove@...gle.com>
> Reviewed-by: Willem de Bruijn <willemb@...gle.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
>
Powered by blists - more mailing lists