[PATCH] rtnetlink: fix data overflow in rtnl_calcit()

[PATCH] rtnetlink: fix data overflow in rtnl_calcit()

[zhudi]

"ip addr show" command execute error when we have a physical
network card with number of VFs larger than 247.

The return value of if_nlmsg_size() in rtnl_calcit() will exceed
range of u16 data type when any network cards has a larger number of
VFs. rtnl_vfinfo_size() will significant increase needed dump size when
the value of num_vfs is larger.

Eventually we get a wrong value of min_ifinfo_dump_size because of overflow
which decides the memory size needed by netlink dump and netlink_dump()
will return -EMSGSIZE because of not enough memory was allocated.

So fix it by promoting  min_dump_alloc data type to u32 to
avoid data overflow and it's also align with the data type of
struct netlink_callback{}.min_dump_alloc which is assigned by
return value of rtnl_calcit()

Signed-off-by: zhudi <zhudi21@huawei.com>
---
 include/linux/netlink.h | 2 +-
 net/core/rtnetlink.c    | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index e3e49f0e5c13..0a7db41b9e42 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -230,7 +230,7 @@ struct netlink_dump_control {
 	int (*done)(struct netlink_callback *);
 	void *data;
 	struct module *module;
-	u16 min_dump_alloc;
+	u32 min_dump_alloc;
 };
 
 int __netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 68e0682450c6..b199137ddcdf 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3709,13 +3709,13 @@ static int rtnl_dellinkprop(struct sk_buff *skb, struct nlmsghdr *nlh,
 	return rtnl_linkprop(RTM_DELLINKPROP, skb, nlh, extack);
 }
 
