Netfilter-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH net 1/5] icmp: introduce helper for NAT'd source address in ndo context
@ 2020-02-09 14:31 Jason A. Donenfeld
  2020-02-09 14:31 ` [PATCH net 2/5] gtp: use icmp_ndo_send helper Jason A. Donenfeld
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Jason A. Donenfeld @ 2020-02-09 14:31 UTC (permalink / raw)
  To: netfilter-devel, netdev; +Cc: Jason A. Donenfeld, Florian Westphal

The ICMP routines use the source address for two reasons:

    1. Rate-limiting ICMP transmissions based on source address, so
       that one source address cannot provoke a flood of replies. If
       the source address is wrong, the rate limiting will be
       incorrectly applied.

    2. Choosing the interface and hence new source address of the
       generated ICMP packet. If the original packet source address
       is wrong, ICMP replies will be sent from the wrong source
       address, resulting in either a misdelivery, infoleak, or just
       general network admin confusion.

Most of the time, the icmp_send and icmpv6_send routines can just reach
down into the skb's IP header to determine the saddr. However, if
icmp_send or icmpv6_send is being called from a network device driver --
there are a few in the tree -- then it's possible that by the time
icmp_send or icmpv6_send looks at the packet, the packet's source
address has already been transformed by SNAT or MASQUERADE or some other
transformation that CONNTRACK knows about. In this case, the packet's
source address is most certainly the *wrong* source address to be used
for the purpose of ICMP replies.

Rather, the source address we want to use for ICMP replies is the
original one, from before the transformation occurred.

Fortunately, it's very easy to just ask CONNTRACK if it knows about this
packet, and if so, how to fix it up. The saddr is the only field in the
header we need to fix up, for the purposes of the subsequent processing
in the icmp_send and icmpv6_send functions, so we do the lookup very
early on, so that the rest of the ICMP machinery can progress as usual.

We don't want to pollute the non-driver path, though, so we introduce
this as a helper to be called by places that actually make use of
this, as suggested by Florian.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: Florian Westphal <fw@strlen.de>
---
Dave - this fixes bugs, so net.git seemed fitting, but if you think this
is a bit invasive or will require several more series of development, I
don't mind working on this in net-next.git instead.

 include/linux/icmpv6.h |  6 ++++++
 include/net/icmp.h     |  6 ++++++
 net/ipv4/icmp.c        | 14 ++++++++++++++
 net/ipv6/ip6_icmp.c    | 15 +++++++++++++++
 4 files changed, 41 insertions(+)

diff --git a/include/linux/icmpv6.h b/include/linux/icmpv6.h
index ef1cbb5f454f..47cea3779eb1 100644
--- a/include/linux/icmpv6.h
+++ b/include/linux/icmpv6.h
@@ -31,6 +31,12 @@ static inline void icmpv6_send(struct sk_buff *skb,
 }
 #endif
 
+#if IS_ENABLED(CONFIG_NF_CONNTRACK)
+void icmpv6_ndo_send(struct sk_buff *skb_in, int type, int code, __be32 info);
+#else
+#define icmpv6_ndo_send icmpv6_send
+#endif
+
 extern int				icmpv6_init(void);
 extern int				icmpv6_err_convert(u8 type, u8 code,
 							   int *err);
diff --git a/include/net/icmp.h b/include/net/icmp.h
index 5d4bfdba9adf..30257506c038 100644
--- a/include/net/icmp.h
+++ b/include/net/icmp.h
@@ -43,6 +43,12 @@ static inline void icmp_send(struct sk_buff *skb_in, int type, int code, __be32
 	__icmp_send(skb_in, type, code, info, &IPCB(skb_in)->opt);
 }
 
+#if IS_ENABLED(CONFIG_NF_CONNTRACK)
+void icmp_ndo_send(struct sk_buff *skb_in, int type, int code, __be32 info);
+#else
+#define icmp_ndo_send icmp_send
+#endif
+
 int icmp_rcv(struct sk_buff *skb);
 int icmp_err(struct sk_buff *skb, u32 info);
 int icmp_init(void);
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 18068ed42f25..54b923bca655 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -748,6 +748,20 @@ out:;
 }
 EXPORT_SYMBOL(__icmp_send);
 
+#if IS_ENABLED(CONFIG_NF_CONNTRACK)
+#include <net/netfilter/nf_conntrack.h>
+void icmp_ndo_send(struct sk_buff *skb_in, int type, int code, __be32 info)
+{
+	enum ip_conntrack_info ctinfo;
+	struct nf_conn *ct;
+
+	ct = nf_ct_get(skb_in, &ctinfo);
+	if (ct)
+		ip_hdr(skb_in)->saddr = ct->tuplehash[0].tuple.src.u3.ip;
+	icmp_send(skb_in, type, code, info);
+}
+EXPORT_SYMBOL(icmp_ndo_send);
+#endif
 
 static void icmp_socket_deliver(struct sk_buff *skb, u32 info)
 {
diff --git a/net/ipv6/ip6_icmp.c b/net/ipv6/ip6_icmp.c
index 02045494c24c..c79bf2d616cf 100644
--- a/net/ipv6/ip6_icmp.c
+++ b/net/ipv6/ip6_icmp.c
@@ -45,4 +45,19 @@ void icmpv6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info)
 	rcu_read_unlock();
 }
 EXPORT_SYMBOL(icmpv6_send);
+
+#if IS_ENABLED(CONFIG_NF_CONNTRACK)
+#include <net/netfilter/nf_conntrack.h>
+void icmpv6_ndo_send(struct sk_buff *skb_in, int type, int code, __be32 info)
+{
+	enum ip_conntrack_info ctinfo;
+	struct nf_conn *ct;
+
+	ct = nf_ct_get(skb_in, &ctinfo);
+	if (ct)
+		ipv6_hdr(skb_in)->saddr = ct->tuplehash[0].tuple.src.u3.in6;
+	icmpv6_send(skb_in, type, code, info);
+}
+EXPORT_SYMBOL(icmpv6_ndo_send);
+#endif
 #endif
-- 
2.25.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH net 2/5] gtp: use icmp_ndo_send helper
  2020-02-09 14:31 [PATCH net 1/5] icmp: introduce helper for NAT'd source address in ndo context Jason A. Donenfeld
@ 2020-02-09 14:31 ` Jason A. Donenfeld
  2020-02-09 14:31 ` [PATCH net 3/5] sunvnet: " Jason A. Donenfeld
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Jason A. Donenfeld @ 2020-02-09 14:31 UTC (permalink / raw)
  To: netfilter-devel, netdev; +Cc: Jason A. Donenfeld, Harald Welte

Because gtp 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@zx2c4.com>
Cc: Harald Welte <laforge@gnumonks.org>
---
 drivers/net/gtp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index af07ea760b35..672cd2caf2fb 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -546,8 +546,8 @@ static int gtp_build_skb_ip4(struct sk_buff *skb, struct net_device *dev,
 	    mtu < ntohs(iph->tot_len)) {
 		netdev_dbg(dev, "packet too big, fragmentation needed\n");
 		memset(IPCB(skb), 0, sizeof(*IPCB(skb)));
-		icmp_send(skb, ICMP_DEST_UNREACH, ICMP_FRAG_NEEDED,
-			  htonl(mtu));
+		icmp_ndo_send(skb, ICMP_DEST_UNREACH, ICMP_FRAG_NEEDED,
+			      htonl(mtu));
 		goto err_rt;
 	}
 
-- 
2.25.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH net 3/5] sunvnet: use icmp_ndo_send helper
  2020-02-09 14:31 [PATCH net 1/5] icmp: introduce helper for NAT'd source address in ndo context Jason A. Donenfeld
  2020-02-09 14:31 ` [PATCH net 2/5] gtp: use icmp_ndo_send helper Jason A. Donenfeld
