netdev - Re: [PATCH net] gre: fix error handler

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]

Message-ID: <1465969537.7945.201.camel@edumazet-glaptop3.roam.corp.google.com>
Date:	Tue, 14 Jun 2016 22:45:37 -0700
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	David Miller <davem@...emloft.net>, Jiri Benc <jbenc@...hat.com>
Cc:	netdev <netdev@...r.kernel.org>, Tom Herbert <tom@...bertland.com>,
	Maciej Żenczykowski <maze@...gle.com>
Subject: Re: [PATCH net] gre: fix error handler

On Tue, 2016-06-14 at 22:15 -0700, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@...gle.com>
> 
> 1) gre_parse_header() can be called from gre_err()
> 
>    At this point transport header points to ICMP header, not the inner
> header.
> 
> 2) We can not really change transport header as ipgre_err() will later
> assume transport header still points to ICMP header.
> 
> 3) pskb_may_pull() logic in gre_parse_header() really works
>   if we are interested at zone pointed by skb->data
> 
> So this fix :
> 
> A) changes gre_parse_header() to use skb->data instead of
> skb_transport_header()
> 
> B) changes gre_err() to pull the IPv4 header immediately following
> the ICMP header that was already pulled earlier.
> 
> C) remove obsolete IPV6 includes
> 
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> Cc: Tom Herbert <tom@...bertland.com>
> Cc: Maciej Żenczykowski <maze@...gle.com>
> ---
>  net/ipv4/gre_demux.c |    4 ++--
>  net/ipv4/ip_gre.c    |    9 +++------
>  2 files changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/net/ipv4/gre_demux.c b/net/ipv4/gre_demux.c
> index 4c39f4fd332a..0ba26ad9809d 100644
> --- a/net/ipv4/gre_demux.c
> +++ b/net/ipv4/gre_demux.c
> @@ -71,7 +71,7 @@ int gre_parse_header(struct sk_buff *skb, struct tnl_ptk_info *tpi,
>  	if (unlikely(!pskb_may_pull(skb, sizeof(struct gre_base_hdr))))
>  		return -EINVAL;
>  
> -	greh = (struct gre_base_hdr *)skb_transport_header(skb);
> +	greh = (struct gre_base_hdr *)skb->data;
>  	if (unlikely(greh->flags & (GRE_VERSION | GRE_ROUTING)))
>  		return -EINVAL;
>  
> @@ -81,7 +81,7 @@ int gre_parse_header(struct sk_buff *skb, struct tnl_ptk_info *tpi,
>  	if (!pskb_may_pull(skb, hdr_len))
>  		return -EINVAL;
>  
> -	greh = (struct gre_base_hdr *)skb_transport_header(skb);
> +	greh = (struct gre_base_hdr *)skb->data;
>  	tpi->proto = greh->protocol;
>  
>  	options = (__be32 *)(greh + 1);
> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> index 4d2025f7ec57..454832bc2897 100644
> --- a/net/ipv4/ip_gre.c
> +++ b/net/ipv4/ip_gre.c
> @@ -49,12 +49,6 @@
>  #include <net/gre.h>
>  #include <net/dst_metadata.h>
>  
> -#if IS_ENABLED(CONFIG_IPV6)
> -#include <net/ipv6.h>
> -#include <net/ip6_fib.h>
> -#include <net/ip6_route.h>
> -#endif
> -
>  /*
>     Problems & solutions
>     --------------------
> @@ -217,11 +211,14 @@ static void gre_err(struct sk_buff *skb, u32 info)
>  	 * by themselves???
>  	 */
>  
> +	const struct iphdr *iph = (struct iphdr *)skb->data;
>  	const int type = icmp_hdr(skb)->type;
>  	const int code = icmp_hdr(skb)->code;
>  	struct tnl_ptk_info tpi;
>  	bool csum_err = false;
>  
> +	pskb_pull(skb, iph->ihl * 4);
> +
>  	if (gre_parse_header(skb, &tpi, &csum_err, htons(ETH_P_IP)) < 0) {
>  		if (!csum_err)		/* ignore csum errors. */
>  			return;
> 


Hmm... I read this prior commit. It looks like we need something else.

commit b7f8fe251e4609e2a437bd2c2dea01e61db6849c
Author: Jiri Benc <jbenc@...hat.com>
Date:   Fri Apr 29 23:31:32 2016 +0200

    gre: do not pull header in ICMP error processing
    
    iptunnel_pull_header expects that IP header was already pulled; with this
    expectation, it pulls the tunnel header. This is not true in gre_err.
    Furthermore, ipv4_update_pmtu and ipv4_redirect expect that skb->data points
    to the IP header.
    
    We cannot pull the tunnel header in this path. It's just a matter of not
    calling iptunnel_pull_header - we don't need any of its effects.
    
    Fixes: bda7bb463436 ("gre: Allow multiple protocol listener for gre protocol.")
    Signed-off-by: Jiri Benc <jbenc@...hat.com>
    Signed-off-by: David S. Miller <davem@...emloft.net>