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 12:38:54 +0200 From: David Lamparter <equinox@...c24.net> To: Nikolay Aleksandrov <razor@...ckwall.org> Cc: netdev@...r.kernel.org, "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 Mon, Jul 04, 2022 at 01:22:36PM +0300, Nikolay Aleksandrov wrote: > On 04/07/2022 12:58, David Lamparter wrote: > > +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 Will remove it. > > + 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 Indeed it does. Moving it down. [...] > > + /* 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."). I left it in with the comment above: > > + /* 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. > > + */ ... to try and avoid silently starting to accept more attributes if/when future patches add other netlink operations reusing the same policy but with adding new attributes. But I don't feel particularly about this - shall I remove it? (just confirming with the rationale above) > > + 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 Ah. Wasn't aware of that coding style aspect. Fixing. Thanks for the review! -David/equi
Powered by blists - more mailing lists