netdev - Re: [PATCH net 3/5] sunvnet: use icmp_ndo

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: <20200210.123619.546500251078019206.davem@davemloft.net>
Date:   Mon, 10 Feb 2020 12:36:19 +0100 (CET)
From:   David Miller <davem@...emloft.net>
To:     Jason@...c4.com
Cc:     netfilter-devel@...r.kernel.org, netdev@...r.kernel.org,
        shannon.nelson@...cle.com
Subject: Re: [PATCH net 3/5] sunvnet: use icmp_ndo_send helper

From: "Jason A. Donenfeld" <Jason@...c4.com>
Date: Sun,  9 Feb 2020 15:31:41 +0100

> Because sunvnet is calling icmp from network device context, it should use
> the ndo helper so that the rate limiting applies correctly.
> 
> Signed-off-by: Jason A. Donenfeld <Jason@...c4.com>

Two things, first you should resubmit this patch series with a proper
header [PATCH 0/N ... ] posting.

Second:

> @@ -1363,14 +1363,14 @@ sunvnet_start_xmit_common(struct sk_buff *skb, struct net_device *dev,
>  			rt = ip_route_output_key(dev_net(dev), &fl4);
>  			if (!IS_ERR(rt)) {
>  				skb_dst_set(skb, &rt->dst);
> -				icmp_send(skb, ICMP_DEST_UNREACH,
> -					  ICMP_FRAG_NEEDED,
> -					  htonl(localmtu));
> +				icmp_ndo_send(skb, ICMP_DEST_UNREACH,
> +					      ICMP_FRAG_NEEDED,
> +					      htonl(localmtu));
>  			}
>  		}

Well, obviously if the saddr could be wrong here then this invalidates
the route lookup done in the lines above your changes.

It looks like this code is just making sure the ICMP path is routable
which is kinda bogus because that is the icmp code's job.  So very
likely the right thing to do is to remove all of that route lookup
and check code entirely.  And that matches what all the other instances
of driver icmp calls in your patces do.