[PATCH iproute2 0/3] Improve iplink index, alias and name parameters handling

[PATCH iproute2 0/3] Improve iplink index, alias and name parameters handling

[Serhey Popovych]

In this series I present following improvements:

  1) Check index is greather than zero and forbid
     specifying it multiple times in iplink_parse().
     Use 0 instead of -1 as special value in iplink_modify().

  2) Do not stop parameters processing after alias given.
     Check alias length does not exceed IFALIASZ - 1.

  3) Drop redundant name parameter length checks in
     iplink_vxcan.c and link_veth.c.

See individual patch description message for details.

Thanks,
Serhii

Serhey Popovych (3):
  iplink: Improve index parameter handling
  iplink: Process "alias" parameter correctly
  iplink: Kill redundant network device name checks

 ip/iplink.c       |   21 ++++++++++-----------
 ip/iplink_vxcan.c |    8 +++-----
 ip/link_veth.c    |    8 +++-----
 3 files changed, 16 insertions(+), 21 deletions(-)

-- 
1.7.10.4

[PATCH iproute2 1/3] iplink: Improve index parameter handling

[Serhey Popovych]

Correctly check for valid network device index supplied on
command line: indexes are always greather than zero. Check
for duplicate "index" argument.

Initialize @index to 0 to simplify handling it in iplink_modify().
Other callers (link_veth.c, iplink_vxcan.c) already did so.

No need to initialize ifi_index with 0 since it is already
initialized at the @struct req initialization time and not
modified in iplink_parse().

Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
---
 ip/iplink.c |   14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/ip/iplink.c b/ip/iplink.c
index 1e685cc..4f9c169 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -586,8 +586,10 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
 			*name = *argv;
 		} else if (strcmp(*argv, "index") == 0) {
 			NEXT_ARG();
+			if (*index)
+				duparg("index", *argv);
 			*index = atoi(*argv);
-			if (*index < 0)
+			if (*index <= 0)
 				invarg("Invalid \"index\" value", *argv);
 		} else if (matches(*argv, "link") == 0) {
 			NEXT_ARG();
@@ -886,7 +888,7 @@ static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv)
 	char *name = NULL;
 	char *link = NULL;
 	char *type = NULL;
-	int index = -1;
+	int index = 0;
 	int group;
 	struct link_util *lu = NULL;
 	struct iplink_req req = {
@@ -922,7 +924,6 @@ static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv)
 				return -1;
 			}
 
-			req.i.ifi_index = 0;
 			addattr32(&req.n, sizeof(req), IFLA_GROUP, group);
 			if (rtnl_talk(&rth, &req.n, NULL) < 0)
 				return -2;
@@ -936,7 +937,7 @@ static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv)
 				"Not enough information: \"dev\" argument is required.\n");
 			exit(-1);
 		}
-		if (cmd == RTM_NEWLINK && index != -1) {
+		if (cmd == RTM_NEWLINK && index) {
 			fprintf(stderr,
 				"index can be used only when creating devices.\n");
 			exit(-1);
@@ -964,10 +965,7 @@ static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv)
 			addattr_l(&req.n, sizeof(req), IFLA_LINK, &ifindex, 4);
 		}
 
