[PATCH net] ipv4: Add ICMPv6 support when parse route ipproto

[PATCH net] ipv4: Add ICMPv6 support when parse route ipproto

[Hangbin Liu]

For ip rules, we need to use 'ipproto ipv6-icmp' to match ICMPv6 headers.
But for ip -6 route, currently we only support tcp, udp and icmp.

Add ICMPv6 support so we can match ipv6-icmp rules for route lookup.

Reported-by: Jianlin Shi <jishi@redhat.com>
Fixes: eacb9384a3fe ("ipv6: support sport, dport and ip_proto in RTM_GETROUTE")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 net/ipv4/netlink.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/ipv4/netlink.c b/net/ipv4/netlink.c
index f86bb4f06609..95601237521f 100644
--- a/net/ipv4/netlink.c
+++ b/net/ipv4/netlink.c
@@ -3,6 +3,7 @@
 #include <linux/types.h>
 #include <net/net_namespace.h>
 #include <net/netlink.h>
+#include <linux/in6.h>
 #include <net/ip.h>
 
 int rtm_getroute_parse_ip_proto(struct nlattr *attr, u8 *ip_proto,
@@ -14,6 +15,7 @@ int rtm_getroute_parse_ip_proto(struct nlattr *attr, u8 *ip_proto,
 	case IPPROTO_TCP:
 	case IPPROTO_UDP:
 	case IPPROTO_ICMP:
+	case IPPROTO_ICMPV6:
 		return 0;
 	default:
 		NL_SET_ERR_MSG(extack, "Unsupported ip proto");
-- 
2.19.2

Re: [PATCH net] ipv4: Add ICMPv6 support when parse route ipproto

[Sabrina Dubroca]

2019-02-25, 15:47:00 +0800, Hangbin Liu wrote:
> @@ -14,6 +15,7 @@ int rtm_getroute_parse_ip_proto(struct nlattr *attr, u8 *ip_proto,
>  	case IPPROTO_TCP:
>  	case IPPROTO_UDP:
>  	case IPPROTO_ICMP:
> +	case IPPROTO_ICMPV6:

Is IPPROTO_ICMPV6 supposed to be valid in the IPv4 code path calling
this function? inet_rtm_getroute_build_skb() doesn't seem to handle
it. Likewise, userspace could pass IPPROTO_ICMP from the IPv6 caller.

Also, should that be guarded by #if IS_ENABLED(CONFIG_IPV6) ?

>  		return 0;
>  	default:
>  		NL_SET_ERR_MSG(extack, "Unsupported ip proto");
> -- 
> 2.19.2
> 

-- 
Sabrina

Re: [PATCH net] ipv4: Add ICMPv6 support when parse route ipproto

[David Ahern]

On 2/25/19 4:14 AM, Sabrina Dubroca wrote:
> 2019-02-25, 15:47:00 +0800, Hangbin Liu wrote:
>> @@ -14,6 +15,7 @@ int rtm_getroute_parse_ip_proto(struct nlattr *attr, u8 *ip_proto,
>>  	case IPPROTO_TCP:
>>  	case IPPROTO_UDP:
>>  	case IPPROTO_ICMP:
>> +	case IPPROTO_ICMPV6:
> 
> Is IPPROTO_ICMPV6 supposed to be valid in the IPv4 code path calling
> this function? 

no. I do not see how that makes sense.

Re: [PATCH net] ipv4: Add ICMPv6 support when parse route ipproto

[Hangbin Liu]

On Mon, Feb 25, 2019 at 11:46:51AM -0700, David Ahern wrote:
> On 2/25/19 4:14 AM, Sabrina Dubroca wrote:
> > 2019-02-25, 15:47:00 +0800, Hangbin Liu wrote:
> >> @@ -14,6 +15,7 @@ int rtm_getroute_parse_ip_proto(struct nlattr *attr, u8 *ip_proto,
> >>  	case IPPROTO_TCP:
> >>  	case IPPROTO_UDP:
> >>  	case IPPROTO_ICMP:
> >> +	case IPPROTO_ICMPV6:
> > 
> > Is IPPROTO_ICMPV6 supposed to be valid in the IPv4 code path calling
> > this function? 
> 
> no. I do not see how that makes sense.

I also thought about this issue. Currently we didn't check the ipproto in both
IPv4 and IPv6. You can set icmp in ip6 rules or icmpv6 in ipv4 rules.
This looks don't make any serious problem. It's just a user mis-configuration,
the kernel check the proto number and won't match normal IP/IPv6 headers.

But yes, we should make it more strict, do you think if I should add a new
rtm_getroute_parse_ip6_proto() function, or just add a family parameter
in previous function?

The #if IS_ENABLED(CONFIG_IPV6) guardian makes sense. I will fix it.

Thanks
Hangbin

Re: [PATCH net] ipv4: Add ICMPv6 support when parse route ipproto

[David Ahern]

On 2/25/19 7:17 PM, Hangbin Liu wrote:
> I also thought about this issue. Currently we didn't check the ipproto in both
> IPv4 and IPv6. You can set icmp in ip6 rules or icmpv6 in ipv4 rules.
> This looks don't make any serious problem. It's just a user mis-configuration,
> the kernel check the proto number and won't match normal IP/IPv6 headers.
> 
> But yes, we should make it more strict, do you think if I should add a new
> rtm_getroute_parse_ip6_proto() function, or just add a family parameter
> in previous function?

I see now. rtm_getroute_parse_ip_proto is used for ipv4 and ipv6. For v4
IPPROTO_ICMPV6 should not be allowed and for v6 IPPROTO_ICMP should
fail. You could a version argument to rtm_getroute_parse_ip_proto and
fail as needed.

Re: [PATCH net] ipv4: Add ICMPv6 support when parse route ipproto

[Hangbin Liu]

Hi David,
On Mon, Feb 25, 2019 at 07:23:33PM -0700, David Ahern wrote:
> On 2/25/19 7:17 PM, Hangbin Liu wrote:
> > I also thought about this issue. Currently we didn't check the ipproto in both
> > IPv4 and IPv6. You can set icmp in ip6 rules or icmpv6 in ipv4 rules.
> > This looks don't make any serious problem. It's just a user mis-configuration,
> > the kernel check the proto number and won't match normal IP/IPv6 headers.
> > 
> > But yes, we should make it more strict, do you think if I should add a new
> > rtm_getroute_parse_ip6_proto() function, or just add a family parameter
> > in previous function?
> 
> I see now. rtm_getroute_parse_ip_proto is used for ipv4 and ipv6. For v4
> IPPROTO_ICMPV6 should not be allowed and for v6 IPPROTO_ICMP should
> fail. You could a version argument to rtm_getroute_parse_ip_proto and
> fail as needed.

Sorry I didn't get here. Do you mean add an IPv6 version of
rtm_getroute_parse_ip_proto?

Thanks
Hangbin

Re: [PATCH net] ipv4: Add ICMPv6 support when parse route ipproto

[Sabrina Dubroca]

2019-02-26, 11:48:54 +0800, Hangbin Liu wrote:
> Hi David,
> On Mon, Feb 25, 2019 at 07:23:33PM -0700, David Ahern wrote:
> > On 2/25/19 7:17 PM, Hangbin Liu wrote:
> > > I also thought about this issue. Currently we didn't check the ipproto in both
> > > IPv4 and IPv6. You can set icmp in ip6 rules or icmpv6 in ipv4 rules.
> > > This looks don't make any serious problem. It's just a user mis-configuration,
> > > the kernel check the proto number and won't match normal IP/IPv6 headers.
> > > 
> > > But yes, we should make it more strict, do you think if I should add a new
> > > rtm_getroute_parse_ip6_proto() function, or just add a family parameter
> > > in previous function?
> > 
> > I see now. rtm_getroute_parse_ip_proto is used for ipv4 and ipv6. For v4
> > IPPROTO_ICMPV6 should not be allowed and for v6 IPPROTO_ICMP should
> > fail. You could a version argument to rtm_getroute_parse_ip_proto and
> > fail as needed.
> 
> Sorry I didn't get here. Do you mean add an IPv6 version of
> rtm_getroute_parse_ip_proto?

Add an argument to rtm_getroute_parse_ip_proto that tells what IP
version to use, and then handle IPPROTO_ICMP/IPPROTO_ICMPV6 depending
on that.

For example:

int rtm_getroute_parse_ip_proto(struct nlattr *attr, u8 *ip_proto,
				bool ipv6, struct netlink_ext_ack *extack)

And pass false from ipv4/true from ipv6.

-- 
Sabrina

[PATCH v2 net] ipv4: Add ICMPv6 support when parse route ipproto

[Hangbin Liu]

For ip rules, we need to use 'ipproto ipv6-icmp' to match ICMPv6 headers.
But for ip -6 route, currently we only support tcp, udp and icmp.

Add ICMPv6 support so we can match ipv6-icmp rules for route lookup.

v2: As David Ahern and Sabrina Dubroca suggested, Add an argument to
rtm_getroute_parse_ip_proto() to handle ICMP/ICMPv6 with different family.

Reported-by: Jianlin Shi <jishi@redhat.com>
Fixes: eacb9384a3fe ("ipv6: support sport, dport and ip_proto in RTM_GETROUTE")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 include/net/ip.h   |  2 +-
 net/ipv4/netlink.c | 17 +++++++++++++----
 net/ipv4/route.c   |  2 +-
 net/ipv6/route.c   |  3 ++-
 4 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/include/net/ip.h b/include/net/ip.h
index 8866bfce6121..80300e8a6280 100644
--- a/include/net/ip.h
+++ b/include/net/ip.h
@@ -716,7 +716,7 @@ extern int sysctl_icmp_msgs_burst;
 int ip_misc_proc_init(void);
 #endif
 
-int rtm_getroute_parse_ip_proto(struct nlattr *attr, u8 *ip_proto,
+int rtm_getroute_parse_ip_proto(struct nlattr *attr, u8 *ip_proto, u8 family,
 				struct netlink_ext_ack *extack);
 
 #endif	/* _IP_H */
diff --git a/net/ipv4/netlink.c b/net/ipv4/netlink.c
index f86bb4f06609..d8e3a1fb8e82 100644
--- a/net/ipv4/netlink.c
+++ b/net/ipv4/netlink.c
@@ -3,9 +3,10 @@
 #include <linux/types.h>
 #include <net/net_namespace.h>
 #include <net/netlink.h>
+#include <linux/in6.h>
 #include <net/ip.h>
 
-int rtm_getroute_parse_ip_proto(struct nlattr *attr, u8 *ip_proto,
+int rtm_getroute_parse_ip_proto(struct nlattr *attr, u8 *ip_proto, u8 family,
 				struct netlink_ext_ack *extack)
 {
 	*ip_proto = nla_get_u8(attr);
@@ -13,11 +14,19 @@ int rtm_getroute_parse_ip_proto(struct nlattr *attr, u8 *ip_proto,
 	switch (*ip_proto) {
 	case IPPROTO_TCP:
 	case IPPROTO_UDP:
+		return 0;
 	case IPPROTO_ICMP:
+		if (family != AF_INET)
+			break;
+		return 0;
+#if IS_ENABLED(CONFIG_IPV6)
+	case IPPROTO_ICMPV6:
+		if (family != AF_INET6)
+			break;
 		return 0;
-	default:
-		NL_SET_ERR_MSG(extack, "Unsupported ip proto");
-		return -EOPNOTSUPP;
+#endif
 	}
+	NL_SET_ERR_MSG(extack, "Unsupported ip proto");
+	return -EOPNOTSUPP;
 }
 EXPORT_SYMBOL_GPL(rtm_getroute_parse_ip_proto);
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 5163b64f8fb3..7bb9128c8363 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -2803,7 +2803,7 @@ static int inet_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
 
 	if (tb[RTA_IP_PROTO]) {
 		err = rtm_getroute_parse_ip_proto(tb[RTA_IP_PROTO],
-						  &ip_proto, extack);
+						  &ip_proto, AF_INET, extack);
 		if (err)
 			return err;
 	}
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index ce15dc4ccbfa..6f949aa5b64f 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -4889,7 +4889,8 @@ static int inet6_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
 
 	if (tb[RTA_IP_PROTO]) {
 		err = rtm_getroute_parse_ip_proto(tb[RTA_IP_PROTO],
-						  &fl6.flowi6_proto, extack);
+						  &fl6.flowi6_proto, AF_INET6,
+						  extack);
 		if (err)
 			goto errout;
 	}
-- 
2.19.2

Re: [PATCH v2 net] ipv4: Add ICMPv6 support when parse route ipproto

[David Miller]

From: Hangbin Liu <liuhangbin@gmail.com>
Date: Wed, 27 Feb 2019 16:15:29 +0800

> For ip rules, we need to use 'ipproto ipv6-icmp' to match ICMPv6 headers.
> But for ip -6 route, currently we only support tcp, udp and icmp.
> 
> Add ICMPv6 support so we can match ipv6-icmp rules for route lookup.
> 
> v2: As David Ahern and Sabrina Dubroca suggested, Add an argument to
> rtm_getroute_parse_ip_proto() to handle ICMP/ICMPv6 with different family.
> 
> Reported-by: Jianlin Shi <jishi@redhat.com>
> Fixes: eacb9384a3fe ("ipv6: support sport, dport and ip_proto in RTM_GETROUTE")
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

Applied and queued up for -stable, thanks.