| Message ID | 7fde1eac7583cc93bc5b1cb3b386c522b32a94c9.1685051273.git.lucien.xin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | rtnetlink: a couple of fixes in linkmsg validation | expand |
On Thu, May 25, 2023 at 05:49:15PM -0400, Xin Long wrote: > In commit 644c7eebbfd5 ("rtnetlink: validate attributes in do_setlink()"), > it moved validate_linkmsg() from rtnl_setlink() to do_setlink(). However, > as validate_linkmsg() is also called in __rtnl_newlink(), it caused > validate_linkmsg() being called twice when running 'ip link set'. > > The validate_linkmsg() was introduced by commit 1840bb13c22f5b ("[RTNL]: > Validate hardware and broadcast address attribute for RTM_NEWLINK") for > existing links. After adding it in do_setlink(), there's no need to call > it in __rtnl_newlink(). > > Instead of deleting it from __rtnl_newlink(), this patch moves it to > rtnl_create_link() to fix the missing validation for the new created > links. > > Fixes: 644c7eebbfd5 ("rtnetlink: validate attributes in do_setlink()") > Signed-off-by: Xin Long <lucien.xin@gmail.com> Reviewed-by: Simon Horman <simon.horman@corigine.com>
On Thu, 25 May 2023 17:49:15 -0400 Xin Long wrote: > In commit 644c7eebbfd5 ("rtnetlink: validate attributes in do_setlink()"), > it moved validate_linkmsg() from rtnl_setlink() to do_setlink(). However, > as validate_linkmsg() is also called in __rtnl_newlink(), it caused > validate_linkmsg() being called twice when running 'ip link set'. > > The validate_linkmsg() was introduced by commit 1840bb13c22f5b ("[RTNL]: > Validate hardware and broadcast address attribute for RTM_NEWLINK") for > existing links. After adding it in do_setlink(), there's no need to call > it in __rtnl_newlink(). > > Instead of deleting it from __rtnl_newlink(), this patch moves it to > rtnl_create_link() to fix the missing validation for the new created > links. > > Fixes: 644c7eebbfd5 ("rtnetlink: validate attributes in do_setlink()") I don't see any bug in here, is there one? Or you're just trying to avoid calling validation twice? I think it's better to validate twice than validate after some changes have already been applied by __rtnl_newlink()... If we really care about the double validation we should pull the validation out of do_setlink(), IMHO.
On Fri, May 26, 2023 at 11:49 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 25 May 2023 17:49:15 -0400 Xin Long wrote: > > In commit 644c7eebbfd5 ("rtnetlink: validate attributes in do_setlink()"), > > it moved validate_linkmsg() from rtnl_setlink() to do_setlink(). However, > > as validate_linkmsg() is also called in __rtnl_newlink(), it caused > > validate_linkmsg() being called twice when running 'ip link set'. > > > > The validate_linkmsg() was introduced by commit 1840bb13c22f5b ("[RTNL]: > > Validate hardware and broadcast address attribute for RTM_NEWLINK") for > > existing links. After adding it in do_setlink(), there's no need to call > > it in __rtnl_newlink(). > > > > Instead of deleting it from __rtnl_newlink(), this patch moves it to > > rtnl_create_link() to fix the missing validation for the new created > > links. > > > > Fixes: 644c7eebbfd5 ("rtnetlink: validate attributes in do_setlink()") > > I don't see any bug in here, is there one? Or you're just trying > to avoid calling validation twice? I think it's better to validate > twice than validate after some changes have already been applied > by __rtnl_newlink()... If we really care about the double validation > we should pull the validation out of do_setlink(), IMHO. Other than avoiding calling validation twice, adding validate_linkmsg() in rtnl_create_link() also fixes the missing validation for newly created links, it includes tb[IFLA_ADDRESS/IFLA_BROADCAST] checks in validate_linkmsg(): For example, because of validate_linkmsg(), these command will fail # ip link add dummy0 type dummy # ip link add link dummy0 name mac0 type macsec # ip link set mac0 address 01 RTNETLINK answers: Invalid argument (I removed the IFLA_ADDRESS validation in iproute2 itself) However, the below will work: # ip link add dummy0 type dummy # ip link add link dummy0 name mac0 address 01 type macsec As for the calling twice thing, validating before any changes happen makes sense. Based on the change in this patch, I will pull the validation out of do_setlink() to these 3 places: @@ -3600,7 +3605,9 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh, return -EEXIST; if (nlh->nlmsg_flags & NLM_F_REPLACE) return -EOPNOTSUPP; - + err = validate_linkmsg(dev, tb, extack); + if (err < 0) + return err; @@ -3377,6 +3383,9 @@ static int rtnl_group_changelink(const struct sk_buff *skb, for_each_netdev_safe(net, dev, aux) { if (dev->group == group) { + err = validate_linkmsg(dev, tb, extack); + if (err < 0) + return err; err = do_setlink(skb, dev, ifm, extack, tb, 0); @@ -3140,6 +3136,10 @@ static int rtnl_setlink(struct sk_buff *skb, struct nlmsghdr *nlh, goto errout; } + err = validate_linkmsg(dev, tb, extack); + if (err < 0) + goto errout; + err = do_setlink(skb, dev, ifm, extack, tb, 0); and yes, one more place calls validate_linkmsg (comparing to the old code with the one in rtnl_create_link), unless someone thinks it's not worth it. Thanks.
On Sat, 27 May 2023 16:36:15 -0400 Xin Long wrote: > Other than avoiding calling validation twice, adding validate_linkmsg() in > rtnl_create_link() also fixes the missing validation for newly created links, > it includes tb[IFLA_ADDRESS/IFLA_BROADCAST] checks in validate_linkmsg(): Ah, I see. Since this is a fix I'd err on the side of keeping the change small and obvious and limit it to adding the call in validate_linkmsg() without worrying about validating multiple times. Then follow up with the refactoring to net-next. I guess it could be acceptable to take the whole thing into net, if you prefer but at least the commit message would need clarification. > As for the calling twice thing, validating before any changes happen > makes sense. > Based on the change in this patch, I will pull the validation out of > do_setlink() > to these 3 places: > > @@ -3600,7 +3605,9 @@ static int __rtnl_newlink(struct sk_buff *skb, > struct nlmsghdr *nlh, > return -EEXIST; > if (nlh->nlmsg_flags & NLM_F_REPLACE) > return -EOPNOTSUPP; > - > + err = validate_linkmsg(dev, tb, extack); > + if (err < 0) > + return err; > > @@ -3377,6 +3383,9 @@ static int rtnl_group_changelink(const struct > sk_buff *skb, > > for_each_netdev_safe(net, dev, aux) { > if (dev->group == group) { > + err = validate_linkmsg(dev, tb, extack); > + if (err < 0) > + return err; > err = do_setlink(skb, dev, ifm, extack, tb, 0); > > @@ -3140,6 +3136,10 @@ static int rtnl_setlink(struct sk_buff *skb, > struct nlmsghdr *nlh, > goto errout; > } > > + err = validate_linkmsg(dev, tb, extack); > + if (err < 0) > + goto errout; > + > err = do_setlink(skb, dev, ifm, extack, tb, 0); > > > and yes, one more place calls validate_linkmsg (comparing to the old code > with the one in rtnl_create_link), unless someone thinks it's not worth it. Yup, LGTM. Renaming do_setlink() to __do_setlink() and adding a wrapper called do_setlink() which does the validation and calls __do_setlink() - seems like another option to explore. Whatever you reckon ends up looking neatest. As long as the validation ends up moving towards the entry point not deeper in - any approach is fine.
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 653901a1bf75..d1f88ba7e391 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -2377,15 +2377,13 @@ static int rtnl_set_vf_rate(struct net_device *dev, int vf, int min_tx_rate, static int validate_linkmsg(struct net_device *dev, struct nlattr *tb[], struct netlink_ext_ack *extack) { - if (dev) { - if (tb[IFLA_ADDRESS] && - nla_len(tb[IFLA_ADDRESS]) < dev->addr_len) - return -EINVAL; + if (tb[IFLA_ADDRESS] && + nla_len(tb[IFLA_ADDRESS]) < dev->addr_len) + return -EINVAL; - if (tb[IFLA_BROADCAST] && - nla_len(tb[IFLA_BROADCAST]) < dev->addr_len) - return -EINVAL; - } + if (tb[IFLA_BROADCAST] && + nla_len(tb[IFLA_BROADCAST]) < dev->addr_len) + return -EINVAL; if (tb[IFLA_AF_SPEC]) { struct nlattr *af; @@ -3285,6 +3283,7 @@ struct net_device *rtnl_create_link(struct net *net, const char *ifname, struct net_device *dev; unsigned int num_tx_queues = 1; unsigned int num_rx_queues = 1; + int err; if (tb[IFLA_NUM_TX_QUEUES]) num_tx_queues = nla_get_u32(tb[IFLA_NUM_TX_QUEUES]); @@ -3320,13 +3319,18 @@ struct net_device *rtnl_create_link(struct net *net, const char *ifname, if (!dev) return ERR_PTR(-ENOMEM); + err = validate_linkmsg(dev, tb, extack); + if (err < 0) { + free_netdev(dev); + return ERR_PTR(err); + } + dev_net_set(dev, net); dev->rtnl_link_ops = ops; dev->rtnl_link_state = RTNL_LINK_INITIALIZING; if (tb[IFLA_MTU]) { u32 mtu = nla_get_u32(tb[IFLA_MTU]); - int err; err = dev_validate_mtu(dev, mtu, extack); if (err) { @@ -3534,10 +3538,6 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh, m_ops = master_dev->rtnl_link_ops; } - err = validate_linkmsg(dev, tb, extack); - if (err < 0) - return err; - if (tb[IFLA_LINKINFO]) { err = nla_parse_nested_deprecated(linkinfo, IFLA_INFO_MAX, tb[IFLA_LINKINFO],
In commit 644c7eebbfd5 ("rtnetlink: validate attributes in do_setlink()"), it moved validate_linkmsg() from rtnl_setlink() to do_setlink(). However, as validate_linkmsg() is also called in __rtnl_newlink(), it caused validate_linkmsg() being called twice when running 'ip link set'. The validate_linkmsg() was introduced by commit 1840bb13c22f5b ("[RTNL]: Validate hardware and broadcast address attribute for RTM_NEWLINK") for existing links. After adding it in do_setlink(), there's no need to call it in __rtnl_newlink(). Instead of deleting it from __rtnl_newlink(), this patch moves it to rtnl_create_link() to fix the missing validation for the new created links. Fixes: 644c7eebbfd5 ("rtnetlink: validate attributes in do_setlink()") Signed-off-by: Xin Long <lucien.xin@gmail.com> --- net/core/rtnetlink.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)