@ 2020-02-09 14:31 ` " Jason A. Donenfeld
  2020-02-10 11:36   ` David Miller
  2020-02-09 14:31 ` [PATCH net 4/5] wireguard: " Jason A. Donenfeld
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 9+ messages in thread
From: Jason A. Donenfeld @ 2020-02-09 14:31 UTC (permalink / raw)
  To: netfilter-devel, netdev; +Cc: Jason A. Donenfeld, Shannon Nelson

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@zx2c4.com>
Cc: Shannon Nelson <shannon.nelson@oracle.com>
---
 drivers/net/ethernet/sun/sunvnet_common.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/sun/sunvnet_common.c b/drivers/net/ethernet/sun/sunvnet_common.c
index c23ce838ff63..9948c00fe625 100644
--- a/drivers/net/ethernet/sun/sunvnet_common.c
+++ b/drivers/net/ethernet/sun/sunvnet_common.c
@@ -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));
 			}
 		}
 #if IS_ENABLED(CONFIG_IPV6)
 		else if (skb->protocol == htons(ETH_P_IPV6))
-			icmpv6_send(skb, ICMPV6_PKT_TOOBIG, 0, localmtu);
+			icmpv6_ndo_send(skb, ICMPV6_PKT_TOOBIG, 0, localmtu);
 #endif
 		goto out_dropped;
 	}
-- 
2.25.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH net 4/5] wireguard: use icmp_ndo_send helper
  2020-02-09 14:31 [PATCH net 1/5] icmp: introduce helper for NAT'd source address in ndo context Jason A. Donenfeld
  2020-02-09 14:31 ` [PATCH net 2/5] gtp: use icmp_ndo_send helper Jason A. Donenfeld
  2020-02-09 14:31 ` [PATCH net 3/5] sunvnet: " Jason A. Donenfeld
@ 2020-02-09 14:31 ` " Jason A. Donenfeld
  2020-02-09 14:31 ` [PATCH net 5/5] xfrm: interface: " Jason A. Donenfeld
  2020-02-10 11:50 ` [PATCH net 1/5] icmp: introduce helper for NAT'd source address in ndo context Florian Westphal
  4 siblings, 0 replies; 9+ messages in thread
