| 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