lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 4 Jul 2022 13:22:36 +0300 From: Nikolay Aleksandrov <razor@...ckwall.org> To: David Lamparter <equinox@...c24.net>, netdev@...r.kernel.org Cc: "David S. Miller" <davem@...emloft.net>, David Ahern <dsahern@...nel.org>, Eric Dumazet <edumazet@...gle.com> Subject: Re: [PATCH net-next v2] net: ip6mr: add RTM_GETROUTE netlink op On 04/07/2022 12:58, David Lamparter wrote: > The IPv6 multicast routing code previously implemented only the dump > variant of RTM_GETROUTE. Implement single MFC item retrieval by copying > and adapting the respective IPv4 code. > > Tested against FRRouting's IPv6 PIM stack. > > Signed-off-by: David Lamparter <equinox@...c24.net> > Cc: David Ahern <dsahern@...nel.org> > --- > > v2: changeover to strict netlink attribute parsing. Doing so actually > exposed a bunch of other issues, first and foremost that rtm_ipv6_policy > does not have RTA_SRC or RTA_DST. This made reusing that policy rather > pointless so I changed it to use a separate rtm_ipv6_mr_policy. > > Thanks again dsahern@ for the feedback on the previous version! > > --- > net/ipv6/ip6mr.c | 128 ++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 127 insertions(+), 1 deletion(-) > > diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c > index ec6e1509fc7c..95dc366a2d9b 100644 > --- a/net/ipv6/ip6mr.c > +++ b/net/ipv6/ip6mr.c > @@ -95,6 +95,8 @@ static int ip6mr_cache_report(const struct mr_table *mrt, struct sk_buff *pkt, > static void mr6_netlink_event(struct mr_table *mrt, struct mfc6_cache *mfc, > int cmd); > static void mrt6msg_netlink_event(const struct mr_table *mrt, struct sk_buff *pkt); > +static int ip6mr_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh, > + struct netlink_ext_ack *extack); > static int ip6mr_rtm_dumproute(struct sk_buff *skb, > struct netlink_callback *cb); > static void mroute_clean_tables(struct mr_table *mrt, int flags); > @@ -1390,7 +1392,7 @@ int __init ip6_mr_init(void) > } > #endif > err = rtnl_register_module(THIS_MODULE, RTNL_FAMILY_IP6MR, RTM_GETROUTE, > - NULL, ip6mr_rtm_dumproute, 0); > + ip6mr_rtm_getroute, ip6mr_rtm_dumproute, 0); > if (err == 0) > return 0; > > @@ -2510,6 +2512,130 @@ static void mrt6msg_netlink_event(const struct mr_table *mrt, struct sk_buff *pk > rtnl_set_sk_err(net, RTNLGRP_IPV6_MROUTE_R, -ENOBUFS); > } > > +const struct nla_policy rtm_ipv6_mr_policy[RTA_MAX + 1] = { > + [RTA_UNSPEC] = { .strict_start_type = RTA_UNSPEC }, I don't think you need to add RTA_UNSPEC, nlmsg_parse() would reject it due to NL_VALIDATE_STRICT > + [RTA_SRC] = NLA_POLICY_EXACT_LEN(sizeof(struct in6_addr)), > + [RTA_DST] = NLA_POLICY_EXACT_LEN(sizeof(struct in6_addr)), > + [RTA_TABLE] = { .type = NLA_U32 }, > +}; > + > +static int ip6mr_rtm_valid_getroute_req(struct sk_buff *skb, > + const struct nlmsghdr *nlh, > + struct nlattr **tb, > + struct netlink_ext_ack *extack) > +{ > + struct rtmsg *rtm; > + int i, err; > + > + if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*rtm))) { > + NL_SET_ERR_MSG(extack, "ipv6: Invalid header for multicast route get request"); > + return -EINVAL; > + } I think you can drop this check if you... > + > + rtm = nlmsg_data(nlh); > + if ((rtm->rtm_src_len && rtm->rtm_src_len != 128) || > + (rtm->rtm_dst_len && rtm->rtm_dst_len != 128) || > + rtm->rtm_tos || rtm->rtm_table || rtm->rtm_protocol || > + rtm->rtm_scope || rtm->rtm_type || rtm->rtm_flags) { > + NL_SET_ERR_MSG(extack, > + "ipv6: Invalid values in header for multicast route get request"); > + return -EINVAL; > + } ...move these after nlmsg_parse() because it already does the hdrlen check for you > + > + err = nlmsg_parse(nlh, sizeof(*rtm), tb, RTA_MAX, rtm_ipv6_mr_policy, > + extack); > + if (err) > + return err; > + > + if ((tb[RTA_SRC] && !rtm->rtm_src_len) || > + (tb[RTA_DST] && !rtm->rtm_dst_len)) { > + NL_SET_ERR_MSG(extack, "ipv6: rtm_src_len and rtm_dst_len must be 128 for IPv6"); > + return -EINVAL; > + } > + > + /* rtm_ipv6_mr_policy does not list other attributes right now, but > + * future changes may reuse rtm_ipv6_mr_policy with adding further > + * attrs. Enforce the subset. > + */ > + for (i = 0; i <= RTA_MAX; i++) { > + if (!tb[i]) > + continue; > + > + switch (i) { > + case RTA_SRC: > + case RTA_DST: > + case RTA_TABLE: > + break; > + default: > + NL_SET_ERR_MSG_ATTR(extack, tb[i], > + "ipv6: Unsupported attribute in multicast route get request"); > + return -EINVAL; > + } > + } I think you can skip this loop as well, nlmsg_parse() shouldn't allow attributes that don't have policy defined when policy is provided (i.e. they should show up as NLA_UNSPEC and you should get "Error: Unknown attribute type."). > + > + return 0; > +} > + > +static int ip6mr_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh, > + struct netlink_ext_ack *extack) > +{ > + struct net *net = sock_net(in_skb->sk); > + struct nlattr *tb[RTA_MAX + 1]; > + struct sk_buff *skb = NULL; > + struct mfc6_cache *cache; > + struct mr_table *mrt; > + struct in6_addr src = {}, grp = {}; reverse xmas tree order > + u32 tableid; > + int err; > + > + err = ip6mr_rtm_valid_getroute_req(in_skb, nlh, tb, extack); > + if (err < 0) > + goto errout; > + > + if (tb[RTA_SRC]) > + src = nla_get_in6_addr(tb[RTA_SRC]); > + if (tb[RTA_DST]) > + grp = nla_get_in6_addr(tb[RTA_DST]); > + tableid = tb[RTA_TABLE] ? nla_get_u32(tb[RTA_TABLE]) : 0; > + > + mrt = ip6mr_get_table(net, tableid ? tableid : RT_TABLE_DEFAULT); > + if (!mrt) { > + NL_SET_ERR_MSG_MOD(extack, "ipv6 MR table does not exist"); > + err = -ENOENT; > + goto errout_free; > + } > + > + /* entries are added/deleted only under RTNL */ > + rcu_read_lock(); > + cache = ip6mr_cache_find(mrt, &src, &grp); > + rcu_read_unlock(); > + if (!cache) { > + NL_SET_ERR_MSG_MOD(extack, "ipv6 MR cache entry not found"); > + err = -ENOENT; > + goto errout_free; > + } > + > + skb = nlmsg_new(mr6_msgsize(false, mrt->maxvif), GFP_KERNEL); > + if (!skb) { > + err = -ENOBUFS; > + goto errout_free; > + } > + > + err = ip6mr_fill_mroute(mrt, skb, NETLINK_CB(in_skb).portid, > + nlh->nlmsg_seq, cache, RTM_NEWROUTE, 0); > + if (err < 0) > + goto errout_free; > + > + err = rtnl_unicast(skb, net, NETLINK_CB(in_skb).portid); > + > +errout: > + return err; > + > +errout_free: > + kfree_skb(skb); > + goto errout; > +} > + > static int ip6mr_rtm_dumproute(struct sk_buff *skb, struct netlink_callback *cb) > { > const struct nlmsghdr *nlh = cb->nlh;
Powered by blists - more mailing lists