-		if (index == -1)
-			req.i.ifi_index = 0;
-		else
-			req.i.ifi_index = index;
+		req.i.ifi_index = index;
 	}
 
 	if (name) {
-- 
1.7.10.4

Re: [PATCH iproute2 1/3] iplink: Improve index parameter handling

[Stephen Hemminger]

On Mon, 18 Dec 2017 20:54:06 +0200
Serhey Popovych <serhe.popovych@gmail.com> wrote:

> diff --git a/ip/iplink.c b/ip/iplink.c
> index 1e685cc..4f9c169 100644
> --- a/ip/iplink.c
> +++ b/ip/iplink.c
> @@ -586,8 +586,10 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
>  			*name = *argv;
>  		} else if (strcmp(*argv, "index") == 0) {
>  			NEXT_ARG();
> +			if (*index)
> +				duparg("index", *argv);
>  			*index = atoi(*argv);
> -			if (*index < 0)
> +			if (*index <= 0)

Why not use strtoul instead of atoi?

Re: [PATCH iproute2 1/3] iplink: Improve index parameter handling

[Serhey Popovich]

[-- Attachment #1.1: Type: text/plain, Size: 1239 bytes --]

Stephen Hemminger wrote:
> On Mon, 18 Dec 2017 20:54:06 +0200
> Serhey Popovych <serhe.popovych@gmail.com> wrote:
> 
>> diff --git a/ip/iplink.c b/ip/iplink.c
>> index 1e685cc..4f9c169 100644
>> --- a/ip/iplink.c
>> +++ b/ip/iplink.c
>> @@ -586,8 +586,10 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
>>  			*name = *argv;
>>  		} else if (strcmp(*argv, "index") == 0) {
>>  			NEXT_ARG();
>> +			if (*index)
>> +				duparg("index", *argv);
>>  			*index = atoi(*argv);
>> -			if (*index < 0)
>> +			if (*index <= 0)
> 
> Why not use strtoul instead of atoi?
Do not see reason for strtoul() instead atoi():

  1) main arg: indexes in kernel represented as "int", which is
     signed. <= 0 values are reserved for various special purposes
     (see net/core/fib_rules.c on how device matching implemented).

     Configuring network device manually with index <= 0 is not correct
     (however possible). Kernel itself never chooses ifindex <= 0.

     Having unsigned int > 0x7fffffff actually means index <= 0.

  2) this is not single place in iproute2 where it is used: not
     going to remove last user.

  3) make changes clear and transparent for review.

Thanks.

> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH iproute2 1/3] iplink: Improve index parameter handling

[Stephen Hemminger]

[-- Attachment #1: Type: text/plain, Size: 1630 bytes --]

On Mon, 18 Dec 2017 23:02:07 +0200
Serhey Popovich <serhe.popovych@gmail.com> wrote:

> Stephen Hemminger wrote:
> > On Mon, 18 Dec 2017 20:54:06 +0200
> > Serhey Popovych <serhe.popovych@gmail.com> wrote:
> >   
> >> diff --git a/ip/iplink.c b/ip/iplink.c
> >> index 1e685cc..4f9c169 100644
> >> --- a/ip/iplink.c
> >> +++ b/ip/iplink.c
> >> @@ -586,8 +586,10 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
> >>  			*name = *argv;
> >>  		} else if (strcmp(*argv, "index") == 0) {
> >>  			NEXT_ARG();
> >> +			if (*index)
> >> +				duparg("index", *argv);
> >>  			*index = atoi(*argv);
> >> -			if (*index < 0)
> >> +			if (*index <= 0)  
> > 
> > Why not use strtoul instead of atoi?  
> Do not see reason for strtoul() instead atoi():
> 
>   1) main arg: indexes in kernel represented as "int", which is
>      signed. <= 0 values are reserved for various special purposes
>      (see net/core/fib_rules.c on how device matching implemented).
> 
>      Configuring network device manually with index <= 0 is not correct
>      (however possible). Kernel itself never chooses ifindex <= 0.
> 
>      Having unsigned int > 0x7fffffff actually means index <= 0.
> 
>   2) this is not single place in iproute2 where it is used: not
>      going to remove last user.
> 
>   3) make changes clear and transparent for review.

I would rather all of iproute2 correctly handles unsigned values.
Too much code is old K&R style C "the world is an int" and "who needs
to check for negative".

There already is get_unsigned() in iproute2 util functions.
Why not use that?


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH iproute2 1/3] iplink: Improve index parameter handling

[Serhey Popovich]

