[PATCH net] net/route: remove ip route rtm_src_len, rtm_dst_len valid check

[PATCH net] net/route: remove ip route rtm_src_len, rtm_dst_len valid check

[Hangbin Liu]

In patch set e266afa9c7af ("Merge branch
'net-use-strict-checks-in-doit-handlers'") we added a check for
rtm_src_len, rtm_dst_len, which will cause cmds like
"ip route get 192.0.2.0/24" failed.

There is no sense to restrict rtm_src_len, rtm_dst_len to 32 for IPv4,
or 128 for IPv6. Remove this check.

Reported-by: Jianlin Shi <jishi@redhat.com>
Fixes: d0440029831b ("net: ipv4: ipmr: perform strict checks also for doit handlers")
Fixes: a00302b60777 ("net: ipv4: route: perform strict checks also for doit handlers")
Fixes: 0eff0a274104 ("net: ipv6: route: perform strict checks also for doit handlers")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 net/ipv4/ipmr.c  | 10 +---------
 net/ipv4/route.c | 10 +---------
 net/ipv6/route.c | 10 +---------
 3 files changed, 3 insertions(+), 27 deletions(-)

diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 6e68def66822..293a0189ff4e 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -2500,9 +2500,7 @@ static int ipmr_rtm_valid_getroute_req(struct sk_buff *skb,
 					      rtm_ipv4_policy, extack);
 
 	rtm = nlmsg_data(nlh);
-	if ((rtm->rtm_src_len && rtm->rtm_src_len != 32) ||
-	    (rtm->rtm_dst_len && rtm->rtm_dst_len != 32) ||
-	    rtm->rtm_tos || rtm->rtm_table || rtm->rtm_protocol ||
+	if (rtm->rtm_tos || rtm->rtm_table || rtm->rtm_protocol ||
 	    rtm->rtm_scope || rtm->rtm_type || rtm->rtm_flags) {
 		NL_SET_ERR_MSG(extack, "ipv4: Invalid values in header for multicast route get request");
 		return -EINVAL;
@@ -2513,12 +2511,6 @@ static int ipmr_rtm_valid_getroute_req(struct sk_buff *skb,
 	if (err)
 		return err;
 
-	if ((tb[RTA_SRC] && !rtm->rtm_src_len) ||
-	    (tb[RTA_DST] && !rtm->rtm_dst_len)) {
-		NL_SET_ERR_MSG(extack, "ipv4: rtm_src_len and rtm_dst_len must be 32 for IPv4");
-		return -EINVAL;
-	}
-
 	for (i = 0; i <= RTA_MAX; i++) {
 		if (!tb[i])
 			continue;
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 87e979f2b74a..fb36780ee415 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -3063,9 +3063,7 @@ static int inet_rtm_valid_getroute_req(struct sk_buff *skb,
 					      rtm_ipv4_policy, extack);
 
 	rtm = nlmsg_data(nlh);
-	if ((rtm->rtm_src_len && rtm->rtm_src_len != 32) ||
-	    (rtm->rtm_dst_len && rtm->rtm_dst_len != 32) ||
-	    rtm->rtm_table || rtm->rtm_protocol ||
+	if (rtm->rtm_table || rtm->rtm_protocol ||
 	    rtm->rtm_scope || rtm->rtm_type) {
 		NL_SET_ERR_MSG(extack, "ipv4: Invalid values in header for route get request");
 		return -EINVAL;
@@ -3083,12 +3081,6 @@ static int inet_rtm_valid_getroute_req(struct sk_buff *skb,
 	if (err)
 		return err;
 
-	if ((tb[RTA_SRC] && !rtm->rtm_src_len) ||
-	    (tb[RTA_DST] && !rtm->rtm_dst_len)) {
-		NL_SET_ERR_MSG(extack, "ipv4: rtm_src_len and rtm_dst_len must be 32 for IPv4");
-		return -EINVAL;
-	}
-
 	for (i = 0; i <= RTA_MAX; i++) {
 		if (!tb[i])
 			continue;
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index affb51c11a25..4e82d4fd1b53 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -5714,9 +5714,7 @@ static int inet6_rtm_valid_getroute_req(struct sk_buff *skb,
 					      rtm_ipv6_policy, extack);
 
 	rtm = nlmsg_data(nlh);
-	if ((rtm->rtm_src_len && rtm->rtm_src_len != 128) ||
-	    (rtm->rtm_dst_len && rtm->rtm_dst_len != 128) ||
-	    rtm->rtm_table || rtm->rtm_protocol || rtm->rtm_scope ||
+	if (rtm->rtm_table || rtm->rtm_protocol || rtm->rtm_scope ||
 	    rtm->rtm_type) {
 		NL_SET_ERR_MSG_MOD(extack, "Invalid values in header for get route request");
 		return -EINVAL;
@@ -5732,12 +5730,6 @@ static int inet6_rtm_valid_getroute_req(struct sk_buff *skb,
 	if (err)
 		return err;
 
-	if ((tb[RTA_SRC] && !rtm->rtm_src_len) ||
-	    (tb[RTA_DST] && !rtm->rtm_dst_len)) {
-		NL_SET_ERR_MSG_MOD(extack, "rtm_src_len and rtm_dst_len must be 128 for IPv6");
-		return -EINVAL;
-	}
-
 	for (i = 0; i <= RTA_MAX; i++) {
 		if (!tb[i])
 			continue;
-- 
2.19.2

Re: [PATCH net] net/route: remove ip route rtm_src_len, rtm_dst_len valid check

[David Ahern]

On 1/10/20 1:24 AM, Hangbin Liu wrote:
> In patch set e266afa9c7af ("Merge branch
> 'net-use-strict-checks-in-doit-handlers'") we added a check for
> rtm_src_len, rtm_dst_len, which will cause cmds like
> "ip route get 192.0.2.0/24" failed.

kernel does not handle route gets for a range. Any output is specific to
the prefix (192.0.2.0 in your example) so it seems to me the /24 request
should fail.

Re: [PATCH net] net/route: remove ip route rtm_src_len, rtm_dst_len valid check

[Hangbin Liu]

On Fri, Jan 10, 2020 at 02:48:03PM -0700, David Ahern wrote:
> On 1/10/20 1:24 AM, Hangbin Liu wrote:
> > In patch set e266afa9c7af ("Merge branch
> > 'net-use-strict-checks-in-doit-handlers'") we added a check for
> > rtm_src_len, rtm_dst_len, which will cause cmds like
> > "ip route get 192.0.2.0/24" failed.
> 
> kernel does not handle route gets for a range. Any output is specific to
> the prefix (192.0.2.0 in your example) so it seems to me the /24 request
> should fail.
> 

OK, so we should check all the range field if NETLINK_F_STRICT_CHK supplied,
like the following patch, right?


diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 87e979f2b74a..a681c5cfbf13 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -3063,8 +3063,7 @@ static int inet_rtm_valid_getroute_req(struct sk_buff *skb,
 					      rtm_ipv4_policy, extack);
 
 	rtm = nlmsg_data(nlh);
-	if ((rtm->rtm_src_len && rtm->rtm_src_len != 32) ||
-	    (rtm->rtm_dst_len && rtm->rtm_dst_len != 32) ||
+	if (rtm->rtm_src_len || rtm->rtm_dst_len ||
 	    rtm->rtm_table || rtm->rtm_protocol ||
 	    rtm->rtm_scope || rtm->rtm_type) {
 		NL_SET_ERR_MSG(extack, "ipv4: Invalid values in header for route get request");
@@ -3083,12 +3082,6 @@ static int inet_rtm_valid_getroute_req(struct sk_buff *skb,
 	if (err)
 		return err;
 
-	if ((tb[RTA_SRC] && !rtm->rtm_src_len) ||
-	    (tb[RTA_DST] && !rtm->rtm_dst_len)) {
-		NL_SET_ERR_MSG(extack, "ipv4: rtm_src_len and rtm_dst_len must be 32 for IPv4");
-		return -EINVAL;
-	}
-
 	for (i = 0; i <= RTA_MAX; i++) {
 		if (!tb[i])
 			continue;


Thanks
Hangbin

Re: [PATCH net] net/route: remove ip route rtm_src_len, rtm_dst_len valid check

[David Ahern]

On 1/10/20 6:18 PM, Hangbin Liu wrote:
> On Fri, Jan 10, 2020 at 02:48:03PM -0700, David Ahern wrote:
>> On 1/10/20 1:24 AM, Hangbin Liu wrote:
>>> In patch set e266afa9c7af ("Merge branch
>>> 'net-use-strict-checks-in-doit-handlers'") we added a check for
>>> rtm_src_len, rtm_dst_len, which will cause cmds like
>>> "ip route get 192.0.2.0/24" failed.
>>
>> kernel does not handle route gets for a range. Any output is specific to
>> the prefix (192.0.2.0 in your example) so it seems to me the /24 request
>> should fail.
>>
> 
> OK, so we should check all the range field if NETLINK_F_STRICT_CHK supplied,
> like the following patch, right?

a dst_len / src_len of 32 (or 128 for v6) is ok. It still means only the
prefix is used for the route get. That's why it was coded this way as
part of the change for stricter checking.

Re: [PATCH net] net/route: remove ip route rtm_src_len, rtm_dst_len valid check

[Hangbin Liu]

On Sat, Jan 11, 2020 at 10:38:23AM -0700, David Ahern wrote:
> On 1/10/20 6:18 PM, Hangbin Liu wrote:
> > On Fri, Jan 10, 2020 at 02:48:03PM -0700, David Ahern wrote:
> >> On 1/10/20 1:24 AM, Hangbin Liu wrote:
> >>> In patch set e266afa9c7af ("Merge branch
> >>> 'net-use-strict-checks-in-doit-handlers'") we added a check for
> >>> rtm_src_len, rtm_dst_len, which will cause cmds like
> >>> "ip route get 192.0.2.0/24" failed.
> >>
> >> kernel does not handle route gets for a range. Any output is specific to
> >> the prefix (192.0.2.0 in your example) so it seems to me the /24 request
> >> should fail.
> >>
> > 
> > OK, so we should check all the range field if NETLINK_F_STRICT_CHK supplied,
> > like the following patch, right?
> 
> a dst_len / src_len of 32 (or 128 for v6) is ok. It still means only the
> prefix is used for the route get. That's why it was coded this way as
> part of the change for stricter checking.
> 

Ah, I see now. Thanks for the interpretation.

Regards
Hangbin