| Message ID | 60cde83b718e88aa7fcae7239a3aac704e048efc.1424456163.git.mleitner@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Hi, 2015-02-20, 16:24:06 -0200, Marcelo Ricardo Leitner wrote: > [...] > --- > net/ipv6/addrconf.c | 32 +++++++++++++++++++++++++++++++- > 1 file changed, 31 insertions(+), 1 deletion(-) > > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c > index f7c8bbeb27b704c0106f714d5a0677c27d3346e0..38892228ccacfe8b67b182784723cc0b67ce572b 100644 > --- a/net/ipv6/addrconf.c > +++ b/net/ipv6/addrconf.c > @@ -4863,6 +4863,36 @@ int addrconf_sysctl_forward(struct ctl_table *ctl, int write, > return ret; > } > > +static > +int addrconf_sysctl_mtu(struct ctl_table *ctl, int write, > + void __user *buffer, size_t *lenp, loff_t *ppos) > +{ > + int *valp = ctl->data; > + int val = *valp; > + loff_t pos = *ppos; > + struct ctl_table lctl; > + int ret; > + > + /* ctl->data points to idev->cnf.mtu6 > + */ > + lctl = *ctl; > + lctl.data = &val; > + > + ret = proc_dointvec(&lctl, write, buffer, lenp, ppos); > + > + if (write) { > + struct inet6_dev *idev = ctl->extra1; > + > + if (val >= IPV6_MIN_MTU && val <= idev->dev->mtu) "all" and "default" don't have an idev, so you need a check here: if (val >= IPV6_MIN_MTU && (!idev || val <= idev->dev->mtu)) > + *valp = val; > + else > + ret = -EINVAL; > + } > + if (ret) > + *ppos = pos; > + return ret; > +} > + You could call proc_dointvec_minmax to do the checks. Something like: static int addrconf_sysctl_mtu(struct ctl_table *ctl, int write, void __user *buffer, size_t *lenp, loff_t *ppos) { struct inet6_dev *idev = ctl->extra1; int min_mtu = IPV6_MIN_MTU; struct ctl_table lctl; lctl = *ctl; lctl.extra1 = &min_mtu; lctl.extra2 = idev ? &idev->dev->mtu : NULL; return proc_dointvec_minmax(&lctl, write, buffer, lenp, ppos); } Thanks,
On Sat, Feb 21, 2015 at 04:54:07PM +0100, Sabrina Dubroca wrote: > Hi, > > 2015-02-20, 16:24:06 -0200, Marcelo Ricardo Leitner wrote: > > > [...] > > > --- > > net/ipv6/addrconf.c | 32 +++++++++++++++++++++++++++++++- > > 1 file changed, 31 insertions(+), 1 deletion(-) > > > > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c > > index f7c8bbeb27b704c0106f714d5a0677c27d3346e0..38892228ccacfe8b67b182784723cc0b67ce572b 100644 > > --- a/net/ipv6/addrconf.c > > +++ b/net/ipv6/addrconf.c > > @@ -4863,6 +4863,36 @@ int addrconf_sysctl_forward(struct ctl_table *ctl, int write, > > return ret; > > } > > > > +static > > +int addrconf_sysctl_mtu(struct ctl_table *ctl, int write, > > + void __user *buffer, size_t *lenp, loff_t *ppos) > > +{ > > + int *valp = ctl->data; > > + int val = *valp; > > + loff_t pos = *ppos; > > + struct ctl_table lctl; > > + int ret; > > + > > + /* ctl->data points to idev->cnf.mtu6 > > + */ > > + lctl = *ctl; > > + lctl.data = &val; > > + > > + ret = proc_dointvec(&lctl, write, buffer, lenp, ppos); > > + > > + if (write) { > > + struct inet6_dev *idev = ctl->extra1; > > + > > + if (val >= IPV6_MIN_MTU && val <= idev->dev->mtu) > > "all" and "default" don't have an idev, so you need a check here: > > if (val >= IPV6_MIN_MTU && (!idev || val <= idev->dev->mtu)) Oh, indeed. > > + *valp = val; > > + else > > + ret = -EINVAL; > > + } > > + if (ret) > > + *ppos = pos; > > + return ret; > > +} > > + > > > You could call proc_dointvec_minmax to do the checks. Something like: > > static > int addrconf_sysctl_mtu(struct ctl_table *ctl, int write, > void __user *buffer, size_t *lenp, loff_t *ppos) > { > struct inet6_dev *idev = ctl->extra1; > int min_mtu = IPV6_MIN_MTU; > struct ctl_table lctl; > > lctl = *ctl; > lctl.extra1 = &min_mtu; > lctl.extra2 = idev ? &idev->dev->mtu : NULL; > > return proc_dointvec_minmax(&lctl, write, buffer, lenp, ppos); > } That's much better! Weird that I did look for a _minmax helper and missed it :/ Thanks Sabrina, I'll send a v2 soon. Marcelo -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c index f7c8bbeb27b704c0106f714d5a0677c27d3346e0..38892228ccacfe8b67b182784723cc0b67ce572b 100644 --- a/net/ipv6/addrconf.c +++ b/net/ipv6/addrconf.c @@ -4863,6 +4863,36 @@ int addrconf_sysctl_forward(struct ctl_table *ctl, int write, return ret; } +static +int addrconf_sysctl_mtu(struct ctl_table *ctl, int write, + void __user *buffer, size_t *lenp, loff_t *ppos) +{ + int *valp = ctl->data; + int val = *valp; + loff_t pos = *ppos; + struct ctl_table lctl; + int ret; + + /* ctl->data points to idev->cnf.mtu6 + */ + lctl = *ctl; + lctl.data = &val; + + ret = proc_dointvec(&lctl, write, buffer, lenp, ppos); + + if (write) { + struct inet6_dev *idev = ctl->extra1; + + if (val >= IPV6_MIN_MTU && val <= idev->dev->mtu) + *valp = val; + else + ret = -EINVAL; + } + if (ret) + *ppos = pos; + return ret; +} + static void dev_disable_change(struct inet6_dev *idev) { struct netdev_notifier_info info; @@ -5014,7 +5044,7 @@ static struct addrconf_sysctl_table .data = &ipv6_devconf.mtu6, .maxlen = sizeof(int), .mode = 0644, - .proc_handler = proc_dointvec, + .proc_handler = addrconf_sysctl_mtu, }, { .procname = "accept_ra",
Currently we don't check if the new MTU is valid or not and this allows one to configure a smaller than minimum allowed by RFCs or even bigger than interface own MTU, which is a problem as it may lead to packet drops. If you have a daemon like NetworkManager running, this may be exploited by remote attackers by forging RA packets with an invalid MTU, possibly leading to a DoS. (NetworkManager currently only validates for values too small, but not for too big ones.) The fix is just to make sure the new value is valid. That is, between IPV6_MIN_MTU and interface's MTU. Note that similar check is already performed at ndisc_router_discovery(), for when kernel itself parses the RA. Signed-off-by: Marcelo Ricardo Leitner <mleitner@redhat.com> --- net/ipv6/addrconf.c | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-)