[-- Attachment #1.1: Type: text/plain, Size: 2230 bytes --]

Stephen Hemminger wrote:
> On Mon, 18 Dec 2017 23:02:07 +0200
> Serhey Popovich <serhe.popovych@gmail.com> wrote:
> 
>> Stephen Hemminger wrote:
>>> On Mon, 18 Dec 2017 20:54:06 +0200
>>> Serhey Popovych <serhe.popovych@gmail.com> wrote:
>>>   
>>>> diff --git a/ip/iplink.c b/ip/iplink.c
>>>> index 1e685cc..4f9c169 100644
>>>> --- a/ip/iplink.c
>>>> +++ b/ip/iplink.c
>>>> @@ -586,8 +586,10 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
>>>>  			*name = *argv;
>>>>  		} else if (strcmp(*argv, "index") == 0) {
>>>>  			NEXT_ARG();
>>>> +			if (*index)
>>>> +				duparg("index", *argv);
>>>>  			*index = atoi(*argv);
>>>> -			if (*index < 0)
>>>> +			if (*index <= 0)  
>>>
>>> Why not use strtoul instead of atoi?  
>> Do not see reason for strtoul() instead atoi():
>>
>>   1) main arg: indexes in kernel represented as "int", which is
>>      signed. <= 0 values are reserved for various special purposes
>>      (see net/core/fib_rules.c on how device matching implemented).
>>
>>      Configuring network device manually with index <= 0 is not correct
>>      (however possible). Kernel itself never chooses ifindex <= 0.
>>
>>      Having unsigned int > 0x7fffffff actually means index <= 0.
>>
>>   2) this is not single place in iproute2 where it is used: not
>>      going to remove last user.
>>
>>   3) make changes clear and transparent for review.
> 
> I would rather all of iproute2 correctly handles unsigned values.
> Too much code is old K&R style C "the world is an int" and "who needs
> to check for negative".

You are right :(. I'm just trying to improve things a bit.

> 
> There already is get_unsigned() in iproute2 util functions.
This is good one based on strtoul(). But do we want to submit say
index = (unsigned int)2147483648(0x7fffffff) to the kernel that is
illegal from it's perspective?

Or do you mean I can prepare treewide change to replace atoi() with
get_unsigned()/get_integer() where appropriate?

We already check if (*index < 0) since commit 3c682146aeff
(iplink: forbid negative ifindex and modifying ifindex), and I just
put index == 0 in the same range of invalid indexes.

> Why not use that?
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH iproute2 1/3] iplink: Improve index parameter handling

[Stephen Hemminger]

[-- Attachment #1: Type: text/plain, Size: 2680 bytes --]

On Mon, 18 Dec 2017 23:37:09 +0200
Serhey Popovich <serhe.popovych@gmail.com> wrote:

> Stephen Hemminger wrote:
> > On Mon, 18 Dec 2017 23:02:07 +0200
> > Serhey Popovich <serhe.popovych@gmail.com> wrote:
> >   
> >> Stephen Hemminger wrote:  
> >>> On Mon, 18 Dec 2017 20:54:06 +0200
> >>> Serhey Popovych <serhe.popovych@gmail.com> wrote:
> >>>     
> >>>> diff --git a/ip/iplink.c b/ip/iplink.c
> >>>> index 1e685cc..4f9c169 100644
> >>>> --- a/ip/iplink.c
> >>>> +++ b/ip/iplink.c
> >>>> @@ -586,8 +586,10 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
> >>>>  			*name = *argv;
> >>>>  		} else if (strcmp(*argv, "index") == 0) {
> >>>>  			NEXT_ARG();
> >>>> +			if (*index)
> >>>> +				duparg("index", *argv);
> >>>>  			*index = atoi(*argv);
> >>>> -			if (*index < 0)
> >>>> +			if (*index <= 0)    
> >>>
> >>> Why not use strtoul instead of atoi?    
> >> Do not see reason for strtoul() instead atoi():
> >>
> >>   1) main arg: indexes in kernel represented as "int", which is
> >>      signed. <= 0 values are reserved for various special purposes
> >>      (see net/core/fib_rules.c on how device matching implemented).
> >>
> >>      Configuring network device manually with index <= 0 is not correct
> >>      (however possible). Kernel itself never chooses ifindex <= 0.
> >>
> >>      Having unsigned int > 0x7fffffff actually means index <= 0.
> >>
> >>   2) this is not single place in iproute2 where it is used: not
> >>      going to remove last user.
> >>
> >>   3) make changes clear and transparent for review.  
> > 
> > I would rather all of iproute2 correctly handles unsigned values.
> > Too much code is old K&R style C "the world is an int" and "who needs
> > to check for negative".  
> 
> You are right :(. I'm just trying to improve things a bit.
> 
> > 
> > There already is get_unsigned() in iproute2 util functions.  
> This is good one based on strtoul(). But do we want to submit say
> index = (unsigned int)2147483648(0x7fffffff) to the kernel that is
> illegal from it's perspective?
> 
> Or do you mean I can prepare treewide change to replace atoi() with
> get_unsigned()/get_integer() where appropriate?
> 
> We already check if (*index < 0) since commit 3c682146aeff
> (iplink: forbid negative ifindex and modifying ifindex), and I just
> put index == 0 in the same range of invalid indexes.
> 

The legacy BSD ABI for interfaces uses int, so that sets the upper
bound for kernel.

The netlink ABI limit is u32 for ifindex so technically 1..UINT32_MAX are
possible values but kernel is bound by BSD mistake.

I will take the original patch.



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH iproute2 1/3] iplink: Improve index parameter handling

