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

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

[Yi-Hung Wei]

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@gmail.com>
---
 drivers/net/geneve.c     | 3 +--
 include/net/ip_tunnels.h | 7 ++++---
 2 files changed, 5 insertions(+), 5 deletions(-)

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,
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 */
-- 
2.7.4

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

[Yi-Hung Wei]

On Tue, Nov 10, 2020 at 4:17 PM Yi-Hung Wei <yihung.wei@gmail.com> 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@gmail.com>
> ---

Sorry that I did not indicate in the subject line of the previous
email.  It should be "[PATCH net]".

-Yi-Hung

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

[Jakub Kicinski]

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@gmail.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 */

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

[Daniel Borkmann]

On 11/14/20 1:13 AM, Jakub Kicinski wrote:
> 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@gmail.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?

Verifier will guarantee that buffer passed into helper is > 0, so seems
okay from BPF side.

Thanks,
Daniel

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

[Jakub Kicinski]

On Sat, 14 Nov 2020 01:46:04 +0100 Daniel Borkmann wrote:
> On 11/14/20 1:13 AM, Jakub Kicinski wrote:
> > 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@gmail.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?  
> 
> Verifier will guarantee that buffer passed into helper is > 0, so seems
> okay from BPF side.

Okay then, the potential change for netfilter is limited to GENEVE
so doesn't move the needle.

Applied, thanks!