Re: [PATCH v2 net-next 09/23] rtnetlink: Update rtnl_dump_ifinfo for strict data checking
On Sun, Oct 07, 2018 at 08:16:30PM -0700, David Ahern wrote:
> From: David Ahern
>
> Update rtnl_dump_ifinfo for strict data checking. If the flag is set,
> the dump request is expected to have an ifinfomsg struct as the header
> potentially followed by one or more attributes. Any data passed in the
> header or as an attribute is taken as a request to influence the data
> returned. Only values supported by the dump handler are allowed to be
> non-0 or set in the request. At the moment only the IFA_TARGET_NETNSID,
> IFLA_EXT_MASK, IFLA_MASTER, and IFLA_LINKINFO attributes are supported.
>
> Existing code does not fail the dump if nlmsg_parse fails. That behavior
> is kept for non-strict checking.
>
> Signed-off-by: David Ahern
Acked-by: Christian Brauner
> ---
> net/core/rtnetlink.c | 113
> +--
> 1 file changed, 83 insertions(+), 30 deletions(-)
>
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 4486e8b7d9d0..12fd52105005 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -1878,8 +1878,52 @@ struct net *rtnl_get_net_ns_capable(struct sock *sk,
> int netnsid)
> }
> EXPORT_SYMBOL_GPL(rtnl_get_net_ns_capable);
>
> +static int rtnl_valid_dump_ifinfo_req(const struct nlmsghdr *nlh,
> + bool strict_check, struct nlattr **tb,
> + struct netlink_ext_ack *extack)
> +{
> + int hdrlen;
> +
> + if (strict_check) {
> + struct ifinfomsg *ifm;
> +
> + if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*ifm))) {
> + NL_SET_ERR_MSG(extack, "Invalid header for link dump");
> + return -EINVAL;
> + }
> +
> + ifm = nlmsg_data(nlh);
> + if (ifm->__ifi_pad || ifm->ifi_type || ifm->ifi_flags ||
> + ifm->ifi_change) {
> + NL_SET_ERR_MSG(extack, "Invalid values in header for
> link dump request");
> + return -EINVAL;
> + }
> + if (ifm->ifi_index) {
> + NL_SET_ERR_MSG(extack, "Filter by device index not
> supported for link dumps");
> + return -EINVAL;
> + }
> +
> + return nlmsg_parse_strict(nlh, sizeof(*ifm), tb, IFLA_MAX,
> + ifla_policy, extack);
> + }
> +
> + /* A hack to preserve kernel<->userspace interface.
> + * The correct header is ifinfomsg. It is consistent with rtnl_getlink.
> + * However, before Linux v3.9 the code here assumed rtgenmsg and that's
> + * what iproute2 < v3.9.0 used.
> + * We can detect the old iproute2. Even including the IFLA_EXT_MASK
> + * attribute, its netlink message is shorter than struct ifinfomsg.
> + */
> + hdrlen = nlmsg_len(nlh) < sizeof(struct ifinfomsg) ?
> + sizeof(struct rtgenmsg) : sizeof(struct ifinfomsg);
> +
> + return nlmsg_parse(nlh, hdrlen, tb, IFLA_MAX, ifla_policy, extack);
> +}
> +
> static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb)
> {
> + struct netlink_ext_ack *extack = cb->extack;
> + const struct nlmsghdr *nlh = cb->nlh;
> struct net *net = sock_net(skb->sk);
> struct net *tgt_net = net;
> int h, s_h;
> @@ -1892,44 +1936,54 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb,
> struct netlink_callback *cb)
> unsigned int flags = NLM_F_MULTI;
> int master_idx = 0;
> int netnsid = -1;
> - int err;
> - int hdrlen;
> + int err, i;
>
> s_h = cb->args[0];
> s_idx = cb->args[1];
>
> - /* A hack to preserve kernel<->userspace interface.
> - * The correct header is ifinfomsg. It is consistent with rtnl_getlink.
> - * However, before Linux v3.9 the code here assumed rtgenmsg and that's
> - * what iproute2 < v3.9.0 used.
> - * We can detect the old iproute2. Even including the IFLA_EXT_MASK
> - * attribute, its netlink message is shorter than struct ifinfomsg.
> - */
> - hdrlen = nlmsg_len(cb->nlh) < sizeof(struct ifinfomsg) ?
> - sizeof(struct rtgenmsg) : sizeof(struct ifinfomsg);
> + err = rtnl_valid_dump_ifinfo_req(nlh, cb->strict_check, tb, extack);
> + if (err < 0) {
> + if (cb->strict_check)
> + return err;
> +
> + goto walk_entries;
> + }
> +
> + for (i = 0; i <= IFLA_MAX; ++i) {
> + if (!tb[i])
> + continue;
>
> - if (nlmsg_parse(cb->nlh, hdrlen, tb, IFLA_MAX,
> - ifla_policy, cb->extack) >= 0) {
> - if (tb[IFLA_TARGET_NETNSID]) {
> - netnsid = nla_get_s32(tb[IFLA_TARGET_NETNSID]);
> + /* new attributes should only be added with strict checking */
> + switch (i) {
> + case IFLA_TARGET_NETNSID:
> + netnsid =
[PATCH v2 net-next 09/23] rtnetlink: Update rtnl_dump_ifinfo for strict data checking
From: David Ahern
Update rtnl_dump_ifinfo for strict data checking. If the flag is set,
the dump request is expected to have an ifinfomsg struct as the header
potentially followed by one or more attributes. Any data passed in the
header or as an attribute is taken as a request to influence the data
returned. Only values supported by the dump handler are allowed to be
non-0 or set in the request. At the moment only the IFA_TARGET_NETNSID,
IFLA_EXT_MASK, IFLA_MASTER, and IFLA_LINKINFO attributes are supported.
Existing code does not fail the dump if nlmsg_parse fails. That behavior
is kept for non-strict checking.
Signed-off-by: David Ahern
---
net/core/rtnetlink.c | 113 +--
1 file changed, 83 insertions(+), 30 deletions(-)
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 4486e8b7d9d0..12fd52105005 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1878,8 +1878,52 @@ struct net *rtnl_get_net_ns_capable(struct sock *sk, int
netnsid)
}
EXPORT_SYMBOL_GPL(rtnl_get_net_ns_capable);
+static int rtnl_valid_dump_ifinfo_req(const struct nlmsghdr *nlh,
+ bool strict_check, struct nlattr **tb,
+ struct netlink_ext_ack *extack)
+{
+ int hdrlen;
+
+ if (strict_check) {
+ struct ifinfomsg *ifm;
+
+ if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*ifm))) {
+ NL_SET_ERR_MSG(extack, "Invalid header for link dump");
+ return -EINVAL;
+ }
+
+ ifm = nlmsg_data(nlh);
+ if (ifm->__ifi_pad || ifm->ifi_type || ifm->ifi_flags ||
+ ifm->ifi_change) {
+ NL_SET_ERR_MSG(extack, "Invalid values in header for
link dump request");
+ return -EINVAL;
+ }
+ if (ifm->ifi_index) {
+ NL_SET_ERR_MSG(extack, "Filter by device index not
supported for link dumps");
+ return -EINVAL;
+ }
+
+ return nlmsg_parse_strict(nlh, sizeof(*ifm), tb, IFLA_MAX,
+ ifla_policy, extack);
+ }
+
+ /* A hack to preserve kernel<->userspace interface.
+* The correct header is ifinfomsg. It is consistent with rtnl_getlink.
+* However, before Linux v3.9 the code here assumed rtgenmsg and that's
+* what iproute2 < v3.9.0 used.
+* We can detect the old iproute2. Even including the IFLA_EXT_MASK
+* attribute, its netlink message is shorter than struct ifinfomsg.
+*/
+ hdrlen = nlmsg_len(nlh) < sizeof(struct ifinfomsg) ?
+sizeof(struct rtgenmsg) : sizeof(struct ifinfomsg);
+
+ return nlmsg_parse(nlh, hdrlen, tb, IFLA_MAX, ifla_policy, extack);
+}
+
static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb)
{
+ struct netlink_ext_ack *extack = cb->extack;
+ const struct nlmsghdr *nlh = cb->nlh;
struct net *net = sock_net(skb->sk);
struct net *tgt_net = net;
int h, s_h;
@@ -1892,44 +1936,54 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb, struct
netlink_callback *cb)
unsigned int flags = NLM_F_MULTI;
int master_idx = 0;
int netnsid = -1;
- int err;
- int hdrlen;
+ int err, i;
s_h = cb->args[0];
s_idx = cb->args[1];
- /* A hack to preserve kernel<->userspace interface.
-* The correct header is ifinfomsg. It is consistent with rtnl_getlink.
-* However, before Linux v3.9 the code here assumed rtgenmsg and that's
-* what iproute2 < v3.9.0 used.
-* We can detect the old iproute2. Even including the IFLA_EXT_MASK
-* attribute, its netlink message is shorter than struct ifinfomsg.
-*/
- hdrlen = nlmsg_len(cb->nlh) < sizeof(struct ifinfomsg) ?
-sizeof(struct rtgenmsg) : sizeof(struct ifinfomsg);
+ err = rtnl_valid_dump_ifinfo_req(nlh, cb->strict_check, tb, extack);
+ if (err < 0) {
+ if (cb->strict_check)
+ return err;
+
+ goto walk_entries;
+ }
+
+ for (i = 0; i <= IFLA_MAX; ++i) {
+ if (!tb[i])
+ continue;
- if (nlmsg_parse(cb->nlh, hdrlen, tb, IFLA_MAX,
- ifla_policy, cb->extack) >= 0) {
- if (tb[IFLA_TARGET_NETNSID]) {
- netnsid = nla_get_s32(tb[IFLA_TARGET_NETNSID]);
+ /* new attributes should only be added with strict checking */
+ switch (i) {
+ case IFLA_TARGET_NETNSID:
+ netnsid = nla_get_s32(tb[i]);
tgt_net = rtnl_get_net_ns_capable(skb->sk, netnsid);
- if (IS_ERR(tgt_net))
+ if (IS_ERR(tgt_net)) {
+
