netdev - Re: [net-next PATCH v2] udp: Resolve NULL pointer dereference over flow-based vxlan device

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: <1463082953.23934.160.camel@edumazet-glaptop3.roam.corp.google.com>
Date:	Thu, 12 May 2016 12:55:53 -0700
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Alexander Duyck <aduyck@...antis.com>
Cc:	netdev@...r.kernel.org, davem@...emloft.net,
	alexander.duyck@...il.com, tgraf@...g.ch, tom@...bertland.com,
	jbenc@...hat.com
Subject: Re: [net-next PATCH v2] udp: Resolve NULL pointer dereference over
 flow-based vxlan device

On Thu, 2016-05-12 at 12:51 -0700, Alexander Duyck wrote:
> While testing an OpenStack configuration using VXLANs I saw the following
> call trace:

> The following trace is seen when receiving a DHCP request over a flow-based
> VXLAN tunnel.  I believe this is caused by the metadata dst having a NULL
> dev value and as a result dev_net(dev) is causing a NULL pointer dereference.
> 
> To resolve this I am replacing the check for skb_dst() with skb_valid_dst()
> so that we do not attempt to use the metadata dst to retrieve a device in
> order to determine the network namespace.
> 
> Fixes: 63058308cd55 ("udp: Add udp6_lib_lookup_skb and udp4_lib_lookup_skb")
> Signed-off-by: Alexander Duyck <aduyck@...antis.com>
> ---
>  net/ipv4/udp.c |    3 ++-
>  net/ipv6/udp.c |    3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index f67f52ba4809..69aa7ab81933 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -114,6 +114,7 @@
>  #include <net/busy_poll.h>
>  #include "udp_impl.h"
>  #include <net/sock_reuseport.h>
> +#include <net/dst_metadata.h>
>  
>  struct udp_table udp_table __read_mostly;
>  EXPORT_SYMBOL(udp_table);
> @@ -614,7 +615,7 @@ struct sock *udp4_lib_lookup_skb(struct sk_buff *skb,
>  {
>  	const struct iphdr *iph = ip_hdr(skb);
>  	const struct net_device *dev =
> -	    skb_dst(skb) ? skb_dst(skb)->dev : skb->dev;
> +		skb_valid_dst(skb) ? skb_dst(skb)->dev : skb->dev;

Looks overly complicated to me.

If this is called from GRO, why don't we simply use skb->dev ?