RE: [net-next 1/2] dummy: add device MTU validation check
> -Original Message- > From: Sabrina Dubroca [mailto:s...@queasysnail.net] > Sent: 2017年9月22日 20:23 > To: Eric Dumazet <eric.duma...@gmail.com> > Cc: Jarod Wilson <ja...@redhat.com>; Zhang Shengju > <zhangshen...@cmss.chinamobile.com>; da...@davemloft.net; > will...@google.com; step...@networkplumber.org; > netdev@vger.kernel.org > Subject: Re: [net-next 1/2] dummy: add device MTU validation check > > 2017-09-22, 04:05:09 -0700, Eric Dumazet wrote: > > On Fri, 2017-09-22 at 10:56 +0200, Sabrina Dubroca wrote: > > > 2017-09-21, 08:02:18 -0700, Eric Dumazet wrote: > > > > On Thu, 2017-09-21 at 21:32 +0800, Zhang Shengju wrote: > > > > > Currently, any mtu value can be assigned when adding a new dummy > device: > > > > > [~]# ip link add name dummy1 mtu 10 type dummy [~]# ip link > > > > > show dummy1 > > > > > 15: dummy1: <BROADCAST,NOARP> mtu 10 qdisc noop state > DOWN mode DEFAULT group default qlen 1000 > > > > > link/ether 0a:61:6b:16:14:ce brd ff:ff:ff:ff:ff:ff > > > > > > > > > > This patch adds device MTU validation check. > > > > > > > > What is wrong with big MTU on dummy ? > > > > > > It looks like the "centralize MTU checking" series broke that, but > > > only for changing the MTU on an existing dummy device. Commit > > > a52ad514fdf3 defined min_mtu/max_mtu in ether_setup, which dummy > > > uses, but there was no MTU check in dummy prior to that commit. > > > > > > > It looks like we accept big mtu on loopback, right ? > > Yes. I only meant that before commit a52ad514fdf3, there was no range check > on dummy's MTU. Commit 25e3e84b183a ("dummy: expend mtu range for > dummy device") and 8b1efc0f83f1 ("net: remove MTU limits on a few > ether_setup callers") fixed that only partially. It's the same with ifb, btw, > it > didn't have any check before a52ad514fdf3, so we should set min_mtu = > max_mtu = 0. > > -- > Sabrina I agree, dummy and ifb device should not have any limit on mtu, just like loopback device. I will send v2 patch, and set min/max_mtu to zero for dummy and ifb device, thanks. ZSJ
Re: [net-next 1/2] dummy: add device MTU validation check
2017-09-22, 04:05:09 -0700, Eric Dumazet wrote: > On Fri, 2017-09-22 at 10:56 +0200, Sabrina Dubroca wrote: > > 2017-09-21, 08:02:18 -0700, Eric Dumazet wrote: > > > On Thu, 2017-09-21 at 21:32 +0800, Zhang Shengju wrote: > > > > Currently, any mtu value can be assigned when adding a new dummy device: > > > > [~]# ip link add name dummy1 mtu 10 type dummy > > > > [~]# ip link show dummy1 > > > > 15: dummy1:mtu 10 qdisc noop state DOWN mode > > > > DEFAULT group default qlen 1000 > > > > link/ether 0a:61:6b:16:14:ce brd ff:ff:ff:ff:ff:ff > > > > > > > > This patch adds device MTU validation check. > > > > > > What is wrong with big MTU on dummy ? > > > > It looks like the "centralize MTU checking" series broke that, but > > only for changing the MTU on an existing dummy device. Commit > > a52ad514fdf3 defined min_mtu/max_mtu in ether_setup, which dummy uses, > > but there was no MTU check in dummy prior to that commit. > > > > It looks like we accept big mtu on loopback, right ? Yes. I only meant that before commit a52ad514fdf3, there was no range check on dummy's MTU. Commit 25e3e84b183a ("dummy: expend mtu range for dummy device") and 8b1efc0f83f1 ("net: remove MTU limits on a few ether_setup callers") fixed that only partially. It's the same with ifb, btw, it didn't have any check before a52ad514fdf3, so we should set min_mtu = max_mtu = 0. -- Sabrina
Re: [net-next 1/2] dummy: add device MTU validation check
On Fri, 2017-09-22 at 10:56 +0200, Sabrina Dubroca wrote: > 2017-09-21, 08:02:18 -0700, Eric Dumazet wrote: > > On Thu, 2017-09-21 at 21:32 +0800, Zhang Shengju wrote: > > > Currently, any mtu value can be assigned when adding a new dummy device: > > > [~]# ip link add name dummy1 mtu 10 type dummy > > > [~]# ip link show dummy1 > > > 15: dummy1:mtu 10 qdisc noop state DOWN mode > > > DEFAULT group default qlen 1000 > > > link/ether 0a:61:6b:16:14:ce brd ff:ff:ff:ff:ff:ff > > > > > > This patch adds device MTU validation check. > > > > What is wrong with big MTU on dummy ? > > It looks like the "centralize MTU checking" series broke that, but > only for changing the MTU on an existing dummy device. Commit > a52ad514fdf3 defined min_mtu/max_mtu in ether_setup, which dummy uses, > but there was no MTU check in dummy prior to that commit. > It looks like we accept big mtu on loopback, right ? lpaa23:~# ifconfig lo mtu 10 lpaa23:~# ifconfig lo loLink encap:Local Loopback inet addr:127.0.0.1 Mask:255.0.0.0 inet6 addr: ::1/128 Scope:Host UP LOOPBACK RUNNING MTU:10 Metric:1 RX packets:3823 errors:0 dropped:0 overruns:0 frame:0 TX packets:3823 errors:0 dropped:0 overruns:0 carrier:0 collisions:0 txqueuelen:1000 RX bytes:759159 (759.1 KB) TX bytes:759159 (759.1 KB) Also we accept very small MTU as well (although this automatically removes IP addresses, as one would expect) lpaa23:~# ifconfig lo mtu 50 lpaa23:~# ifconfig lo loLink encap:Local Loopback UP LOOPBACK RUNNING MTU:50 Metric:1 RX packets:4052 errors:0 dropped:0 overruns:0 frame:0 TX packets:4052 errors:0 dropped:0 overruns:0 carrier:0 collisions:0 txqueuelen:1000 RX bytes:806274 (806.2 KB) TX bytes:806274 (806.2 KB) So, why dummy devices would not accept bit MTU ? Do we have some fundamental assumption in the stack ? If yes, we need to fix loopback urgently, it is more important than dummy. Thanks.
Re: [net-next 1/2] dummy: add device MTU validation check
2017-09-21, 08:02:18 -0700, Eric Dumazet wrote: > On Thu, 2017-09-21 at 21:32 +0800, Zhang Shengju wrote: > > Currently, any mtu value can be assigned when adding a new dummy device: > > [~]# ip link add name dummy1 mtu 10 type dummy > > [~]# ip link show dummy1 > > 15: dummy1:mtu 10 qdisc noop state DOWN mode DEFAULT > > group default qlen 1000 > > link/ether 0a:61:6b:16:14:ce brd ff:ff:ff:ff:ff:ff > > > > This patch adds device MTU validation check. > > What is wrong with big MTU on dummy ? It looks like the "centralize MTU checking" series broke that, but only for changing the MTU on an existing dummy device. Commit a52ad514fdf3 defined min_mtu/max_mtu in ether_setup, which dummy uses, but there was no MTU check in dummy prior to that commit. > If this is a generic rule, this check should belong in core network > stack. > > > > > Signed-off-by: Zhang Shengju > > --- > > drivers/net/dummy.c | 8 > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/net/dummy.c b/drivers/net/dummy.c > > index e31ab3b..0276b2b 100644 > > --- a/drivers/net/dummy.c > > +++ b/drivers/net/dummy.c > > @@ -365,6 +365,14 @@ static int dummy_validate(struct nlattr *tb[], struct > > nlattr *data[], > > if (!is_valid_ether_addr(nla_data(tb[IFLA_ADDRESS]))) > > return -EADDRNOTAVAIL; > > } > > + > > + if (tb[IFLA_MTU]) { > > + u32 mtu = nla_get_u32(tb[IFLA_MTU]); > > You do not verify/validate nla_len(tb[IFLA_MTU]). I think ifla_policy already performs that check: static const struct nla_policy ifla_policy[IFLA_MAX+1] = { [...] [IFLA_MTU] = { .type = NLA_U32 }, static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh, struct netlink_ext_ack *extack) { [...] err = nlmsg_parse(nlh, sizeof(*ifm), tb, IFLA_MAX, ifla_policy, extack); -- Sabrina
RE: [net-next 1/2] dummy: add device MTU validation check
> -Original Message- > From: Eric Dumazet [mailto:eric.duma...@gmail.com] > Sent: 2017年9月21日 23:02 > To: Zhang Shengju <zhangshen...@cmss.chinamobile.com> > Cc: da...@davemloft.net; will...@google.com; > step...@networkplumber.org; netdev@vger.kernel.org > Subject: Re: [net-next 1/2] dummy: add device MTU validation check > > On Thu, 2017-09-21 at 21:32 +0800, Zhang Shengju wrote: > > Currently, any mtu value can be assigned when adding a new dummy device: > > [~]# ip link add name dummy1 mtu 10 type dummy [~]# ip link show > > dummy1 > > 15: dummy1: <BROADCAST,NOARP> mtu 10 qdisc noop state DOWN > mode DEFAULT group default qlen 1000 > > link/ether 0a:61:6b:16:14:ce brd ff:ff:ff:ff:ff:ff > > > > This patch adds device MTU validation check. > > What is wrong with big MTU on dummy ? > > If this is a generic rule, this check should belong in core network stack. > dummy_setup() function setup mtu range: [0, ETH_MAX_MTU]. This will be checked at dev_set_mtu() function in core network stack. So if you add a new dummy device without specify mtu value, you can't set a value out of range [0, ETH_MAX_MTU] afterward. BUT you can set any mtu when adding new device. This cause an inconsistence. > > > > Signed-off-by: Zhang Shengju <zhangshen...@cmss.chinamobile.com> > > --- > > drivers/net/dummy.c | 8 > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/net/dummy.c b/drivers/net/dummy.c index > > e31ab3b..0276b2b 100644 > > --- a/drivers/net/dummy.c > > +++ b/drivers/net/dummy.c > > @@ -365,6 +365,14 @@ static int dummy_validate(struct nlattr *tb[], struct > nlattr *data[], > > if (!is_valid_ether_addr(nla_data(tb[IFLA_ADDRESS]))) > > return -EADDRNOTAVAIL; > > } > > + > > + if (tb[IFLA_MTU]) { > > + u32 mtu = nla_get_u32(tb[IFLA_MTU]); > > You do not verify/validate nla_len(tb[IFLA_MTU]). > > Do not ever trust user space. MTU attribute is just u32, do you think it's necessary to check the length? Actually I don't see any place to check the length of mtu attribute in network stack code. > > > + > > + if (mtu > ETH_MAX_MTU) > > + return -EINVAL; > > + } > > + > > return 0; > > } > > >
Re: [net-next 1/2] dummy: add device MTU validation check
On Thu, 2017-09-21 at 21:32 +0800, Zhang Shengju wrote: > Currently, any mtu value can be assigned when adding a new dummy device: > [~]# ip link add name dummy1 mtu 10 type dummy > [~]# ip link show dummy1 > 15: dummy1:mtu 10 qdisc noop state DOWN mode DEFAULT > group default qlen 1000 > link/ether 0a:61:6b:16:14:ce brd ff:ff:ff:ff:ff:ff > > This patch adds device MTU validation check. What is wrong with big MTU on dummy ? If this is a generic rule, this check should belong in core network stack. > > Signed-off-by: Zhang Shengju > --- > drivers/net/dummy.c | 8 > 1 file changed, 8 insertions(+) > > diff --git a/drivers/net/dummy.c b/drivers/net/dummy.c > index e31ab3b..0276b2b 100644 > --- a/drivers/net/dummy.c > +++ b/drivers/net/dummy.c > @@ -365,6 +365,14 @@ static int dummy_validate(struct nlattr *tb[], struct > nlattr *data[], > if (!is_valid_ether_addr(nla_data(tb[IFLA_ADDRESS]))) > return -EADDRNOTAVAIL; > } > + > + if (tb[IFLA_MTU]) { > + u32 mtu = nla_get_u32(tb[IFLA_MTU]); You do not verify/validate nla_len(tb[IFLA_MTU]). Do not ever trust user space. > + > + if (mtu > ETH_MAX_MTU) > + return -EINVAL; > + } > + > return 0; > } >