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

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

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

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

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

On Mon, 18 Dec 2017 20:54:06 +0200
Serhey Popovych [off-list ref] wrote:

Why not use strtoul instead of atoi?

Stephen Hemminger wrote:

On Mon, 18 Dec 2017 20:54:06 +0200
Serhey Popovych [off-list ref] wrote:

quoted

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.

On Mon, 18 Dec 2017 23:02:07 +0200
Serhey Popovich [off-list ref] wrote:

Stephen Hemminger wrote:

quoted

On Mon, 18 Dec 2017 20:54:06 +0200
Serhey Popovych [off-list ref] wrote:
  

quoted

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?

Stephen Hemminger wrote:

On Mon, 18 Dec 2017 23:02:07 +0200
Serhey Popovich [off-list ref] wrote:

quoted

Stephen Hemminger wrote:

quoted

On Mon, 18 Dec 2017 20:54:06 +0200
Serhey Popovych [off-list ref] wrote:
  

quoted

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?

On Mon, 18 Dec 2017 23:37:09 +0200
Serhey Popovich [off-list ref] wrote:

Stephen Hemminger wrote:

quoted

On Mon, 18 Dec 2017 23:02:07 +0200
Serhey Popovich [off-list ref] wrote:
  

quoted

Stephen Hemminger wrote:  

quoted

On Mon, 18 Dec 2017 20:54:06 +0200
Serhey Popovych [off-list ref] wrote:
    

quoted

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.

quoted

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.

Stephen Hemminger wrote:

On Mon, 18 Dec 2017 23:37:09 +0200
Serhey Popovich [off-list ref] wrote:

quoted

Stephen Hemminger wrote:

quoted

On Mon, 18 Dec 2017 23:02:07 +0200
Serhey Popovich [off-list ref] wrote:
  

quoted

Stephen Hemminger wrote:  

quoted

On Mon, 18 Dec 2017 20:54:06 +0200
Serhey Popovych [off-list ref] wrote:
    

quoted

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.

quoted

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.