[PATCH net 0/3] rtnetlink: a couple of fixes in linkmsg validation

[PATCH net 0/3] rtnetlink: a couple of fixes in linkmsg validation

[Xin Long]

validate_linkmsg() was introduced to do linkmsg validation for existing
links. However, the new created links also need this linkmsg validation.

Move it to the correct place in Patch 1, and add more tb checks in
Patch 2 and 3.

Xin Long (3):
  rtnetlink: move validate_linkmsg into rtnl_create_link
  rtnetlink: move IFLA_GSO_ tb check to validate_linkmsg
  rtnetlink: add the missing IFLA_GRO_ tb check in validate_linkmsg

 net/core/rtnetlink.c | 70 +++++++++++++++++++++++++++-----------------
 1 file changed, 43 insertions(+), 27 deletions(-)

-- 
2.39.1

[PATCH net 1/3] rtnetlink: move validate_linkmsg into rtnl_create_link

[Xin Long]

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(-)

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],
-- 
2.39.1

Re: [PATCH net 1/3] rtnetlink: move validate_linkmsg into rtnl_create_link

[Simon Horman]

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>

Re: [PATCH net 1/3] rtnetlink: move validate_linkmsg into rtnl_create_link

[Jakub Kicinski]

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.
-- 
pw-bot: cr

Re: [PATCH net 1/3] rtnetlink: move validate_linkmsg into rtnl_create_link

[Xin Long]

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.

Re: [PATCH net 1/3] rtnetlink: move validate_linkmsg into rtnl_create_link

[Jakub Kicinski]

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.

[PATCH net 2/3] rtnetlink: move IFLA_GSO_ tb check to validate_linkmsg

[Xin Long]

These IFLA_GSO_* tb check should also be done for the new created link,
otherwise, they can be set to a huge value when creating links:

  # ip link add dummy1 gso_max_size 4294967295 type dummy
  # ip -d link show dummy1
    dummy addrgenmode eui64 ... gso_max_size 4294967295

Fixes: 46e6b992c250 ("rtnetlink: allow GSO maximums to be set on device creation")
Fixes: 9eefedd58ae1 ("net: add gso_ipv4_max_size and gro_ipv4_max_size per device")
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/core/rtnetlink.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index d1f88ba7e391..68a58d0a7b79 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2385,6 +2385,25 @@ static int validate_linkmsg(struct net_device *dev, struct nlattr *tb[],
 	    nla_len(tb[IFLA_BROADCAST]) < dev->addr_len)
 		return -EINVAL;
 
