| Message ID | 1363184338-15781-1-git-send-email-vyasevic@redhat.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, 13 Mar 2013 10:18:58 -0400 Vlad Yasevich <vyasevic@redhat.com> wrote: > Range/validity checks on rta_type in rtnetlink_rcv_msg() do > not account for flags that may be set. This causes the function > to return -EINVAL when flags are set on the type (for example > NLA_F_NESTED). > > Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> > --- > net/core/rtnetlink.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c > index 1868625..dc5edf1 100644 > --- a/net/core/rtnetlink.c > +++ b/net/core/rtnetlink.c > @@ -2538,7 +2538,7 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh) > struct rtattr *attr = (void *)nlh + NLMSG_ALIGN(min_len); > > while (RTA_OK(attr, attrlen)) { > - unsigned int flavor = attr->rta_type; > + unsigned int flavor = attr->rta_type & NLA_TYPE_MASK; > if (flavor) { > if (flavor > rta_max[sz_idx]) > return -EINVAL; No. This is effectively an ABI change. It adds nothing. The NLA_F_NESTED attribute wasn't in the first generation version of netlink (before my time with Linux). It doesn't make sense to all of sudden start accepting it on requests. Also, then you would expect the query to set the NESTED flag as well, and that would be another ABI change. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/13/2013 11:36 AM, Stephen Hemminger wrote: > On Wed, 13 Mar 2013 10:18:58 -0400 > Vlad Yasevich <vyasevic@redhat.com> wrote: > >> Range/validity checks on rta_type in rtnetlink_rcv_msg() do >> not account for flags that may be set. This causes the function >> to return -EINVAL when flags are set on the type (for example >> NLA_F_NESTED). >> >> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> >> --- >> net/core/rtnetlink.c | 2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c >> index 1868625..dc5edf1 100644 >> --- a/net/core/rtnetlink.c >> +++ b/net/core/rtnetlink.c >> @@ -2538,7 +2538,7 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh) >> struct rtattr *attr = (void *)nlh + NLMSG_ALIGN(min_len); >> >> while (RTA_OK(attr, attrlen)) { >> - unsigned int flavor = attr->rta_type; >> + unsigned int flavor = attr->rta_type & NLA_TYPE_MASK; >> if (flavor) { >> if (flavor > rta_max[sz_idx]) >> return -EINVAL; > > No. This is effectively an ABI change. It adds nothing. > It makes nested IFLA_PROTINFO work that the bridge code expects. Without this change, sending a nested IFLA_PROTINFO causes a EIVNAL return. -vlad > The NLA_F_NESTED attribute wasn't in the first generation version of netlink > (before my time with Linux). It doesn't make sense to all of sudden start > accepting it on requests. Also, then you would expect the query to set > the NESTED flag as well, and that would be another ABI change. > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/13/2013 11:36 AM, Stephen Hemminger wrote: > On Wed, 13 Mar 2013 10:18:58 -0400 > Vlad Yasevich <vyasevic@redhat.com> wrote: > >> Range/validity checks on rta_type in rtnetlink_rcv_msg() do >> not account for flags that may be set. This causes the function >> to return -EINVAL when flags are set on the type (for example >> NLA_F_NESTED). >> >> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> >> --- >> net/core/rtnetlink.c | 2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c >> index 1868625..dc5edf1 100644 >> --- a/net/core/rtnetlink.c >> +++ b/net/core/rtnetlink.c >> @@ -2538,7 +2538,7 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh) >> struct rtattr *attr = (void *)nlh + NLMSG_ALIGN(min_len); >> >> while (RTA_OK(attr, attrlen)) { >> - unsigned int flavor = attr->rta_type; >> + unsigned int flavor = attr->rta_type & NLA_TYPE_MASK; >> if (flavor) { >> if (flavor > rta_max[sz_idx]) >> return -EINVAL; > > No. This is effectively an ABI change. It adds nothing. > > The NLA_F_NESTED attribute wasn't in the first generation version of netlink > (before my time with Linux). It doesn't make sense to all of sudden start > accepting it on requests. Also, then you would expect the query to set > the NESTED flag as well, and that would be another ABI change. > So let me rebuff this a bit more intelligently. 1) NLA_F_NESTED is used by netfilter in a lot of places. It seems that only rtnetlink interface doesn't account for it. 2) The following commit: commit 25c71c75ac87508528db053b818944f3650dd7a6 Author: stephen hemminger <shemminger@vyatta.com> Date: Tue Nov 13 07:53:05 2012 +0000 bridge: bridge port parameters over netlink introduced NLA_F_NESTED usage in rtnetlink for both setlink and getlink, so one could argue that was an ABI change. However, I can prove that without the change I am introducing, one can not use the above mentioned bridge API. Feel free to try it with iproute2 patches I sent earlier ([PATCH iproute2 0/2] Add support for bridge port link information). Now, if we truly want to say that we may not introduce NLA_F_NESTED into the rtnetlink patch, then the above commit to bridge needs to be reworked. Do you still object to this patch? If so, we would have to rework the bridge netlink api. Otherwise, it enables the current API to actually work. Thanks -vlad -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/14/13 at 01:40pm, Vlad Yasevich wrote: > So let me rebuff this a bit more intelligently. > > 1) NLA_F_NESTED is used by netfilter in a lot of places. It seems that > only rtnetlink interface doesn't account for it. Stephen is not wrong, strictly speaking this is an ABI breaker. However, we have been applying NLA_TYPE_MASK in nla_type() and thus in nla_parse(), nlmsg_find_attr(), etc. since the introduction of the current kernel netlink API. static inline int nla_type(const struct nlattr *nla) { return nla->nla_type & NLA_TYPE_MASK; } I have been attempting to get rid of that ugly attribute parsing code in rtnetlink_rcv_msg() but it's hard to get rid of. I shall give this another try. > 2) The following commit: > > commit 25c71c75ac87508528db053b818944f3650dd7a6 > Author: stephen hemminger <shemminger@vyatta.com> > Date: Tue Nov 13 07:53:05 2012 +0000 > > bridge: bridge port parameters over netlink > > introduced NLA_F_NESTED usage in rtnetlink for both setlink and > getlink, so one could argue that was an ABI change. You are right, user space applications that do not apply NLA_TYPE_MASK will no longer see the IFLA_PROTINFO attribute with the above commit. > prove that without the change I am introducing, one can not use the > above mentioned bridge API. Feel free to try it with iproute2 > patches > I sent earlier ([PATCH iproute2 0/2] Add support for bridge port > link information). I am not against your patch but I would love to see the affected code paths to no longer rely on the attribute array provided by the rtnetlink doit function but instead call nla_parse() themselves for the sake of proper validation. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/14/2013 05:28 PM, Thomas Graf wrote: > On 03/14/13 at 01:40pm, Vlad Yasevich wrote: >> So let me rebuff this a bit more intelligently. >> >> 1) NLA_F_NESTED is used by netfilter in a lot of places. It seems that >> only rtnetlink interface doesn't account for it. > > Stephen is not wrong, strictly speaking this is an ABI breaker. > > However, we have been applying NLA_TYPE_MASK in nla_type() and > thus in nla_parse(), nlmsg_find_attr(), etc. since the > introduction of the current kernel netlink API. > > static inline int nla_type(const struct nlattr *nla) > { > return nla->nla_type & NLA_TYPE_MASK; > } > > I have been attempting to get rid of that ugly attribute parsing > code in rtnetlink_rcv_msg() but it's hard to get rid of. I shall > give this another try. > >> 2) The following commit: >> >> commit 25c71c75ac87508528db053b818944f3650dd7a6 >> Author: stephen hemminger <shemminger@vyatta.com> >> Date: Tue Nov 13 07:53:05 2012 +0000 >> >> bridge: bridge port parameters over netlink >> >> introduced NLA_F_NESTED usage in rtnetlink for both setlink and >> getlink, so one could argue that was an ABI change. > > You are right, user space applications that do not apply > NLA_TYPE_MASK will no longer see the IFLA_PROTINFO attribute > with the above commit. > >> prove that without the change I am introducing, one can not use the >> above mentioned bridge API. Feel free to try it with iproute2 >> patches >> I sent earlier ([PATCH iproute2 0/2] Add support for bridge port >> link information). > > I am not against your patch but I would love to see the affected > code paths to no longer rely on the attribute array provided by > the rtnetlink doit function but instead call nla_parse() themselves > for the sake of proper validation. > Doing a quick check on all the callers for rtnl_register and their handlers the following do not use nla_parse: 1) dn_fib_rtm_newroute/delroute - Don't seem to care about attribute types. 2) dn_cache_getroute() - suspect use. relies on rta_buf populated by rtnetlink_rcv_msg 3) inet_rtm_newroute/delroute - rtm_to_fib_config() uses a custom loop with nla_type(), so safe. That's all that a quick look finds. Out of all of them, looks like on dn_cache_getroute() would be broken. -vlad -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/14/13 at 08:30pm, Vlad Yasevich wrote: > Doing a quick check on all the callers for rtnl_register and their > handlers the following do not use nla_parse: > 1) dn_fib_rtm_newroute/delroute - Don't seem to care about attribute > types. > 2) dn_cache_getroute() - suspect use. relies on rta_buf populated by > rtnetlink_rcv_msg > > 3) inet_rtm_newroute/delroute - rtm_to_fib_config() uses a custom loop > with nla_type(), so safe. > > That's all that a quick look finds. Out of all of them, looks like > on dn_cache_getroute() would be broken. So checking values in rta_max[] which lists the maximum attribute allowed for each message family range, all are limited to low values so the NLA_F_NESTED bit is guaranteed to have been unused up to now. The risk that remains is that we would start accepting an attribute which we previously didn't but we have that risk with every new attribute that is added. Acked-by: Thomas Graf <tgraf@suug.ch> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Thomas Graf <tgraf@suug.ch> Date: Fri, 15 Mar 2013 08:51:24 +0000 > On 03/14/13 at 08:30pm, Vlad Yasevich wrote: >> Doing a quick check on all the callers for rtnl_register and their >> handlers the following do not use nla_parse: >> 1) dn_fib_rtm_newroute/delroute - Don't seem to care about attribute >> types. >> 2) dn_cache_getroute() - suspect use. relies on rta_buf populated by >> rtnetlink_rcv_msg >> >> 3) inet_rtm_newroute/delroute - rtm_to_fib_config() uses a custom loop >> with nla_type(), so safe. >> >> That's all that a quick look finds. Out of all of them, looks like >> on dn_cache_getroute() would be broken. > > So checking values in rta_max[] which lists the maximum attribute > allowed for each message family range, all are limited to low > values so the NLA_F_NESTED bit is guaranteed to have been unused > up to now. > > The risk that remains is that we would start accepting an attribute > which we previously didn't but we have that risk with every new > attribute that is added. > > Acked-by: Thomas Graf <tgraf@suug.ch> Applied and queued up for -stable. And also agreed that abstracting the rtnetlink attribute parsing out properly is the way to go. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 1868625..dc5edf1 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -2538,7 +2538,7 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh) struct rtattr *attr = (void *)nlh + NLMSG_ALIGN(min_len); while (RTA_OK(attr, attrlen)) { - unsigned int flavor = attr->rta_type; + unsigned int flavor = attr->rta_type & NLA_TYPE_MASK; if (flavor) { if (flavor > rta_max[sz_idx]) return -EINVAL;
Range/validity checks on rta_type in rtnetlink_rcv_msg() do not account for flags that may be set. This causes the function to return -EINVAL when flags are set on the type (for example NLA_F_NESTED). Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> --- net/core/rtnetlink.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)