[PATCH] rtnetlink: Actually use the policy for the IFLA_VF_INFO

[PATCH] rtnetlink: Actually use the policy for the IFLA_VF_INFO

[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")
Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 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

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index a2b90e1fc115..7d5dc347bf7c 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -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;
-- 
2.1.4

Re: [PATCH] rtnetlink: Actually use the policy for the IFLA_VF_INFO

[Daniel Borkmann]

Hi Jason,

On 07/01/2015 12:52 AM, Jason Gunthorpe wrote:
> 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(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 01ced4a..7d63551 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -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;
-- 
1.9.3

Re: [PATCH] rtnetlink: Actually use the policy for the IFLA_VF_INFO

[Daniel Borkmann]

On 07/01/2015 11:36 AM, Daniel Borkmann wrote:
> Hi Jason,
>
> On 07/01/2015 12:52 AM, Jason Gunthorpe wrote:
>> 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.

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

Thanks,
Daniel

Re: [PATCH] rtnetlink: Actually use the policy for the IFLA_VF_INFO

[Jason Gunthorpe]

On Thu, Jul 02, 2015 at 10:34:54AM +0200, Daniel Borkmann wrote:
> >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

Re: [PATCH] rtnetlink: Actually use the policy for the IFLA_VF_INFO

[Daniel Borkmann]

On 07/03/2015 01:06 AM, Jason Gunthorpe wrote:
> On Thu, Jul 02, 2015 at 10:34:54AM +0200, Daniel Borkmann wrote:
>>> 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

Re: [PATCH] rtnetlink: Actually use the policy for the IFLA_VF_INFO

[Jason Gunthorpe]

On Wed, Jul 01, 2015 at 11:36:15AM +0200, Daniel Borkmann wrote:
> Hi Jason,
> 
> On 07/01/2015 12:52 AM, Jason Gunthorpe wrote:
> >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

> 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