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:44:52 +0300 From: Nikolay Aleksandrov <razor@...ckwall.org> To: David Lamparter <equinox@...c24.net> 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 04/07/2022 13:38, David Lamparter wrote: > 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. > They really should be using policies specific to their actions with only the allowed attributes. Re-using this policy is ok only if those match, otherwise it's a bug IMO. > But I don't feel particularly about this - shall I remove it? (just > confirming with the rationale above) > I don't have a preference either, IMO if anyone re-uses this policy without making sure the same attributes and types are needed is adding buggy code. Actually the thing that I like about keeping the loop is the more specific error message, let's see what others think. >>> + 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