netdev - Re: [PATCH] net: ip/tnl: Set iph->id only when don't fragment is not set

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 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:   Sat, 23 Nov 2019 09:53:33 -0800
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Oliver Herms <oliver.peter.herms@...il.com>, davem@...emloft.net,
        kuznet@....inr.ac.ru, yoshfuji@...ux-ipv6.org
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH] net: ip/tnl: Set iph->id only when don't fragment is not
 set



On 11/23/19 6:58 AM, Oliver Herms wrote:
> In IPv4 the identification field ensures that fragments of different datagrams
> are not mixed by the receiver. Packets with Don't Fragment (DF) flag set are not
> to be fragmented in transit and thus don't need an identification.

Official sources for this assertion please, so that we can double check if you
implemented the proper avoidance ?

> Calculating the identification takes significant CPU time.
> This patch will increase IP tunneling performance by ~10% unless DF is not set.
> However, DF is set by default which is best practice.
> 
> Signed-off-by: Oliver Herms <oliver.peter.herms@...il.com>
> ---
>  net/ipv4/ip_tunnel_core.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ipv4/ip_tunnel_core.c b/net/ipv4/ip_tunnel_core.c
> index 1452a97914a0..8636c1e0e7b7 100644
> --- a/net/ipv4/ip_tunnel_core.c
> +++ b/net/ipv4/ip_tunnel_core.c
> @@ -73,7 +73,9 @@ void iptunnel_xmit(struct sock *sk, struct rtable *rt, struct sk_buff *skb,
>  	iph->daddr	=	dst;
>  	iph->saddr	=	src;
>  	iph->ttl	=	ttl;
> -	__ip_select_ident(net, iph, skb_shinfo(skb)->gso_segs ?: 1);
> +
> +	if (unlikely((iph->frag_off & htons(IP_DF)) == false))

This unlikely() seems wrong to me.

You do not know what are the odds of IP_DF being set or not.



> +		__ip_select_ident(net, iph, skb_shinfo(skb)->gso_segs ?: 1);
>  
>  	err = ip_local_out(net, sk, skb);
>  
> 

So we are going to send 2 bytes with garbage if we do not call __ip_select_ident()

This would cause various security threats, since the garbage might reveal a secret.