| Message ID | 5269ddae1a0a164a488703654c9bb6e24845c637.1426509272.git.siva.mannem.lnx@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, Mar 16, 2015 at 4:02 PM, Siva Mannem <siva.mannem.lnx@gmail.com> wrote: > This patch validates all netlink attributes and return error if any of the > validation fails. > > Signed-off-by: Siva Mannem <siva.mannem.lnx@gmail.com> > Suggested-by: David Miller <davem@davemloft.net> > --- > net/bridge/br_netlink.c | 39 ++++++++++++++++++++++++++++++++++++++- > 1 file changed, 38 insertions(+), 1 deletion(-) > > diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c > index d80e802..0b18c0d 100644 > --- a/net/bridge/br_netlink.c > +++ b/net/bridge/br_netlink.c > @@ -740,12 +740,49 @@ static int br_changelink(struct net_device *brdev, struct nlattr *tb[], > struct nlattr *data[]) > { > struct net_bridge *br = netdev_priv(brdev); > - int err; > + unsigned long forward_delay; > + unsigned long hello_time; > + unsigned long max_age; > + unsigned long ageing_time; > + u32 t; > + int err = -ERANGE; > > if (!data) > return 0; > > if (data[IFLA_BR_FORWARD_DELAY]) { > + t = nla_get_u32(data[IFLA_BR_FORWARD_DELAY]); > + forward_delay = clock_t_to_jiffies(t); > + if (forward_delay < BR_MIN_FORWARD_DELAY || > + forward_delay > BR_MAX_FORWARD_DELAY) > + return err; Is there a way to avoiding duplicate range checking code by passing an extra arg to br_set_forward_delay(..., bool validate_only) and friends? -- 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
From: Siva Mannem <siva.mannem.lnx@gmail.com> Date: Tue, 17 Mar 2015 04:32:44 +0530 > This patch validates all netlink attributes and return error if any of the > validation fails. > > Signed-off-by: Siva Mannem <siva.mannem.lnx@gmail.com> > Suggested-by: David Miller <davem@davemloft.net> You have to remove the range checking code from all the other functions too. Then no errors can be returned by them at all actually and that's an really nice cleanup. Please don't make microscopic narrow changes like this, think about the higher level implications and the things that are no longer necessary as a result. Thanks. -- 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
Also, this should be the first change in your patch series. Your current first patch just adds more bogus range checking to the helper functions, which this patch makes %100 unnecessary. -- 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
On Tue, Mar 17, 2015 at 9:54 AM, David Miller <davem@davemloft.net> wrote: > From: Siva Mannem <siva.mannem.lnx@gmail.com> > Date: Tue, 17 Mar 2015 04:32:44 +0530 > >> This patch validates all netlink attributes and return error if any of the >> validation fails. >> >> Signed-off-by: Siva Mannem <siva.mannem.lnx@gmail.com> >> Suggested-by: David Miller <davem@davemloft.net> > > You have to remove the range checking code from all the other > functions too. > > Then no errors can be returned by them at all actually and that's an > really nice cleanup. > > Please don't make microscopic narrow changes like this, think about > the higher level implications and the things that are no longer > necessary as a result. > > Thanks. These functions(br_set_forward_delay/hello_time/max_age/ageing_time) are also called from other places i.e old_dev_ioctl() and sysfs code. If range checking has to be removed from these functions, then the same has to be added in the path from where they are being invoked. As Scott mentioned, these functions can be enhanced to take extra argument to avoid extra range checking happening in br_changelink() path. Is this approach ok?
From: Siva Mannem <siva.mannem.lnx@gmail.com> Date: Tue, 17 Mar 2015 10:29:28 +0530 > As Scott mentioned, these functions can be enhanced to take > extra argument to avoid extra range checking happening in br_changelink() > path. Is this approach ok? Make validation helper inline functions, call them from the individual sysfs call sites and from br_changelink(). -- 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/bridge/br_netlink.c b/net/bridge/br_netlink.c index d80e802..0b18c0d 100644 --- a/net/bridge/br_netlink.c +++ b/net/bridge/br_netlink.c @@ -740,12 +740,49 @@ static int br_changelink(struct net_device *brdev, struct nlattr *tb[], struct nlattr *data[]) { struct net_bridge *br = netdev_priv(brdev); - int err; + unsigned long forward_delay; + unsigned long hello_time; + unsigned long max_age; + unsigned long ageing_time; + u32 t; + int err = -ERANGE; if (!data) return 0; if (data[IFLA_BR_FORWARD_DELAY]) { + t = nla_get_u32(data[IFLA_BR_FORWARD_DELAY]); + forward_delay = clock_t_to_jiffies(t); + if (forward_delay < BR_MIN_FORWARD_DELAY || + forward_delay > BR_MAX_FORWARD_DELAY) + return err; + } + + if (data[IFLA_BR_HELLO_TIME]) { + t = nla_get_u32(data[IFLA_BR_HELLO_TIME]); + hello_time = clock_t_to_jiffies(t); + if (hello_time < BR_MIN_HELLO_TIME || + hello_time > BR_MAX_HELLO_TIME) + return err; + } + + if (data[IFLA_BR_MAX_AGE]) { + t = nla_get_u32(data[IFLA_BR_MAX_AGE]); + max_age = clock_t_to_jiffies(t); + if (max_age < BR_MIN_MAX_AGE || + max_age > BR_MAX_MAX_AGE) + return err; + } + + if (data[IFLA_BR_AGEING_TIME]) { + t = nla_get_u32(data[IFLA_BR_AGEING_TIME]); + ageing_time = clock_t_to_jiffies(t); + if (ageing_time < BR_MIN_AGEING_TIME || + ageing_time > BR_MAX_AGEING_TIME) + return err; + } + + if (data[IFLA_BR_FORWARD_DELAY]) { err = br_set_forward_delay(br, nla_get_u32(data[IFLA_BR_FORWARD_DELAY])); if (err) return err;
This patch validates all netlink attributes and return error if any of the validation fails. Signed-off-by: Siva Mannem <siva.mannem.lnx@gmail.com> Suggested-by: David Miller <davem@davemloft.net> --- net/bridge/br_netlink.c | 39 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-)