+	if (tb[IFLA_GSO_MAX_SIZE] &&
+	    nla_get_u32(tb[IFLA_GSO_MAX_SIZE]) > dev->tso_max_size) {
+		NL_SET_ERR_MSG(extack, "too big gso_max_size");
+		return -EINVAL;
+	}
+
+	if (tb[IFLA_GSO_MAX_SEGS] &&
+	    (nla_get_u32(tb[IFLA_GSO_MAX_SEGS]) > GSO_MAX_SEGS ||
+	     nla_get_u32(tb[IFLA_GSO_MAX_SEGS]) > dev->tso_max_segs)) {
+		NL_SET_ERR_MSG(extack, "too big gso_max_segs");
+		return -EINVAL;
+	}
+
+	if (tb[IFLA_GSO_IPV4_MAX_SIZE] &&
+	    nla_get_u32(tb[IFLA_GSO_IPV4_MAX_SIZE]) > dev->tso_max_size) {
+		NL_SET_ERR_MSG(extack, "too big gso_ipv4_max_size");
+		return -EINVAL;
+	}
+
 	if (tb[IFLA_AF_SPEC]) {
 		struct nlattr *af;
 		int rem, err;
@@ -2856,11 +2875,6 @@ static int do_setlink(const struct sk_buff *skb,
 	if (tb[IFLA_GSO_MAX_SIZE]) {
 		u32 max_size = nla_get_u32(tb[IFLA_GSO_MAX_SIZE]);
 
-		if (max_size > dev->tso_max_size) {
-			err = -EINVAL;
-			goto errout;
-		}
-
 		if (dev->gso_max_size ^ max_size) {
 			netif_set_gso_max_size(dev, max_size);
 			status |= DO_SETLINK_MODIFIED;
@@ -2870,11 +2884,6 @@ static int do_setlink(const struct sk_buff *skb,
 	if (tb[IFLA_GSO_MAX_SEGS]) {
 		u32 max_segs = nla_get_u32(tb[IFLA_GSO_MAX_SEGS]);
 
-		if (max_segs > GSO_MAX_SEGS || max_segs > dev->tso_max_segs) {
-			err = -EINVAL;
-			goto errout;
-		}
-
 		if (dev->gso_max_segs ^ max_segs) {
 			netif_set_gso_max_segs(dev, max_segs);
 			status |= DO_SETLINK_MODIFIED;
@@ -2893,11 +2902,6 @@ static int do_setlink(const struct sk_buff *skb,
 	if (tb[IFLA_GSO_IPV4_MAX_SIZE]) {
 		u32 max_size = nla_get_u32(tb[IFLA_GSO_IPV4_MAX_SIZE]);
 
-		if (max_size > dev->tso_max_size) {
-			err = -EINVAL;
-			goto errout;
-		}
-
 		if (dev->gso_ipv4_max_size ^ max_size) {
 			netif_set_gso_ipv4_max_size(dev, max_size);
 			status |= DO_SETLINK_MODIFIED;
-- 
2.39.1

Re: [PATCH net 2/3] rtnetlink: move IFLA_GSO_ tb check to validate_linkmsg

[Simon Horman]

On Thu, May 25, 2023 at 05:49:16PM -0400, Xin Long wrote:
> These IFLA_GSO_* tb check should also be done for the new created link,
> otherwise, they can be set to a huge value when creating links:
> 
>   # ip link add dummy1 gso_max_size 4294967295 type dummy
>   # ip -d link show dummy1
>     dummy addrgenmode eui64 ... gso_max_size 4294967295
> 
> Fixes: 46e6b992c250 ("rtnetlink: allow GSO maximums to be set on device creation")
> Fixes: 9eefedd58ae1 ("net: add gso_ipv4_max_size and gro_ipv4_max_size per device")
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

[PATCH net 3/3] rtnetlink: add the missing IFLA_GRO_ tb check in validate_linkmsg

[Xin Long]

This fixes the issue that dev gro_max_size and gso_ipv4_max_size
can be set to a huge value:

  # ip link add dummy1 type dummy
  # ip link set dummy1 gro_max_size 4294967295
  # ip -d link show dummy1 |grep gro_max_size
    dummy addrgenmode eui64 ... gro_max_size 4294967295

Fixes: 0fe79f28bfaf ("net: allow gro_max_size to exceed 65536")
Fixes: 9eefedd58ae1 ("net: add gso_ipv4_max_size and gro_ipv4_max_size per device")
Reported-by: Xiumei Mu <xmu@redhat.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/core/rtnetlink.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 68a58d0a7b79..4c7d84072bd4 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2398,12 +2398,24 @@ static int validate_linkmsg(struct net_device *dev, struct nlattr *tb[],
 		return -EINVAL;
 	}
 
+	if (tb[IFLA_GRO_MAX_SIZE] &&
+	    nla_get_u32(tb[IFLA_GRO_MAX_SIZE]) > GRO_MAX_SIZE) {
+		NL_SET_ERR_MSG(extack, "too big gro_max_size");
+		return -EINVAL;
+	}
+
 	if (tb[IFLA_GSO_IPV4_MAX_SIZE] &&
 	    nla_get_u32(tb[IFLA_GSO_IPV4_MAX_SIZE]) > dev->tso_max_size) {
 		NL_SET_ERR_MSG(extack, "too big gso_ipv4_max_size");
 		return -EINVAL;
 	}
 
+	if (tb[IFLA_GRO_IPV4_MAX_SIZE] &&
+	    nla_get_u32(tb[IFLA_GRO_IPV4_MAX_SIZE]) > GRO_MAX_SIZE) {
+		NL_SET_ERR_MSG(extack, "too big gro_ipv4_max_size");
+		return -EINVAL;
+	}
+
 	if (tb[IFLA_AF_SPEC]) {
 		struct nlattr *af;
 		int rem, err;
-- 
2.39.1

Re: [PATCH net 3/3] rtnetlink: add the missing IFLA_GRO_ tb check in validate_linkmsg

[Simon Horman]

On Thu, May 25, 2023 at 05:49:17PM -0400, Xin Long wrote:
> This fixes the issue that dev gro_max_size and gso_ipv4_max_size
> can be set to a huge value:
> 
>   # ip link add dummy1 type dummy
>   # ip link set dummy1 gro_max_size 4294967295
>   # ip -d link show dummy1 |grep gro_max_size
>     dummy addrgenmode eui64 ... gro_max_size 4294967295
> 
> Fixes: 0fe79f28bfaf ("net: allow gro_max_size to exceed 65536")
> Fixes: 9eefedd58ae1 ("net: add gso_ipv4_max_size and gro_ipv4_max_size per device")
> Reported-by: Xiumei Mu <xmu@redhat.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>