* [PATCH] rtnetlink: Actually use the policy for the IFLA_VF_INFO
@ 2015-06-30 22:52 Jason Gunthorpe
2015-07-01 9:36 ` Daniel Borkmann
0 siblings, 1 reply; 6+ messages in thread
From: Jason Gunthorpe @ 2015-06-30 22:52 UTC (permalink / raw)
To: netdev, Daniel Borkmann, Mitch Williams, Jeff Kirsher
Cc: Nicolas Dichtel, Jiri Pirko, Thomas Graf, David S. Miller
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] rtnetlink: Actually use the policy for the IFLA_VF_INFO
2015-06-30 22:52 [PATCH] rtnetlink: Actually use the policy for the IFLA_VF_INFO Jason Gunthorpe
@ 2015-07-01 9:36 ` Daniel Borkmann
2015-07-02 8:34 ` Daniel Borkmann
2015-07-02 16:23 ` Jason Gunthorpe
0 siblings, 2 replies; 6+ messages in thread
From: Daniel Borkmann @ 2015-07-01 9:36 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: netdev, Mitch Williams, Jeff Kirsher, Nicolas Dichtel, Jiri Pirko,
Thomas Graf, David S. Miller, Chris Wright
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] rtnetlink: Actually use the policy for the IFLA_VF_INFO
2015-07-01 9:36 ` Daniel Borkmann
@ 2015-07-02 8:34 ` Daniel Borkmann
2015-07-02 23:06 ` Jason Gunthorpe
2015-07-02 16:23 ` Jason Gunthorpe
1 sibling, 1 reply; 6+ messages in thread
From: Daniel Borkmann @ 2015-07-02 8:34 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: netdev, Mitch Williams, Jeff Kirsher, Nicolas Dichtel, Jiri Pirko,
Thomas Graf, David S. Miller, Chris Wright
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] rtnetlink: Actually use the policy for the IFLA_VF_INFO
2015-07-01 9:36 ` Daniel Borkmann
2015-07-02 8:34 ` Daniel Borkmann
@ 2015-07-02 16:23 ` Jason Gunthorpe
1 sibling, 0 replies; 6+ messages in thread
From: Jason Gunthorpe @ 2015-07-02 16:23 UTC (permalink / raw)
To: Daniel Borkmann
Cc: netdev, Mitch Williams, Jeff Kirsher, Nicolas Dichtel, Jiri Pirko,
Thomas Graf, David S. Miller, Chris Wright
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] rtnetlink: Actually use the policy for the IFLA_VF_INFO
2015-07-02 8:34 ` Daniel Borkmann
@ 2015-07-02 23:06 ` Jason Gunthorpe
2015-07-03 21:52 ` Daniel Borkmann
0 siblings, 1 reply; 6+ messages in thread
From: Jason Gunthorpe @ 2015-07-02 23:06 UTC (permalink / raw)
To: Daniel Borkmann
Cc: netdev, Mitch Williams, Jeff Kirsher, Nicolas Dichtel, Jiri Pirko,
Thomas Graf, David S. Miller, Chris Wright
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] rtnetlink: Actually use the policy for the IFLA_VF_INFO
2015-07-02 23:06 ` Jason Gunthorpe
@ 2015-07-03 21:52 ` Daniel Borkmann
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Borkmann @ 2015-07-03 21:52 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: netdev, Mitch Williams, Jeff Kirsher, Nicolas Dichtel, Jiri Pirko,
Thomas Graf, David S. Miller, Chris Wright
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-07-03 21:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-06-30 22:52 [PATCH] rtnetlink: Actually use the policy for the IFLA_VF_INFO Jason Gunthorpe
2015-07-01 9:36 ` Daniel Borkmann
2015-07-02 8:34 ` Daniel Borkmann
2015-07-02 23:06 ` Jason Gunthorpe
2015-07-03 21:52 ` Daniel Borkmann
2015-07-02 16:23 ` Jason Gunthorpe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).