From: Jason A. Donenfeld @ 2020-02-09 14:31 UTC (permalink / raw)
  To: netfilter-devel, netdev; +Cc: Jason A. Donenfeld

Because wireguard 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@zx2c4.com>
---
 drivers/net/wireguard/device.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireguard/device.c b/drivers/net/wireguard/device.c
index 16b19824b9ad..43db442b1373 100644
--- a/drivers/net/wireguard/device.c
+++ b/drivers/net/wireguard/device.c
@@ -203,9 +203,9 @@ static netdev_tx_t wg_xmit(struct sk_buff *skb, struct net_device *dev)
 err:
 	++dev->stats.tx_errors;
 	if (skb->protocol == htons(ETH_P_IP))
-		icmp_send(skb, ICMP_DEST_UNREACH, ICMP_HOST_UNREACH, 0);
+		icmp_ndo_send(skb, ICMP_DEST_UNREACH, ICMP_HOST_UNREACH, 0);
 	else if (skb->protocol == htons(ETH_P_IPV6))
-		icmpv6_send(skb, ICMPV6_DEST_UNREACH, ICMPV6_ADDR_UNREACH, 0);
+		icmpv6_ndo_send(skb, ICMPV6_DEST_UNREACH, ICMPV6_ADDR_UNREACH, 0);
 	kfree_skb(skb);
 	return ret;
 }
-- 
2.25.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH net 5/5] xfrm: interface: use icmp_ndo_send helper
  2020-02-09 14:31 [PATCH net 1/5] icmp: introduce helper for NAT'd source address in ndo context Jason A. Donenfeld
                   ` (2 preceding siblings ...)
  2020-02-09 14:31 ` [PATCH net 4/5] wireguard: " Jason A. Donenfeld
@ 2020-02-09 14:31 ` " Jason A. Donenfeld
  2020-02-10 11:50 ` [PATCH net 1/5] icmp: introduce helper for NAT'd source address in ndo context Florian Westphal
  4 siblings, 0 replies; 9+ messages in thread
From: Jason A. Donenfeld @ 2020-02-09 14:31 UTC (permalink / raw)
  To: netfilter-devel, netdev
  Cc: Jason A. Donenfeld, Nicolas Dichtel, Steffen Klassert

Because xfrmi 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@zx2c4.com>
Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/xfrm/xfrm_interface.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/xfrm/xfrm_interface.c b/net/xfrm/xfrm_interface.c
index dc651a628dcf..3361e3ac5714 100644
--- a/net/xfrm/xfrm_interface.c
+++ b/net/xfrm/xfrm_interface.c
@@ -300,10 +300,10 @@ xfrmi_xmit2(struct sk_buff *skb, struct net_device *dev, struct flowi *fl)
 			if (mtu < IPV6_MIN_MTU)
 				mtu = IPV6_MIN_MTU;
 
-			icmpv6_send(skb, ICMPV6_PKT_TOOBIG, 0, mtu);
+			icmpv6_ndo_send(skb, ICMPV6_PKT_TOOBIG, 0, mtu);
 		} else {
-			icmp_send(skb, ICMP_DEST_UNREACH, ICMP_FRAG_NEEDED,
-				  htonl(mtu));
+			icmp_ndo_send(skb, ICMP_DEST_UNREACH, ICMP_FRAG_NEEDED,
+				      htonl(mtu));
 		}
 
 		dst_release(dst);
