netdev - Re: [PATCH net 1/3] rtnetlink: move validate_linkmsg into rtnl_create

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 linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CADvbK_eoJUrDFrW_Kons7RnU5qivdA9ezULcMacB-H+QcNNNCQ@mail.gmail.com>
Date: Sat, 27 May 2023 16:36:15 -0400
From: Xin Long <lucien.xin@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: network dev <netdev@...r.kernel.org>, davem@...emloft.net, 
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, 
	Thomas Graf <tgraf@...radead.org>, Alexander Duyck <alexanderduyck@...com>
Subject: Re: [PATCH net 1/3] rtnetlink: move validate_linkmsg into rtnl_create_link

On Fri, May 26, 2023 at 11:49 PM Jakub Kicinski <kuba@...nel.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.