Re: [PATCH 1/3] dev: Prevent creating network devices with negative ifindex
>> What do you think? > > Passing -1 is an error, it doesn't make sense to try and be > helpful to buggy userland. Here is commit I actually change/fix: commit 9c7dafbfab15 ("net: Allow to create links with given ifindex") In this change done the opposite: check for ifm->ifi_index moved to register_netdevice() from rtnl_newlink(). Let me understand finally: why do I need to reverse and move check for dev->ifindex from register_netdevice() to rtnl_newlink()? That will partially revert commit I note above. -- Thanks, Serhey
Re: [PATCH 1/3] dev: Prevent creating network devices with negative ifindex
On Fri, 16 Jun 2017 19:44:45 +0300 Serhey Popovychwrote: > > On Fri, 16 Jun 2017 17:23:51 +0300 > > Serhey Popovych wrote: > > > >> Interface index is signed integer, we can pass ifm->ifi_index > >> from userspace via netlink and create network device with > >> negative ifindex value. > >> > >> Fixes: 9c7dafbfab15 ("net: Allow to create links with given ifindex") > >> Signed-off-by: Serhey Popovych > >> --- > >> net/core/dev.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/net/core/dev.c b/net/core/dev.c > >> index 8658074..dae8010 100644 > >> --- a/net/core/dev.c > >> +++ b/net/core/dev.c > >> @@ -7491,7 +7491,7 @@ int register_netdevice(struct net_device *dev) > >>} > >> > >>ret = -EBUSY; > >> - if (!dev->ifindex) > >> + if (dev->ifindex <= 0) > >>dev->ifindex = dev_new_index(net); > >>else if (__dev_get_by_index(net, dev->ifindex)) > >>goto err_uninit; > > > > You should fix this by adding error check in the netlink portion > > that allows creating devices with given ifindex. Passing < 0 > > should be an error.But should this break some setups if I add such check to > > netlink > portion? In my opinion it is better to choose silently different > ifindex rather than reporting failure. That's why I prefer doing > this in register_netdevice(). > > Also there is similar problem for drivers/net/veth.c, it might > happen that other places will be added later that setup > dev->ifindex and then call register_netdevice(). > > What do you think? Passing -1 is an error, it doesn't make sense to try and be helpful to buggy userland.
Re: [PATCH 1/3] dev: Prevent creating network devices with negative ifindex
> On Fri, 16 Jun 2017 17:23:51 +0300 > Serhey Popovychwrote: > >> Interface index is signed integer, we can pass ifm->ifi_index >> from userspace via netlink and create network device with >> negative ifindex value. >> >> Fixes: 9c7dafbfab15 ("net: Allow to create links with given ifindex") >> Signed-off-by: Serhey Popovych >> --- >> net/core/dev.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/net/core/dev.c b/net/core/dev.c >> index 8658074..dae8010 100644 >> --- a/net/core/dev.c >> +++ b/net/core/dev.c >> @@ -7491,7 +7491,7 @@ int register_netdevice(struct net_device *dev) >> } >> >> ret = -EBUSY; >> -if (!dev->ifindex) >> +if (dev->ifindex <= 0) >> dev->ifindex = dev_new_index(net); >> else if (__dev_get_by_index(net, dev->ifindex)) >> goto err_uninit; > > You should fix this by adding error check in the netlink portion > that allows creating devices with given ifindex. Passing < 0 > should be an error.But should this break some setups if I add such check to > netlink portion? In my opinion it is better to choose silently different ifindex rather than reporting failure. That's why I prefer doing this in register_netdevice(). Also there is similar problem for drivers/net/veth.c, it might happen that other places will be added later that setup dev->ifindex and then call register_netdevice(). What do you think? Thanks for quick review! > -- Thanks, Serhey
Re: [PATCH 1/3] dev: Prevent creating network devices with negative ifindex
On Fri, 16 Jun 2017 17:23:51 +0300 Serhey Popovychwrote: > Interface index is signed integer, we can pass ifm->ifi_index > from userspace via netlink and create network device with > negative ifindex value. > > Fixes: 9c7dafbfab15 ("net: Allow to create links with given ifindex") > Signed-off-by: Serhey Popovych > --- > net/core/dev.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/core/dev.c b/net/core/dev.c > index 8658074..dae8010 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -7491,7 +7491,7 @@ int register_netdevice(struct net_device *dev) > } > > ret = -EBUSY; > - if (!dev->ifindex) > + if (dev->ifindex <= 0) > dev->ifindex = dev_new_index(net); > else if (__dev_get_by_index(net, dev->ifindex)) > goto err_uninit; You should fix this by adding error check in the netlink portion that allows creating devices with given ifindex. Passing < 0 should be an error.
[PATCH 1/3] dev: Prevent creating network devices with negative ifindex
Interface index is signed integer, we can pass ifm->ifi_index from userspace via netlink and create network device with negative ifindex value. Fixes: 9c7dafbfab15 ("net: Allow to create links with given ifindex") Signed-off-by: Serhey Popovych--- net/core/dev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/core/dev.c b/net/core/dev.c index 8658074..dae8010 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -7491,7 +7491,7 @@ int register_netdevice(struct net_device *dev) } ret = -EBUSY; - if (!dev->ifindex) + if (dev->ifindex <= 0) dev->ifindex = dev_new_index(net); else if (__dev_get_by_index(net, dev->ifindex)) goto err_uninit; -- 1.8.3.1