-- 
2.25.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net 3/5] sunvnet: use icmp_ndo_send helper
  2020-02-09 14:31 ` [PATCH net 3/5] sunvnet: " Jason A. Donenfeld
@ 2020-02-10 11:36   ` David Miller
  2020-02-10 13:34     ` Jason A. Donenfeld
  0 siblings, 1 reply; 9+ messages in thread
From: David Miller @ 2020-02-10 11:36 UTC (permalink / raw)
  To: Jason; +Cc: netfilter-devel, netdev, shannon.nelson

From: "Jason A. Donenfeld" <Jason@zx2c4.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@zx2c4.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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net 1/5] icmp: introduce helper for NAT'd source address in ndo context
  2020-02-09 14:31 [PATCH net 1/5] icmp: introduce helper for NAT'd source address in ndo context Jason A. Donenfeld
                   ` (3 preceding siblings ...)
  2020-02-09 14:31 ` [PATCH net 5/5] xfrm: interface: " Jason A. Donenfeld
@ 2020-02-10 11:50 ` Florian Westphal
  2020-02-10 13:33   ` Jason A. Donenfeld
  4 siblings, 1 reply; 9+ messages in thread
From: Florian Westphal @ 2020-02-10 11:50 UTC (permalink / raw)
  To: Jason A. Donenfeld; +Cc: netfilter-devel, netdev, Florian Westphal

Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> +#if IS_ENABLED(CONFIG_NF_CONNTRACK)

CONFIG_NF_NAT would be preferrable even though it makes little
difference in practice I guess.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net 1/5] icmp: introduce helper for NAT'd source address in ndo context
  2020-02-10 11:50 ` [PATCH net 1/5] icmp: introduce helper for NAT'd source address in ndo context Florian Westphal
@ 2020-02-10 13:33   ` Jason A. Donenfeld
  0 siblings, 0 replies; 9+ messages in thread
From: Jason A. Donenfeld @ 2020-02-10 13:33 UTC (permalink / raw)
  To: Florian Westphal; +Cc: netfilter-devel, Netdev

On Mon, Feb 10, 2020 at 12:50 PM Florian Westphal <fw@strlen.de> wrote:
>
> Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > +#if IS_ENABLED(CONFIG_NF_CONNTRACK)
>
> CONFIG_NF_NAT would be preferrable even though it makes little
> difference in practice I guess.

Sure, will do for v2, thanks.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net 3/5] sunvnet: use icmp_ndo_send helper
  2020-02-10 11:36   ` David Miller
@ 2020-02-10 13:34     ` Jason A. Donenfeld
  0 siblings, 0 replies; 9+ messages in thread
From: Jason A. Donenfeld @ 2020-02-10 13:34 UTC (permalink / raw)
  To: David Miller; +Cc: netfilter-devel, Netdev, shannon.nelson

On Mon, Feb 10, 2020 at 12:36 PM David Miller <davem@davemloft.net> wrote:
>
> From: "Jason A. Donenfeld" <Jason@zx2c4.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@zx2c4.com>
>
> Two things, first you should resubmit this patch series with a proper
> header [PATCH 0/N ... ] posting.

Ack, will do for v2.

>
> 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.

Good point. I'll simplify that by just getting rid of the superfluous
route lookup and make sure that the other ones are okay too.

Thanks,
Jason

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-09 14:31 [PATCH net 1/5] icmp: introduce helper for NAT'd source address in ndo context Jason A. Donenfeld
2020-02-09 14:31 ` [PATCH net 2/5] gtp: use icmp_ndo_send helper Jason A. Donenfeld
2020-02-09 14:31 ` [PATCH net 3/5] sunvnet: " Jason A. Donenfeld
2020-02-10 11:36   ` David Miller
2020-02-10 13:34     ` Jason A. Donenfeld
2020-02-09 14:31 ` [PATCH net 4/5] wireguard: " Jason A. Donenfeld
2020-02-09 14:31 ` [PATCH net 5/5] xfrm: interface: " Jason A. Donenfeld
2020-02-10 11:50 ` [PATCH net 1/5] icmp: introduce helper for NAT'd source address in ndo context Florian Westphal
2020-02-10 13:33   ` Jason A. Donenfeld

Netfilter-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netfilter-devel/0 netfilter-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netfilter-devel netfilter-devel/ https://lore.kernel.org/netfilter-devel \
		netfilter-devel@vger.kernel.org
	public-inbox-index netfilter-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netfilter-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git