David Ahern
2020-Jan-14 16:49 UTC
[Bridge] [PATCH net-next 3/8] net: bridge: vlan: add rtm definitions and dump support
On 1/14/20 9:45 AM, Nikolay Aleksandrov wrote:> On 14/01/2020 18:36, Nikolay Aleksandrov wrote: >> On 14/01/2020 17:34, David Ahern wrote: >>> On 1/14/20 6:55 AM, Jakub Kicinski wrote: >>>> On Mon, 13 Jan 2020 17:52:28 +0200, Nikolay Aleksandrov wrote: >>>>> +static int br_vlan_rtm_dump(struct sk_buff *skb, struct netlink_callback *cb) >>>>> +{ >>>>> + int idx = 0, err = 0, s_idx = cb->args[0]; >>>>> + struct net *net = sock_net(skb->sk); >>>>> + struct br_vlan_msg *bvm; >>>>> + struct net_device *dev; >>>>> + >>>>> + if (cb->nlh->nlmsg_len < nlmsg_msg_size(sizeof(*bvm))) { >>>> >>>> I wonder if it'd be useful to make this a strict != check? At least >>>> when strict validation is on? Perhaps we'll one day want to extend >>>> the request? >>>> >>> >>> +1. All new code should be using the strict checks. >>> >> >> IIRC, I did it to be able to add filter attributes later, but it should just use nlmsg_parse() >> instead and all will be taken care of. >> I'll respin v2 with that change. >> >> Thanks, >> Nik >> > > Actually nlmsg_parse() uses the same "<" check for the size before parsing. :) > If I change to it and with no attributes to parse would be essentially equal to the > current situation, but if I make it strict "!=" then we won't be able to add > filter attributes later as we won't be backwards compatible. I'll continue looking > into it, but IMO we should leave it as it is in order to be able to add the filtering later. > > Thoughts ? > > > >If the header is > sizeof(*bvm) I expect this part of __nla_validate_parse() to kick in: if (unlikely(rem > 0)) { pr_warn_ratelimited("netlink: %d bytes leftover after parsing attributes in process `%s'.\n", rem, current->comm); NL_SET_ERR_MSG(extack, "bytes leftover after parsing attributes"); if (validate & NL_VALIDATE_TRAILING) return -EINVAL; }
Nikolay Aleksandrov
2020-Jan-14 16:50 UTC
[Bridge] [PATCH net-next 3/8] net: bridge: vlan: add rtm definitions and dump support
On 14/01/2020 18:49, David Ahern wrote:> On 1/14/20 9:45 AM, Nikolay Aleksandrov wrote: >> On 14/01/2020 18:36, Nikolay Aleksandrov wrote: >>> On 14/01/2020 17:34, David Ahern wrote: >>>> On 1/14/20 6:55 AM, Jakub Kicinski wrote: >>>>> On Mon, 13 Jan 2020 17:52:28 +0200, Nikolay Aleksandrov wrote: >>>>>> +static int br_vlan_rtm_dump(struct sk_buff *skb, struct netlink_callback *cb) >>>>>> +{ >>>>>> + int idx = 0, err = 0, s_idx = cb->args[0]; >>>>>> + struct net *net = sock_net(skb->sk); >>>>>> + struct br_vlan_msg *bvm; >>>>>> + struct net_device *dev; >>>>>> + >>>>>> + if (cb->nlh->nlmsg_len < nlmsg_msg_size(sizeof(*bvm))) { >>>>> >>>>> I wonder if it'd be useful to make this a strict != check? At least >>>>> when strict validation is on? Perhaps we'll one day want to extend >>>>> the request? >>>>> >>>> >>>> +1. All new code should be using the strict checks. >>>> >>> >>> IIRC, I did it to be able to add filter attributes later, but it should just use nlmsg_parse() >>> instead and all will be taken care of. >>> I'll respin v2 with that change. >>> >>> Thanks, >>> Nik >>> >> >> Actually nlmsg_parse() uses the same "<" check for the size before parsing. :) >> If I change to it and with no attributes to parse would be essentially equal to the >> current situation, but if I make it strict "!=" then we won't be able to add >> filter attributes later as we won't be backwards compatible. I'll continue looking >> into it, but IMO we should leave it as it is in order to be able to add the filtering later. >> >> Thoughts ? >> >> >> >> > > If the header is > sizeof(*bvm) I expect this part of > __nla_validate_parse() to kick in: > > if (unlikely(rem > 0)) { > pr_warn_ratelimited("netlink: %d bytes leftover after > parsing attributes in process `%s'.\n", > rem, current->comm); > NL_SET_ERR_MSG(extack, "bytes leftover after parsing > attributes"); > if (validate & NL_VALIDATE_TRAILING) > return -EINVAL; > } >Ah fair enough, so nlmsg_parse() would be better even without attrs.