[iproute PATCH 0/2] Check MAC address length when changing it

[iproute PATCH 0/2] Check MAC address length when changing it

[Phil Sutter]

Due to minimal checking in kernel space, MAC address setting was
problematic in multiple ways:

- Setting an overly long MAC address was accepted and the extra parts
  simply ignored.

- Setting an overly short MAC address for VFs was accepted and the
  missing part filled with random garbage.

While patch 1 makes sure that in the second case above missing parts are
padded with zero, patch 2 implements a real solution for both by
comparing new and old address lengths prior to accepting the input. For
VFs it is simply assumed that the length must match that of the PF's
address.

Phil Sutter (2):
  iplink: Add missing variable initialization
  iplink: Check address length via netlink

 ip/iplink.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 51 insertions(+), 3 deletions(-)

-- 
2.8.2

[iproute PATCH 1/2] iplink: Add missing variable initialization

[Phil Sutter]

Without this, we might feed garbage to the kernel when the address is
shorter than expected.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/iplink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ip/iplink.c b/ip/iplink.c
index d2e586b6d1332..4cb9bab66b916 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -273,7 +273,7 @@ static int iplink_parse_vf(int vf, int *argcp, char ***argvp,
 	while (NEXT_ARG_OK()) {
 		NEXT_ARG();
 		if (matches(*argv, "mac") == 0) {
-			struct ifla_vf_mac ivm;
+			struct ifla_vf_mac ivm = { 0 };
 
 			NEXT_ARG();
 			ivm.vf = vf;
-- 
2.8.2

[iproute PATCH 2/2] iplink: Check address length via netlink

[Phil Sutter]

This is a feature which was lost during the conversion to netlink
interface: If the device exists and a user tries to change the link
layer address, query the kernel for the old address first and reject the
new one if sizes differ.

This patch adds the same check when setting VF address by assuming same
length as PF device.

Note that at least for VFs the check can't be done in kernel space since
struct ifla_vf_mac lacks a length field and due to netlink padding the
exact size can't be communicated to the kernel.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/iplink.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 50 insertions(+), 2 deletions(-)

diff --git a/ip/iplink.c b/ip/iplink.c
index 4cb9bab66b916..68e5faea3581d 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -237,6 +237,36 @@ struct iplink_req {
 	char			buf[1024];
 };
 
+static int nl_get_ll_addr_len(unsigned int dev_index)
+{
+	int len;
+	struct iplink_req req = {
+		.n = {
+			.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg)),
+			.nlmsg_type = RTM_GETLINK,
+			.nlmsg_flags = NLM_F_REQUEST
+		},
+		.i = {
+			.ifi_family = preferred_family,
+			.ifi_index = dev_index,
+		}
+	};
+	struct rtattr *tb[IFLA_MAX+1];
+
+	if (rtnl_talk(&rth, &req.n, &req.n, sizeof(req)) < 0)
+		return -1;
+
+	len = req.n.nlmsg_len - NLMSG_LENGTH(sizeof(req.i));
+	if (len < 0)
+		return -1;
+
+	parse_rtattr_flags(tb, IFLA_MAX, IFLA_RTA(&req.i), len, NLA_F_NESTED);
+	if (!tb[IFLA_ADDRESS])
+		return -1;
+
+	return RTA_PAYLOAD(tb[IFLA_ADDRESS]);
+}
+
 static int iplink_parse_vf(int vf, int *argcp, char ***argvp,
 			   struct iplink_req *req, int dev_index)
 {
@@ -274,12 +304,19 @@ static int iplink_parse_vf(int vf, int *argcp, char ***argvp,
 		NEXT_ARG();
 		if (matches(*argv, "mac") == 0) {
 			struct ifla_vf_mac ivm = { 0 };
+			int halen = nl_get_ll_addr_len(dev_index);
 
 			NEXT_ARG();
 			ivm.vf = vf;
 			len = ll_addr_a2n((char *)ivm.mac, 32, *argv);
 			if (len < 0)
 				return -1;
+			if (halen > 0 && len != halen) {
+				fprintf(stderr,
+					"Invalid address length %d - must be %d bytes\n",
+					len, halen);
+				return -1;
+			}
 			addattr_l(&req->n, sizeof(*req), IFLA_VF_MAC, &ivm, sizeof(ivm));
 		} else if (matches(*argv, "vlan") == 0) {
 			struct ifla_vf_vlan ivv;
@@ -428,6 +465,7 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
 	int numrxqueues = -1;
 	int dev_index = 0;
 	int link_netnsid = -1;
+	int addr_len = 0;
 
 	*group = -1;
 	ret = argc;
@@ -452,10 +490,10 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
 			*link = *argv;
 		} else if (matches(*argv, "address") == 0) {
 			NEXT_ARG();
-			len = ll_addr_a2n(abuf, sizeof(abuf), *argv);
+			addr_len = ll_addr_a2n(abuf, sizeof(abuf), *argv);
 			if (len < 0)
 				return -1;
-			addattr_l(&req->n, sizeof(*req), IFLA_ADDRESS, abuf, len);
+			addattr_l(&req->n, sizeof(*req), IFLA_ADDRESS, abuf, addr_len);
 		} else if (matches(*argv, "broadcast") == 0 ||
 			   strcmp(*argv, "brd") == 0) {
 			NEXT_ARG();
@@ -677,6 +715,16 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
 		argc--; argv++;
 	}
 
+	if (dev_index && addr_len) {
+		int halen = nl_get_ll_addr_len(dev_index);
+		if (halen >= 0 && halen != addr_len) {
+			fprintf(stderr,
+			        "Invalid address length %d - must be %d bytes\n",
+			        addr_len, halen);
+			return -1;
+		}
+	}
+
 	return ret - argc;
 }
 
-- 
2.8.2

Re: [iproute PATCH 0/2] Check MAC address length when changing it

[Stephen Hemminger]

On Thu, 16 Jun 2016 16:19:38 +0200
Phil Sutter <phil@nwl.cc> wrote:

> Due to minimal checking in kernel space, MAC address setting was
> problematic in multiple ways:
> 
> - Setting an overly long MAC address was accepted and the extra parts
>   simply ignored.
> 
> - Setting an overly short MAC address for VFs was accepted and the
>   missing part filled with random garbage.
> 
> While patch 1 makes sure that in the second case above missing parts are
> padded with zero, patch 2 implements a real solution for both by
> comparing new and old address lengths prior to accepting the input. For
> VFs it is simply assumed that the length must match that of the PF's
> address.
> 
> Phil Sutter (2):
>   iplink: Add missing variable initialization
>   iplink: Check address length via netlink
> 
>  ip/iplink.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 51 insertions(+), 3 deletions(-)
> 

Sure. Applied