Skip to contentSkip to conversation

rtnetlink: Actually use the policy for the IFLA_VF_INFO

PATCH In review Jason Gunthorpe 2 participants 6 messages Jun 30, 2015 – Jul 3, 2015

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Jason Gunthorpe

author
#0 lore raw

PATCH 1/1 rtnetlink: Actually use the policy for the IFLA_VF_INFO

It turns out the policy was defined but never actually checked, so lets check it.

Fixes ebc08a6f47ee ("rtnetlink: Add VF config code to rtnetlink")
Signed-off-by Jason Gunthorpe <[email protected]>
---
 net/core/rtnetlink.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

DaveM: This shouldn't be applied until someone with the hardware that uses this can test it to make sure the policy is actually correct and matches what iproute does..

I noticed this by inspection when investigating how to properly use netlink in another area. Compile tested only.

Daniel/Mitch/Jeff: Can you test this?

I suspect the absence of these checks allows user space to cause a read past the end of a buffer?

I dropped the ifla_vfinfo_policy to match how IFLA_VF_PORTS/IFLA_VF_PORT is working but I wonder if that should be changed as well?

Thanks,
Jason

net/core/rtnetlink.c +5 -4
index a2b90e1fc115..7d5dc347bf7c 100644
@@ -1258,10 +1258,6 @@ static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = {
 	[IFLA_INFO_SLAVE_DATA]	= { .type = NLA_NESTED },
 };
 
