Re: [PATCH net-next v3 2/3] virtio_net: add optional flow dissection in virtio_net_hdr_to_skb

[sdf@google.com]

On 06/01, Tanner Love 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.

> The existing behavior of accepting these vnet headers is part of the
> ABI. But individual admins may want to enforce restrictions. For
> example, verifying that a GSO_TCPV4 gso_type matches packet contents:
> unencapsulated TCP/IPV4 packet with payload exceeding gso_size and
> hdr_len at payload offset.

> 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.

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

> [ um build error ]
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Tanner Love <tannerlove@google.com>
> Suggested-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..cdc6152b40c6 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))
> +		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 4b171ebec084..ae2ce382f4f4 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",

This sounds generic, but iiuc, it makes sense only for bpf flow
dissection? Should it be bpf_flow_dissect_vnet_hdr instead?

Or should we drop this sysctl in favor of some per-program flag
(either attach_flags or some signal via btf)? That way,
individual bpf programs can signal their willingness to
parse 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.rc0.204.g9fa02ecfa5-goog