netdev - Re: [PATCH] ip_tunnels: Set tunnel option flag when tunnel metadata is present

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 13 Nov 2020 16:13:59 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Yi-Hung Wei <yihung.wei@...il.com>,
        Pablo Neira Ayuso <pablo@...filter.org>,
        Daniel Borkmann <daniel@...earbox.net>
Cc:     davem@...emloft.net, kuznet@....inr.ac.ru, yoshfuji@...ux-ipv6.org,
        pieter.jansenvanvuuren@...ronome.com, netdev@...r.kernel.org
Subject: Re: [PATCH] ip_tunnels: Set tunnel option flag when tunnel metadata
 is present

On Tue, 10 Nov 2020 16:16:40 -0800 Yi-Hung Wei wrote:
> Currently, we may set the tunnel option flag when the size of metadata
> is zero.  For example, we set TUNNEL_GENEVE_OPT in the receive function
> no matter the geneve option is present or not.  As this may result in
> issues on the tunnel flags consumers, this patch fixes the issue.
> 
> Related discussion:
> * https://lore.kernel.org/netdev/1604448694-19351-1-git-send-email-yihung.wei@gmail.com/T/#u
> 
> Fixes: 256c87c17c53 ("net: check tunnel option type in tunnel flags")
> Signed-off-by: Yi-Hung Wei <yihung.wei@...il.com>

Seems fine to me, however BPF (and maybe Netfilter?) can set options
passed by user without checking if they are 0 length.

Daniel, Pablo, are you okay with this change or should we limit it to
just fixing the GENEVE oddness?

> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
> index d07008a818df..1426bfc009bc 100644
> --- a/drivers/net/geneve.c
> +++ b/drivers/net/geneve.c
> @@ -224,8 +224,7 @@ static void geneve_rx(struct geneve_dev *geneve, struct geneve_sock *gs,
>  	if (ip_tunnel_collect_metadata() || gs->collect_md) {
>  		__be16 flags;
>  
> -		flags = TUNNEL_KEY | TUNNEL_GENEVE_OPT |
> -			(gnvh->oam ? TUNNEL_OAM : 0) |
> +		flags = TUNNEL_KEY | (gnvh->oam ? TUNNEL_OAM : 0) |
>  			(gnvh->critical ? TUNNEL_CRIT_OPT : 0);
>  
>  		tun_dst = udp_tun_rx_dst(skb, geneve_get_sk_family(gs), flags,

For the minimal fix we'd just need the change above, plus:

-               ip_tunnel_info_opts_set(&tun_dst->u.tun_info,
-                                       gnvh->options, gnvh->opt_len * 4,
-                                       TUNNEL_GENEVE_OPT);
+               if (gnvh->opt_len)
+                       ip_tunnel_info_opts_set(&tun_dst->u.tun_info,
+                                               gnvh->options,
+                                               gnvh->opt_len * 4,
+                                               TUNNEL_GENEVE_OPT);

Right?

> diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
> index 02ccd32542d0..61620677b034 100644
> --- a/include/net/ip_tunnels.h
> +++ b/include/net/ip_tunnels.h
> @@ -478,9 +478,11 @@ static inline void ip_tunnel_info_opts_set(struct ip_tunnel_info *info,
>  					   const void *from, int len,
>  					   __be16 flags)
>  {
> -	memcpy(ip_tunnel_info_opts(info), from, len);
>  	info->options_len = len;
> -	info->key.tun_flags |= flags;
> +	if (len > 0) {
> +		memcpy(ip_tunnel_info_opts(info), from, len);
> +		info->key.tun_flags |= flags;
> +	}
>  }
>  
>  static inline struct ip_tunnel_info *lwt_tun_info(struct lwtunnel_state *lwtstate)
> @@ -526,7 +528,6 @@ static inline void ip_tunnel_info_opts_set(struct ip_tunnel_info *info,
>  					   __be16 flags)
>  {
>  	info->options_len = 0;
> -	info->key.tun_flags |= flags;
>  }
>  
>  #endif /* CONFIG_INET */