-static u16 rtnl_calcit(struct sk_buff *skb, struct nlmsghdr *nlh)
+static u32 rtnl_calcit(struct sk_buff *skb, struct nlmsghdr *nlh)
 {
 	struct net *net = sock_net(skb->sk);
 	struct net_device *dev;
 	struct nlattr *tb[IFLA_MAX+1];
 	u32 ext_filter_mask = 0;
-	u16 min_ifinfo_dump_size = 0;
+	u32 min_ifinfo_dump_size = 0;
 	int hdrlen;
 
 	/* Same kernel<->userspace interface hack as in rtnl_dump_ifinfo. */
@@ -3735,7 +3735,7 @@ static u16 rtnl_calcit(struct sk_buff *skb, struct nlmsghdr *nlh)
 	 */
 	rcu_read_lock();
 	for_each_netdev_rcu(net, dev) {
-		min_ifinfo_dump_size = max_t(u16, min_ifinfo_dump_size,
+		min_ifinfo_dump_size = max_t(u32, min_ifinfo_dump_size,
 					     if_nlmsg_size(dev,
 						           ext_filter_mask));
 	}
@@ -5494,7 +5494,7 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (kind == 2 && nlh->nlmsg_flags&NLM_F_DUMP) {
 		struct sock *rtnl;
 		rtnl_dumpit_func dumpit;
-		u16 min_dump_alloc = 0;
+		u32 min_dump_alloc = 0;
 
 		link = rtnl_get_link(family, type);
 		if (!link || !link->dumpit) {
-- 
2.23.0

Re: [PATCH] rtnetlink: fix data overflow in rtnl_calcit()

[Jesse Brandeburg]

zhudi wrote:

> "ip addr show" command execute error when we have a physical
> network card with number of VFs larger than 247.

Oh man, this bug has been hurting us forever and I've tried to fix it
several times without much luck, so thanks for working on it!

CC: David Ahern <dsahern@gmail.com>

As he's mentioned this bug.
 
> The return value of if_nlmsg_size() in rtnl_calcit() will exceed
> range of u16 data type when any network cards has a larger number of
> VFs. rtnl_vfinfo_size() will significant increase needed dump size when
> the value of num_vfs is larger.
> 
> Eventually we get a wrong value of min_ifinfo_dump_size because of overflow
> which decides the memory size needed by netlink dump and netlink_dump()
> will return -EMSGSIZE because of not enough memory was allocated.
> 
> So fix it by promoting  min_dump_alloc data type to u32 to
> avoid data overflow and it's also align with the data type of
> struct netlink_callback{}.min_dump_alloc which is assigned by
> return value of rtnl_calcit()

I defer to others here on whether this is an acceptable API change.

> Signed-off-by: zhudi <zhudi21@huawei.com>

Kernel documentation says for you to use your real name, please do so,
unless you're a rock star and have officially changed your name to
zhudi.

> ---
>  include/linux/netlink.h | 2 +-
>  net/core/rtnetlink.c    | 8 ++++----
>  2 files changed, 5 insertions(+), 5 deletions(-)

Does it work without any changes to iproute2?


> 
> diff --git a/include/linux/netlink.h b/include/linux/netlink.h
> index e3e49f0e5c13..0a7db41b9e42 100644
> --- a/include/linux/netlink.h
> +++ b/include/linux/netlink.h
> @@ -230,7 +230,7 @@ struct netlink_dump_control {
>  	int (*done)(struct netlink_callback *);
>  	void *data;
>  	struct module *module;
> -	u16 min_dump_alloc;
> +	u32 min_dump_alloc;
>  };

As long as nothing in the API depends on the length of this struct, it
should work.

Re: [PATCH] rtnetlink: fix data overflow in rtnl_calcit()

[Vladimir Oltean]

On Fri, Oct 16, 2020 at 02:36:25PM -0700, Jesse Brandeburg wrote:
> > Signed-off-by: zhudi <zhudi21@huawei.com>
> 
> Kernel documentation says for you to use your real name, please do so,
> unless you're a rock star and have officially changed your name to
> zhudi.

Well, his real name is probably 朱棣, and the pinyin transliteration
system doesn't really insist on separating 朱 (zhu) from 棣 (di), or on
capitalizing any of twose words, so I'm not sure what your point is.
Would you prefer his sign-off to read 朱棣 <zhudi21@huawei.com>?

Re: [PATCH] rtnetlink: fix data overflow in rtnl_calcit()

[Jesse Brandeburg]

Vladimir Oltean wrote:

> On Fri, Oct 16, 2020 at 02:36:25PM -0700, Jesse Brandeburg wrote:
> > > Signed-off-by: zhudi <zhudi21@huawei.com>
> > 
> > Kernel documentation says for you to use your real name, please do so,
> > unless you're a rock star and have officially changed your name to
> > zhudi.

I apologize for seeming off-putting, my goal was to add a little levity
here, but I know that email does a bad job of transmitting my intent,
and I will do better.

> 
> Well, his real name is probably 朱棣, and the pinyin transliteration
> system doesn't really insist on separating 朱 (zhu) from 棣 (di), or on
> capitalizing any of twose words, so I'm not sure what your point is.
> Would you prefer his sign-off to read 朱棣 <zhudi21@huawei.com>?

Ah, thanks Vladimir for explaining the difference. If this is common
parlance for commit messages from our Chinese developers, please forgive
me, I'm trying to balance obvious correctness against typical usage.

I'll adjust my expectations for single word names, thanks!

Jesse

答复: [PATCH] rtnetlink: fix data overflow in rtnl_calcit()

[zhudi (J)]

> > "ip addr show" command execute error when we have a physical network
> > card with number of VFs larger than 247.
> 
> Oh man, this bug has been hurting us forever and I've tried to fix it several
> times without much luck, so thanks for working on it!
> 
> CC: David Ahern <dsahern@gmail.com>
> 
> As he's mentioned this bug.
> ...... 
> Kernel documentation says for you to use your real name, please do so,
> unless you're a rock star and have officially changed your name to zhudi.
> 

May be I should use name such as di.zhu  to avoid unnecessary problem :)

> > ---
> >  include/linux/netlink.h | 2 +-
> >  net/core/rtnetlink.c    | 8 ++++----
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> Does it work without any changes to iproute2?

The patch only change the internal data struct currently only used by netlink_dump_start() function.
So I think this patch  has not effects on iproute2 or other packages

Re: [PATCH] rtnetlink: fix data overflow in rtnl_calcit()

[Michal Kubecek]

On Fri, Oct 16, 2020 at 10:02:38AM +0800, zhudi wrote:
> "ip addr show" command execute error when we have a physical
> network card with number of VFs larger than 247.
> 
> The return value of if_nlmsg_size() in rtnl_calcit() will exceed
> range of u16 data type when any network cards has a larger number of
> VFs. rtnl_vfinfo_size() will significant increase needed dump size when
> the value of num_vfs is larger.
> 
> Eventually we get a wrong value of min_ifinfo_dump_size because of overflow
> which decides the memory size needed by netlink dump and netlink_dump()
> will return -EMSGSIZE because of not enough memory was allocated.
> 
> So fix it by promoting  min_dump_alloc data type to u32 to
> avoid data overflow and it's also align with the data type of
> struct netlink_callback{}.min_dump_alloc which is assigned by
> return value of rtnl_calcit()

Unfortunately this is only part of the problem. For a NIC with so many
VFs (not sure if exactly 247 but it's close to that), IFLA_VFINFO_LIST
nested attribute itself would be over 64KB long which is not possible as
attribute size is u16.

So we should rather fail in such case (except when IFLA_VFINFO_LIST
itself fits into 64KB but the whole netlink message would not) and
provide an alternative way to get information about all VFs.

Michal

Re: [PATCH] rtnetlink: fix data overflow in rtnl_calcit()

[Jakub Kicinski]

On Sat, 17 Oct 2020 14:34:11 +0200 Michal Kubecek wrote:
> On Fri, Oct 16, 2020 at 10:02:38AM +0800, zhudi wrote:
> > "ip addr show" command execute error when we have a physical
> > network card with number of VFs larger than 247.
> > 
> > The return value of if_nlmsg_size() in rtnl_calcit() will exceed
> > range of u16 data type when any network cards has a larger number of
> > VFs. rtnl_vfinfo_size() will significant increase needed dump size when
> > the value of num_vfs is larger.
> > 
> > Eventually we get a wrong value of min_ifinfo_dump_size because of overflow
> > which decides the memory size needed by netlink dump and netlink_dump()
> > will return -EMSGSIZE because of not enough memory was allocated.
> > 
> > So fix it by promoting  min_dump_alloc data type to u32 to
> > avoid data overflow and it's also align with the data type of
> > struct netlink_callback{}.min_dump_alloc which is assigned by
> > return value of rtnl_calcit()  
> 
> Unfortunately this is only part of the problem. For a NIC with so many
> VFs (not sure if exactly 247 but it's close to that), IFLA_VFINFO_LIST
> nested attribute itself would be over 64KB long which is not possible as
> attribute size is u16.
> 
> So we should rather fail in such case (except when IFLA_VFINFO_LIST
> itself fits into 64KB but the whole netlink message would not) and
> provide an alternative way to get information about all VFs.

Right, we should probably move to devlink as much as possible.

zhudi, why not use size_t? Seems like the most natural fit for 
counting size.

Re: [PATCH] rtnetlink: fix data overflow in rtnl_calcit()

[zhudi (J)]

 > On Fri, Oct 16, 2020 at 10:02:38AM +0800, zhudi wrote:
> > "ip addr show" command execute error when we have a physical network
> > card with number of VFs larger than 247.
> >
> > The return value of if_nlmsg_size() in rtnl_calcit() will exceed range
> > of u16 data type when any network cards has a larger number of VFs.
> > rtnl_vfinfo_size() will significant increase needed dump size when the
> > value of num_vfs is larger.
> >
> > Eventually we get a wrong value of min_ifinfo_dump_size because of
> > overflow which decides the memory size needed by netlink dump and
> > netlink_dump() will return -EMSGSIZE because of not enough memory was
> allocated.
> >
> > So fix it by promoting  min_dump_alloc data type to u32 to avoid data
> > overflow and it's also align with the data type of struct
> > netlink_callback{}.min_dump_alloc which is assigned by return value of
> > rtnl_calcit()
> 
> Unfortunately this is only part of the problem. For a NIC with so many VFs
> (not sure if exactly 247 but it's close to that), IFLA_VFINFO_LIST nested
> attribute itself would be over 64KB long which is not possible as attribute size
> is u16.
> 
> So we should rather fail in such case (except when IFLA_VFINFO_LIST itself
> fits into 64KB but the whole netlink message would not) and provide an
> alternative way to get information about all VFs.

Thanks for your replying,  it's right. The patch only fix the situation that IFLA_VFINFO_LIST itself
fits into 64KB but the whole netlink message would not.

Re: [PATCH] rtnetlink: fix data overflow in rtnl_calcit()

[zhudi (J)]

> On Sat, 17 Oct 2020 14:34:11 +0200 Michal Kubecek wrote:
> > On Fri, Oct 16, 2020 at 10:02:38AM +0800, zhudi wrote:
> > > "ip addr show" command execute error when we have a physical
> network
> > > card with number of VFs larger than 247.
> > >
> > > The return value of if_nlmsg_size() in rtnl_calcit() will exceed
> > > range of u16 data type when any network cards has a larger number of
> > > VFs. rtnl_vfinfo_size() will significant increase needed dump size
> > > when the value of num_vfs is larger.
> > >
> > > Eventually we get a wrong value of min_ifinfo_dump_size because of
> > > overflow which decides the memory size needed by netlink dump and
> > > netlink_dump() will return -EMSGSIZE because of not enough memory
> was allocated.
> > >
> > > So fix it by promoting  min_dump_alloc data type to u32 to avoid
> > > data overflow and it's also align with the data type of struct
> > > netlink_callback{}.min_dump_alloc which is assigned by return value
> > > of rtnl_calcit()
> >
> > Unfortunately this is only part of the problem. For a NIC with so many
> > VFs (not sure if exactly 247 but it's close to that), IFLA_VFINFO_LIST
> > nested attribute itself would be over 64KB long which is not possible
> > as attribute size is u16.
> >
> > So we should rather fail in such case (except when IFLA_VFINFO_LIST
> > itself fits into 64KB but the whole netlink message would not) and
> > provide an alternative way to get information about all VFs.
> 
> Right, we should probably move to devlink as much as possible.
> 
> zhudi, why not use size_t? Seems like the most natural fit for counting size.

Thanks for your replying.
min_dump_alloc original type used is u16 and it's eventually assigned to 
struct netlink_callback{}. min_dump_alloc which data type is u32. So I just simply
promote to u32.
Should be used size_t instead of u32?

Re: [PATCH] rtnetlink: fix data overflow in rtnl_calcit()

[Jakub Kicinski]

On Mon, 19 Oct 2020 01:59:19 +0000 zhudi (J) wrote:
> > zhudi, why not use size_t? Seems like the most natural fit for counting size.  
> 
> Thanks for your replying.
> min_dump_alloc original type used is u16 and it's eventually assigned to 
> struct netlink_callback{}. min_dump_alloc which data type is u32. So I just simply
> promote to u32.
> Should be used size_t instead of u32?

I had a closer look, and I agree that u32 should be fine in struct
netlink_dump_control, rtnetlink_rcv_msg(), and as a return value from
rtnl_calcit().

But please use size_t for the local variable in rtnl_calcit().
This way you can convert the max_t() to a max().

When you send v2 please move the declaration of min_ifinfo_dump_size
after struct net *net = sock_net(skb->sk); (to get closer to longest 
to shortest declaration order).