[Serhey Popovich]

[-- Attachment #1.1: Type: text/plain, Size: 2783 bytes --]

Stephen Hemminger wrote:
> On Mon, 18 Dec 2017 23:37:09 +0200
> Serhey Popovich <serhe.popovych@gmail.com> wrote:
> 
>> Stephen Hemminger wrote:
>>> On Mon, 18 Dec 2017 23:02:07 +0200
>>> Serhey Popovich <serhe.popovych@gmail.com> wrote:
>>>   
>>>> Stephen Hemminger wrote:  
>>>>> On Mon, 18 Dec 2017 20:54:06 +0200
>>>>> Serhey Popovych <serhe.popovych@gmail.com> wrote:
>>>>>     
>>>>>> diff --git a/ip/iplink.c b/ip/iplink.c
>>>>>> index 1e685cc..4f9c169 100644
>>>>>> --- a/ip/iplink.c
>>>>>> +++ b/ip/iplink.c
>>>>>> @@ -586,8 +586,10 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
>>>>>>  			*name = *argv;
>>>>>>  		} else if (strcmp(*argv, "index") == 0) {
>>>>>>  			NEXT_ARG();
>>>>>> +			if (*index)
>>>>>> +				duparg("index", *argv);
>>>>>>  			*index = atoi(*argv);
>>>>>> -			if (*index < 0)
>>>>>> +			if (*index <= 0)    
>>>>>
>>>>> Why not use strtoul instead of atoi?    
>>>> Do not see reason for strtoul() instead atoi():
>>>>
>>>>   1) main arg: indexes in kernel represented as "int", which is
>>>>      signed. <= 0 values are reserved for various special purposes
>>>>      (see net/core/fib_rules.c on how device matching implemented).
>>>>
>>>>      Configuring network device manually with index <= 0 is not correct
>>>>      (however possible). Kernel itself never chooses ifindex <= 0.
>>>>
>>>>      Having unsigned int > 0x7fffffff actually means index <= 0.
>>>>
>>>>   2) this is not single place in iproute2 where it is used: not
>>>>      going to remove last user.
>>>>
>>>>   3) make changes clear and transparent for review.  
>>>
>>> I would rather all of iproute2 correctly handles unsigned values.
>>> Too much code is old K&R style C "the world is an int" and "who needs
>>> to check for negative".  
>>
>> You are right :(. I'm just trying to improve things a bit.
>>
>>>
>>> There already is get_unsigned() in iproute2 util functions.  
>> This is good one based on strtoul(). But do we want to submit say
>> index = (unsigned int)2147483648(0x7fffffff) to the kernel that is
>> illegal from it's perspective?
>>
>> Or do you mean I can prepare treewide change to replace atoi() with
>> get_unsigned()/get_integer() where appropriate?
>>
>> We already check if (*index < 0) since commit 3c682146aeff
>> (iplink: forbid negative ifindex and modifying ifindex), and I just
>> put index == 0 in the same range of invalid indexes.
>>
> 
> The legacy BSD ABI for interfaces uses int, so that sets the upper
> bound for kernel.
> 
> The netlink ABI limit is u32 for ifindex so technically 1..UINT32_MAX are
> possible values but kernel is bound by BSD mistake.
Thank you for in depth explanation!

> 
> I will take the original patch.
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[PATCH iproute2 2/3] iplink: Process "alias" parameter correctly

[Serhey Popovych]

Do not stop parameters processing after "alias" parameter: it might
not be a last one. Seems copy pasted from "type" parameter code.

Check it's length does not exceed IFALIASZ - 1. Better we warn
than get RTNL error.

Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
---
 ip/iplink.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/ip/iplink.c b/ip/iplink.c
index 4f9c169..4c96711 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -770,11 +770,12 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
 			argc--; argv++;
 			break;
 		} else if (matches(*argv, "alias") == 0) {
+			len = strlen(*argv);
+			if (len >= IFALIASZ)
+				invarg("alias too long\n", *argv);
 			NEXT_ARG();
 			addattr_l(&req->n, sizeof(*req), IFLA_IFALIAS,
-				  *argv, strlen(*argv));
-			argc--; argv++;
-			break;
+				  *argv, len);
 		} else if (strcmp(*argv, "group") == 0) {
 			NEXT_ARG();
 			if (*group != -1)
-- 
1.7.10.4

[PATCH iproute2 3/3] iplink: Kill redundant network device name checks

[Serhey Popovych]

Since commit 625df645b703 (Check user supplied interface name lengths)
iplink_parse() validates network device name using check_ifname()
helpers.

Remove redundant "name" length checks from iplink_parse() callers.

Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
---
 ip/iplink_vxcan.c |    8 +++-----
 ip/link_veth.c    |    8 +++-----
 2 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/ip/iplink_vxcan.c b/ip/iplink_vxcan.c
index 680f640..c13224c 100644
--- a/ip/iplink_vxcan.c
+++ b/ip/iplink_vxcan.c
@@ -38,7 +38,7 @@ static int vxcan_parse_opt(struct link_util *lu, int argc, char **argv,
 	char *link = NULL;
 	char *type = NULL;
 	int index = 0;
-	int err, len;
+	int err;
 	struct rtattr *data;
 	int group;
 	struct ifinfomsg *ifm, *peer_ifm;
@@ -66,10 +66,8 @@ static int vxcan_parse_opt(struct link_util *lu, int argc, char **argv,
 		return err;
 
 	if (name) {
-		len = strlen(name) + 1;
-		if (len > IFNAMSIZ)
-			invarg("\"name\" too long\n", *argv);
-		addattr_l(hdr, 1024, IFLA_IFNAME, name, len);
+		addattr_l(hdr, 1024,
+			  IFLA_IFNAME, name, strlen(name) + 1);
 	}
 
 	peer_ifm = RTA_DATA(data);
diff --git a/ip/link_veth.c b/ip/link_veth.c
index a368827..fcfd1ef 100644
--- a/ip/link_veth.c
+++ b/ip/link_veth.c
@@ -36,7 +36,7 @@ static int veth_parse_opt(struct link_util *lu, int argc, char **argv,
 	char *link = NULL;
 	char *type = NULL;
 	int index = 0;
-	int err, len;
+	int err;
 	struct rtattr *data;
 	int group;
 	struct ifinfomsg *ifm, *peer_ifm;
@@ -64,10 +64,8 @@ static int veth_parse_opt(struct link_util *lu, int argc, char **argv,
 		return err;
 
 	if (name) {
-		len = strlen(name) + 1;
-		if (len > IFNAMSIZ)
-			invarg("\"name\" too long\n", *argv);
-		addattr_l(hdr, 1024, IFLA_IFNAME, name, len);
+		addattr_l(hdr, 1024,
+			  IFLA_IFNAME, name, strlen(name) + 1);
 	}
 
 	peer_ifm = RTA_DATA(data);
-- 
1.7.10.4