| Message ID | c9860365b10dbd648724a6d65ea0dede8ec56464.1531129207.git.sd@queasysnail.net |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net/ipv6: addr_gen_mode fixes | expand |
On 7/9/18 4:25 AM, Sabrina Dubroca wrote: > addrconf_sysctl_addr_gen_mode() has multiple problems. First, it ignores > the errors returned by proc_dointvec(). > > addrconf_sysctl_addr_gen_mode() calls proc_dointvec() directly, which > writes the value to memory, and then checks if it's valid and may return > EINVAL. If a bad value is given, the value displayed when reading > net.ipv6.conf.foo.addr_gen_mode next time will be invalid. In case the > value provided by the user was valid, addrconf_dev_config() won't be > called since idev->cnf.addr_gen_mode has already been updated. > > Fix this in the usual way we deal with values that need to be checked > after the proc_do*() helper has returned: define a local ctl_table and > storage, call proc_dointvec() on that temporary area, then check and > store. > > addrconf_sysctl_addr_gen_mode() also writes the new value to the global > ipv6_devconf_dflt, when we're writing to some netns's default, so that > new netns will inherit the value that was set by the change occuring in > any netns. That doesn't make any sense, so let's drop this assignment. > > Finally, since addr_gen_mode is a __u32, switch to proc_douintvec(). > > Fixes: d35a00b8e33d ("net/ipv6: allow sysctl to change link-local address generation mode") > Signed-off-by: Sabrina Dubroca <sd@queasysnail.net> > --- > net/ipv6/addrconf.c | 27 ++++++++++++++------------- > 1 file changed, 14 insertions(+), 13 deletions(-) > Reviewed-by: David Ahern <dsahern@gmail.com>
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c index 91580c62bb86..e9ba53d2a147 100644 --- a/net/ipv6/addrconf.c +++ b/net/ipv6/addrconf.c @@ -5892,32 +5892,31 @@ static int addrconf_sysctl_addr_gen_mode(struct ctl_table *ctl, int write, loff_t *ppos) { int ret = 0; - int new_val; + u32 new_val; struct inet6_dev *idev = (struct inet6_dev *)ctl->extra1; struct net *net = (struct net *)ctl->extra2; + struct ctl_table tmp = { + .data = &new_val, + .maxlen = sizeof(new_val), + .mode = ctl->mode, + }; if (!rtnl_trylock()) return restart_syscall(); - ret = proc_dointvec(ctl, write, buffer, lenp, ppos); + new_val = *((u32 *)ctl->data); - if (write) { - new_val = *((int *)ctl->data); + ret = proc_douintvec(&tmp, write, buffer, lenp, ppos); + if (ret != 0) + goto out; + if (write) { if (check_addr_gen_mode(new_val) < 0) { ret = -EINVAL; goto out; } - /* request for default */ - if (&net->ipv6.devconf_dflt->addr_gen_mode == ctl->data) { - ipv6_devconf_dflt.addr_gen_mode = new_val; - - /* request for individual net device */ - } else { - if (!idev) - goto out; - + if (idev) { if (check_stable_privacy(idev, net, new_val) < 0) { ret = -EINVAL; goto out; @@ -5928,6 +5927,8 @@ static int addrconf_sysctl_addr_gen_mode(struct ctl_table *ctl, int write, addrconf_dev_config(idev->dev); } } + + *((u32 *)ctl->data) = new_val; } out:
addrconf_sysctl_addr_gen_mode() has multiple problems. First, it ignores the errors returned by proc_dointvec(). addrconf_sysctl_addr_gen_mode() calls proc_dointvec() directly, which writes the value to memory, and then checks if it's valid and may return EINVAL. If a bad value is given, the value displayed when reading net.ipv6.conf.foo.addr_gen_mode next time will be invalid. In case the value provided by the user was valid, addrconf_dev_config() won't be called since idev->cnf.addr_gen_mode has already been updated. Fix this in the usual way we deal with values that need to be checked after the proc_do*() helper has returned: define a local ctl_table and storage, call proc_dointvec() on that temporary area, then check and store. addrconf_sysctl_addr_gen_mode() also writes the new value to the global ipv6_devconf_dflt, when we're writing to some netns's default, so that new netns will inherit the value that was set by the change occuring in any netns. That doesn't make any sense, so let's drop this assignment. Finally, since addr_gen_mode is a __u32, switch to proc_douintvec(). Fixes: d35a00b8e33d ("net/ipv6: allow sysctl to change link-local address generation mode") Signed-off-by: Sabrina Dubroca <sd@queasysnail.net> --- net/ipv6/addrconf.c | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-)