netdev - Re: [PATCH net-next v1 4/5] ipv6: do not drop vrf udp multicast packets

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: <f24cd29e-2cba-0a07-13a8-a8192dca4e2f@gmail.com>
Date:   Mon, 24 Sep 2018 17:21:46 -0600
From:   David Ahern <dsahern@...il.com>
To:     Mike Manning <mmanning@...tta.att-mail.com>, netdev@...r.kernel.org
Cc:     Dewi Morgan <morgand@...tta.att-mail.com>
Subject: Re: [PATCH net-next v1 4/5] ipv6: do not drop vrf udp multicast
 packets

On 9/24/18 10:13 AM, Mike Manning wrote:
> From: Dewi Morgan <morgand@...tta.att-mail.com>
> 
> For bound udp sockets in a vrf, also check the sdif to get the index
> for ingress devices enslaved to an l3mdev. Verify the multicast address
> against the enslaved rather than the l3mdev device.
> 
> Signed-off-by: Dewi Morgan <morgand@...tta.att-mail.com>
> Signed-off-by: Mike Manning <mmanning@...tta.att-mail.com>
> ---
>  net/ipv6/ip6_input.c | 24 ++++++++++++++++++++----
>  net/ipv6/udp.c       |  8 +++++---
>  2 files changed, 25 insertions(+), 7 deletions(-)
> 

This should be 2 patches -- 1 that modifies the socket lookup to
consider and 1 that alters in the input path. They are completely
separate changes.


> diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c
> index 108f5f88ec98..82ffb5cdd2ab 100644
> --- a/net/ipv6/ip6_input.c
> +++ b/net/ipv6/ip6_input.c
> @@ -324,11 +324,14 @@ void ipv6_list_rcv(struct list_head *head, struct packet_type *pt,
>  static int ip6_input_finish(struct net *net, struct sock *sk, struct sk_buff *skb)
>  {
>  	const struct inet6_protocol *ipprot;
> +	int sdif = inet6_sdif(skb);
> +	bool have_final = false;
>  	struct inet6_dev *idev;
> +	struct net_device *dev;

make sdif and dev declarations local to where they are needed.


>  	unsigned int nhoff;
> +	bool deliver;

deliver is not needed.

>  	int nexthdr;
>  	bool raw;
> -	bool have_final = false;

so no need to move this one.

>  
>  	/*
>  	 *	Parse extension headers
> @@ -371,9 +374,22 @@ static int ip6_input_finish(struct net *net, struct sock *sk, struct sk_buff *sk
>  			skb_postpull_rcsum(skb, skb_network_header(skb),
>  					   skb_network_header_len(skb));
>  			hdr = ipv6_hdr(skb);
> -			if (ipv6_addr_is_multicast(&hdr->daddr) &&
> -			    !ipv6_chk_mcast_addr(skb->dev, &hdr->daddr,
> -			    &hdr->saddr) &&
> +
> +			/* skb->dev passed may be master dev for vrfs. */
> +			if (sdif) {
> +				dev = dev_get_by_index_rcu(dev_net(skb->dev),

net is a passed in argument. Why not use it?

> +							   sdif);
> +				if (!dev) {
> +					kfree_skb(skb);
> +					return -ENODEV;

The rcu_read_lock() is held. I believe 'goto discard' is sufficient if
the enslaved device disappeared.

> +				}
> +			} else {
> +				dev = skb->dev;
> +			}
> +
> +			deliver = ipv6_chk_mcast_addr(dev, &hdr->daddr,
> +						      &hdr->saddr);
> +			if (ipv6_addr_is_multicast(&hdr->daddr) && !deliver &&
>  			    !ipv6_is_mld(skb, nexthdr, skb_network_header_len(skb)))
>  				goto discard;
>  		}

I think the original code only needs skb->dev changed to dev making this
a much smaller patch.