-static const struct nla_policy ifla_vfinfo_policy[IFLA_VF_INFO_MAX+1] = {
-	[IFLA_VF_INFO]		= { .type = NLA_NESTED },
-};
-
 static const struct nla_policy ifla_vf_policy[IFLA_VF_MAX+1] = {
 	[IFLA_VF_MAC]		= { .len = sizeof(struct ifla_vf_mac) },
 	[IFLA_VF_VLAN]		= { .len = sizeof(struct ifla_vf_vlan) },

@@ -1681,6 +1677,7 @@ static int do_setlink(const struct sk_buff *skb,
 	}
 
 	if (tb[IFLA_VFINFO_LIST]) {
+		struct nlattr *vf_attrs[IFLA_VF_MAX + 1];
 		struct nlattr *attr;
 		int rem;
 		nla_for_each_nested(attr, tb[IFLA_VFINFO_LIST], rem) {

@@ -1688,6 +1685,10 @@ static int do_setlink(const struct sk_buff *skb,
 				err = -EINVAL;
 				goto errout;
 			}
+			err = nla_parse_nested(vf_attrs, IFLA_VF_MAX, attr,
+					       ifla_vf_policy);
+			if (err < 0)
+				goto errout;
 			err = do_setvfinfo(dev, attr);
 			if (err < 0)
 				goto errout;
Signature · 1 line
2.1.4
#1 lore raw

Hi Jason,

Quoted 4 lines · Jason Gunthorpe

It turns out the policy was defined but never actually checked, so lets check it.

Fixes ebc08a6f47ee ("rtnetlink: Add VF config code to rtnetlink")

I would argue that the actual commit would be ...

Fixes c02db8c6290b ("rtnetlink: make SR-IOV VF interface symmetric")

... since in ebc08a6f47ee, these members were part of ifla_policy[] which has been validated (if we ignore the fact that it was NLA_BINARY).

So, commit c02db8c6290b moved it into a nested attribute (IFLA_VF_INFO) where we indeed don't do further validation. Imho, we should pass the parsed attribute table from nla_parse_nested() down into do_setvfinfo(), something like the below; I can give it a test run on my ixgbe.

Cheers,
Daniel

 net/core/rtnetlink.c | 184 ++++++++++++++++++++++++++-------------------------
 1 file changed, 94 insertions(+), 90 deletions(-)
net/core/rtnetlink.c +94 -90
index 01ced4a..7d63551 100644
@@ -1328,10 +1328,6 @@ static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = {
 	[IFLA_INFO_SLAVE_DATA]	= { .type = NLA_NESTED },
 };
 
-static const struct nla_policy ifla_vfinfo_policy[IFLA_VF_INFO_MAX+1] = {
-	[IFLA_VF_INFO]		= { .type = NLA_NESTED },
-};
-
 static const struct nla_policy ifla_vf_policy[IFLA_VF_MAX+1] = {
 	[IFLA_VF_MAC]		= { .len = sizeof(struct ifla_vf_mac) },
 	[IFLA_VF_VLAN]		= { .len = sizeof(struct ifla_vf_vlan) },

@@ -1488,96 +1484,98 @@ static int validate_linkmsg(struct net_device *dev, struct nlattr *tb[])
 	return 0;
 }
 
-static int do_setvfinfo(struct net_device *dev, struct nlattr *attr)
+static int do_setvfinfo(struct net_device *dev, struct nlattr **tb)
 {
-	int rem, err = -EINVAL;
-	struct nlattr *vf;
 	const struct net_device_ops *ops = dev->netdev_ops;
+	int err = -EINVAL;
 
-	nla_for_each_nested(vf, attr, rem) {
-		switch (nla_type(vf)) {
-		case IFLA_VF_MAC: {
-			struct ifla_vf_mac *ivm;
-			ivm = nla_data(vf);
-			err = -EOPNOTSUPP;
-			if (ops->ndo_set_vf_mac)
-				err = ops->ndo_set_vf_mac(dev, ivm->vf,
-							  ivm->mac);
-			break;
-		}
-		case IFLA_VF_VLAN: {
-			struct ifla_vf_vlan *ivv;
-			ivv = nla_data(vf);
-			err = -EOPNOTSUPP;
-			if (ops->ndo_set_vf_vlan)
-				err = ops->ndo_set_vf_vlan(dev, ivv->vf,
-							   ivv->vlan,
-							   ivv->qos);
-			break;
-		}
-		case IFLA_VF_TX_RATE: {
-			struct ifla_vf_tx_rate *ivt;
-			struct ifla_vf_info ivf;
-			ivt = nla_data(vf);
-			err = -EOPNOTSUPP;
-			if (ops->ndo_get_vf_config)
-				err = ops->ndo_get_vf_config(dev, ivt->vf,
-							     &ivf);
-			if (err)
-				break;
-			err = -EOPNOTSUPP;
-			if (ops->ndo_set_vf_rate)
-				err = ops->ndo_set_vf_rate(dev, ivt->vf,
-							   ivf.min_tx_rate,
-							   ivt->rate);
-			break;
-		}
-		case IFLA_VF_RATE: {
-			struct ifla_vf_rate *ivt;
-			ivt = nla_data(vf);
-			err = -EOPNOTSUPP;
-			if (ops->ndo_set_vf_rate)
-				err = ops->ndo_set_vf_rate(dev, ivt->vf,
-							   ivt->min_tx_rate,
-							   ivt->max_tx_rate);
-			break;
-		}
-		case IFLA_VF_SPOOFCHK: {
-			struct ifla_vf_spoofchk *ivs;
-			ivs = nla_data(vf);
-			err = -EOPNOTSUPP;
-			if (ops->ndo_set_vf_spoofchk)
-				err = ops->ndo_set_vf_spoofchk(dev, ivs->vf,
-							       ivs->setting);
-			break;
-		}
-		case IFLA_VF_LINK_STATE: {
-			struct ifla_vf_link_state *ivl;
-			ivl = nla_data(vf);
-			err = -EOPNOTSUPP;
-			if (ops->ndo_set_vf_link_state)
-				err = ops->ndo_set_vf_link_state(dev, ivl->vf,
-								 ivl->link_state);
-			break;
-		}
-		case IFLA_VF_RSS_QUERY_EN: {
-			struct ifla_vf_rss_query_en *ivrssq_en;
+	if (tb[IFLA_VF_MAC]) {
+		struct ifla_vf_mac *ivm = nla_data(tb[IFLA_VF_MAC]);
 
-			ivrssq_en = nla_data(vf);
-			err = -EOPNOTSUPP;
-			if (ops->ndo_set_vf_rss_query_en)
-				err = ops->ndo_set_vf_rss_query_en(dev,
-							    ivrssq_en->vf,
-							    ivrssq_en->setting);
-			break;
-		}
-		default:
-			err = -EINVAL;
-			break;
-		}
-		if (err)
-			break;
+		err = -EOPNOTSUPP;
+		if (ops->ndo_set_vf_mac)
+			err = ops->ndo_set_vf_mac(dev, ivm->vf,
+						  ivm->mac);
+		if (err < 0)
+			return err;
+	}
+
+	if (tb[IFLA_VF_VLAN]) {
+		struct ifla_vf_vlan *ivv = nla_data(tb[IFLA_VF_VLAN]);
+
+		err = -EOPNOTSUPP;
+		if (ops->ndo_set_vf_vlan)
+			err = ops->ndo_set_vf_vlan(dev, ivv->vf, ivv->vlan,
+						   ivv->qos);
+		if (err < 0)
+			return err;
+	}
+
+	if (tb[IFLA_VF_TX_RATE]) {
+		struct ifla_vf_tx_rate *ivt = nla_data(tb[IFLA_VF_TX_RATE]);
+		struct ifla_vf_info ivf;
+
+		err = -EOPNOTSUPP;
+		if (ops->ndo_get_vf_config)
+			err = ops->ndo_get_vf_config(dev, ivt->vf, &ivf);
+		if (err < 0)
+			return err;
+
+		err = -EOPNOTSUPP;
+		if (ops->ndo_set_vf_rate)
+			err = ops->ndo_set_vf_rate(dev, ivt->vf,
+						   ivf.min_tx_rate,
+						   ivt->rate);
+		if (err < 0)
+			return err;
+	}
+
+	if (tb[IFLA_VF_RATE]) {
+		struct ifla_vf_rate *ivt = nla_data(tb[IFLA_VF_RATE]);
+
+		err = -EOPNOTSUPP;
+		if (ops->ndo_set_vf_rate)
+			err = ops->ndo_set_vf_rate(dev, ivt->vf,
+						   ivt->min_tx_rate,
+						   ivt->max_tx_rate);
+		if (err < 0)
+			return err;
 	}
+
+	if (tb[IFLA_VF_SPOOFCHK]) {
+		struct ifla_vf_spoofchk *ivs = nla_data(tb[IFLA_VF_SPOOFCHK]);
+
+		err = -EOPNOTSUPP;
+		if (ops->ndo_set_vf_spoofchk)
+			err = ops->ndo_set_vf_spoofchk(dev, ivs->vf,
+						       ivs->setting);
+		if (err < 0)
+			return err;
+	}
+
+	if (tb[IFLA_VF_LINK_STATE]) {
+		struct ifla_vf_link_state *ivl = nla_data(tb[IFLA_VF_LINK_STATE]);
+
+		err = -EOPNOTSUPP;
+		if (ops->ndo_set_vf_link_state)
+			err = ops->ndo_set_vf_link_state(dev, ivl->vf,
+							 ivl->link_state);
+		if (err < 0)
+			return err;
+	}
+
+	if (tb[IFLA_VF_RSS_QUERY_EN]) {
+		struct ifla_vf_rss_query_en *ivrssq_en =
+					nla_data(tb[IFLA_VF_RSS_QUERY_EN]);
+
+		err = -EOPNOTSUPP;
+		if (ops->ndo_set_vf_rss_query_en)
+			err = ops->ndo_set_vf_rss_query_en(dev, ivrssq_en->vf,
+							   ivrssq_en->setting);
+		if (err < 0)
+			return err;
+	}
+
 	return err;
 }
 

@@ -1773,14 +1771,20 @@ static int do_setlink(const struct sk_buff *skb,
 	}
 
 	if (tb[IFLA_VFINFO_LIST]) {
+		struct nlattr *vfinfo[IFLA_VF_MAX + 1];
 		struct nlattr *attr;
 		int rem;
+
 		nla_for_each_nested(attr, tb[IFLA_VFINFO_LIST], rem) {
 			if (nla_type(attr) != IFLA_VF_INFO) {
 				err = -EINVAL;
 				goto errout;
 			}
-			err = do_setvfinfo(dev, attr);
+			err = nla_parse_nested(vfinfo, IFLA_VF_MAX, attr,
+					       ifla_vf_policy);
+			if (err < 0)
+				goto errout;
+			err = do_setvfinfo(dev, vfinfo);
 			if (err < 0)
 				goto errout;
 			status |= DO_SETLINK_NOTIFY;
Signature · 1 line
1.9.3
#2 lore raw
Quoted 12 lines · Daniel Borkmann

Hi Jason,

Quoted 4 lines · Jason Gunthorpe

It turns out the policy was defined but never actually checked, so lets check it.

Fixes ebc08a6f47ee ("rtnetlink: Add VF config code to rtnetlink")

I would argue that the actual commit would be ...

Fixes c02db8c6290b ("rtnetlink: make SR-IOV VF interface symmetric")
Quoted 7 lines

... since in ebc08a6f47ee, these members were part of ifla_policy[] which has been validated (if we ignore the fact that it was NLA_BINARY).

So, commit c02db8c6290b moved it into a nested attribute (IFLA_VF_INFO) where we indeed don't do further validation. Imho, we should pass the parsed attribute table from nla_parse_nested() down into do_setvfinfo(), something like the below; I can give it a test run on my ixgbe.

Sorry for the late reply, something like this looks good from my side.

Thanks,
Daniel

author
#3 lore raw
Quoted 6 lines · Daniel Borkmann
Quoted 4 lines

So, commit c02db8c6290b moved it into a nested attribute (IFLA_VF_INFO) where we indeed don't do further validation. Imho, we should pass the parsed attribute table from nla_parse_nested() down into do_setvfinfo(), something like the below; I can give it a test run on my ixgbe.

Sorry for the late reply, something like this looks good from my side.

Okay, since it is your patch, will you send it to DaveM with my Reported-By?

Cheers,
Jason

#4 lore raw
Quoted 10 lines · Jason Gunthorpe
Quoted 6 lines · Daniel Borkmann
Quoted 4 lines

So, commit c02db8c6290b moved it into a nested attribute (IFLA_VF_INFO) where we indeed don't do further validation. Imho, we should pass the parsed attribute table from nla_parse_nested() down into do_setvfinfo(), something like the below; I can give it a test run on my ixgbe.

Sorry for the late reply, something like this looks good from my side.

Okay, since it is your patch, will you send it to DaveM with my Reported-By?

Either way is fine by me, if you prefer, you can also just update your initial submission. In any case, we should get it fixed eventually.

Thanks,
Daniel

author
#5 lore raw
In reply to Daniel Borkmann
Quoted 11 lines · Daniel Borkmann

Hi Jason,

Quoted 4 lines · Jason Gunthorpe

It turns out the policy was defined but never actually checked, so lets check it.

Fixes ebc08a6f47ee ("rtnetlink: Add VF config code to rtnetlink")

I would argue that the actual commit would be ...

Fixes c02db8c6290b ("rtnetlink: make SR-IOV VF interface symmetric")

Yes, agree

Quoted 4 lines

So, commit c02db8c6290b moved it into a nested attribute (IFLA_VF_INFO) where we indeed don't do further validation. Imho, we should pass the parsed attribute table from nla_parse_nested() down into do_setvfinfo(), something like the below; I can give it a test run on my ixgbe.

Yes, that is saner overall

Jason