Invalid speed settings by the user are currently acknowledged as correct but not stored. Instead the return of the store operation of the file "gw_bandwidth" should indicate that the given value is not acceptable.
Signed-off-by: Sven Eckelmann sven@narfation.org --- v2: * rebased on current master * add missing header linux/errno.h
net/batman-adv/gateway_common.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/batman-adv/gateway_common.c b/net/batman-adv/gateway_common.c index c50931c..6b930a6 100644 --- a/net/batman-adv/gateway_common.c +++ b/net/batman-adv/gateway_common.c @@ -19,6 +19,7 @@ #include "main.h"
#include <linux/atomic.h> +#include <linux/errno.h> #include <linux/byteorder/generic.h> #include <linux/kernel.h> #include <linux/netdevice.h> @@ -160,7 +161,7 @@ ssize_t batadv_gw_bandwidth_set(struct net_device *net_dev, char *buff,
ret = batadv_parse_gw_bandwidth(net_dev, buff, &down_new, &up_new); if (!ret) - goto end; + return -EINVAL;
if (!down_new) down_new = 1; @@ -184,7 +185,6 @@ ssize_t batadv_gw_bandwidth_set(struct net_device *net_dev, char *buff, atomic_set(&bat_priv->gw.bandwidth_up, up_new); batadv_gw_tvlv_container_update(bat_priv);
-end: return count; }
Signed-off-by: Sven Eckelmann sven@narfation.org --- v2: * rebased on current master
compat-include/linux/kernel.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/compat-include/linux/kernel.h b/compat-include/linux/kernel.h index 2015f7f..663f9e9 100644 --- a/compat-include/linux/kernel.h +++ b/compat-include/linux/kernel.h @@ -36,6 +36,18 @@ _r = -ERANGE;\ _r;\ }) + +#define kstrtou64(cp, base, v)\ +({\ + unsigned long long _v;\ + int _r;\ + _r = strict_strtoull(cp, base, &_v);\ + *(v) = (uint64_t)_v;\ + if ((unsigned long long)*(v) != _v)\ + _r = -ERANGE;\ + _r;\ +}) + #define kstrtoul strict_strtoul #define kstrtol strict_strtol
On Sunday, June 21, 2015 14:42:50 Sven Eckelmann wrote:
Signed-off-by: Sven Eckelmann sven@narfation.org
v2:
- rebased on current master
compat-include/linux/kernel.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)
Applied in revision 194b581.
Thanks, Marek
Signed-off-by: Sven Eckelmann sven@narfation.org --- v2: * rebased on current master
compat-include/linux/kernel.h | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/compat-include/linux/kernel.h b/compat-include/linux/kernel.h index 663f9e9..c39cbe8 100644 --- a/compat-include/linux/kernel.h +++ b/compat-include/linux/kernel.h @@ -53,4 +53,21 @@
#endif /* < KERNEL_VERSION(2, 6, 39) */
+#if LINUX_VERSION_CODE < KERNEL_VERSION(3, 14, 0) + +#define U8_MAX ((u8)~0U) +#define S8_MAX ((s8)(U8_MAX >> 1)) +#define S8_MIN ((s8)(-S8_MAX - 1)) +#define U16_MAX ((u16)~0U) +#define S16_MAX ((s16)(U16_MAX >> 1)) +#define S16_MIN ((s16)(-S16_MAX - 1)) +#define U32_MAX ((u32)~0U) +#define S32_MAX ((s32)(U32_MAX >> 1)) +#define S32_MIN ((s32)(-S32_MAX - 1)) +#define U64_MAX ((u64)~0ULL) +#define S64_MAX ((s64)(U64_MAX >> 1)) +#define S64_MIN ((s64)(-S64_MAX - 1)) + +#endif /* < KERNEL_VERSION(3, 14, 0) */ + #endif /* _NET_BATMAN_ADV_COMPAT_LINUX_KERNEL_H_ */
On Sunday, June 21, 2015 14:42:51 Sven Eckelmann wrote:
Signed-off-by: Sven Eckelmann sven@narfation.org
v2:
- rebased on current master
compat-include/linux/kernel.h | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
Applied in revision 557adc4.
Thanks, Marek
The TVLV for the gw_bandwidth stores everything as u32. But the gw_bandwidth reads the signed long which limits the maximum value to (2 ** 31 - 1) on systems with 4 byte long. Also the input value is always converted from either Mibit/s or Kibit/s to 100Kibit/s. This reduces the values even further when the user sets it via the default unit Kibit/s. It may even cause an integer overflow and end up with a value the user never intended.
Instead read the values as u64, check for possible overflows, do the unit adjustments and then reduce the size to u32.
Signed-off-by: Sven Eckelmann sven@narfation.org --- v2: * rebased on current master
net/batman-adv/gateway_common.c | 49 +++++++++++++++++++++++++++++++++++------ 1 file changed, 42 insertions(+), 7 deletions(-)
diff --git a/net/batman-adv/gateway_common.c b/net/batman-adv/gateway_common.c index 6b930a6..058b957 100644 --- a/net/batman-adv/gateway_common.c +++ b/net/batman-adv/gateway_common.c @@ -18,6 +18,7 @@ #include "gateway_common.h" #include "main.h"
+#include <asm/div64.h> #include <linux/atomic.h> #include <linux/errno.h> #include <linux/byteorder/generic.h> @@ -44,7 +45,7 @@ static bool batadv_parse_gw_bandwidth(struct net_device *net_dev, char *buff, { enum batadv_bandwidth_units bw_unit_type = BATADV_BW_UNIT_KBIT; char *slash_ptr, *tmp_ptr; - long ldown, lup; + u64 ldown, lup; int ret;
slash_ptr = strchr(buff, '/'); @@ -62,7 +63,7 @@ static bool batadv_parse_gw_bandwidth(struct net_device *net_dev, char *buff, *tmp_ptr = '\0'; }
- ret = kstrtol(buff, 10, &ldown); + ret = kstrtou64(buff, 10, &ldown); if (ret) { batadv_err(net_dev, "Download speed of gateway mode invalid: %s\n", @@ -72,14 +73,31 @@ static bool batadv_parse_gw_bandwidth(struct net_device *net_dev, char *buff,
switch (bw_unit_type) { case BATADV_BW_UNIT_MBIT: - *down = ldown * 10; + /* prevent overflow */ + if (U64_MAX / 10 < ldown) { + batadv_err(net_dev, + "Download speed of gateway mode too large: %s\n", + buff); + return false; + } + + ldown *= 10; break; case BATADV_BW_UNIT_KBIT: default: - *down = ldown / 100; + do_div(ldown, 100); break; }
+ if (U32_MAX < ldown) { + batadv_err(net_dev, + "Download speed of gateway mode too large: %s\n", + buff); + return false; + } + + *down = ldown; + /* we also got some upload info */ if (slash_ptr) { bw_unit_type = BATADV_BW_UNIT_KBIT; @@ -95,7 +113,7 @@ static bool batadv_parse_gw_bandwidth(struct net_device *net_dev, char *buff, *tmp_ptr = '\0'; }
- ret = kstrtol(slash_ptr + 1, 10, &lup); + ret = kstrtou64(slash_ptr + 1, 10, &lup); if (ret) { batadv_err(net_dev, "Upload speed of gateway mode invalid: %s\n", @@ -105,13 +123,30 @@ static bool batadv_parse_gw_bandwidth(struct net_device *net_dev, char *buff,
switch (bw_unit_type) { case BATADV_BW_UNIT_MBIT: - *up = lup * 10; + /* prevent overflow */ + if (U64_MAX / 10 < lup) { + batadv_err(net_dev, + "Upload speed of gateway mode too large: %s\n", + slash_ptr + 1); + return false; + } + + lup *= 10; break; case BATADV_BW_UNIT_KBIT: default: - *up = lup / 100; + do_div(lup, 100); break; } + + if (U32_MAX < lup) { + batadv_err(net_dev, + "Upload speed of gateway mode too large: %s\n", + slash_ptr + 1); + return false; + } + + *up = lup; }
return true;
On 21/06/15 14:42, Sven Eckelmann wrote:
The TVLV for the gw_bandwidth stores everything as u32. But the gw_bandwidth reads the signed long which limits the maximum value to (2 ** 31 - 1) on systems with 4 byte long. Also the input value is always converted from either Mibit/s or Kibit/s to 100Kibit/s. This reduces the values even further when the user sets it via the default unit Kibit/s. It may even cause an integer overflow and end up with a value the user never intended.
Instead read the values as u64, check for possible overflows, do the unit adjustments and then reduce the size to u32.
Signed-off-by: Sven Eckelmann sven@narfation.org
v2:
- rebased on current master
shouldn't this patch be for maint as it is also fixing a potential overflow issue?
On Monday 29 June 2015 11:47:00 Antonio Quartulli wrote:
On 21/06/15 14:42, Sven Eckelmann wrote:
The TVLV for the gw_bandwidth stores everything as u32. But the gw_bandwidth reads the signed long which limits the maximum value to (2 ** 31 - 1) on systems with 4 byte long. Also the input value is always converted from either Mibit/s or Kibit/s to 100Kibit/s. This reduces the values even further when the user sets it via the default unit Kibit/s. It may even cause an integer overflow and end up with a value the user never intended.
Instead read the values as u64, check for possible overflows, do the unit adjustments and then reduce the size to u32.
Signed-off-by: Sven Eckelmann sven@narfation.org
v2:
- rebased on current master
shouldn't this patch be for maint as it is also fixing a potential overflow issue?
Only the configuration for users is not correctly stored. For example 4294967296Mbit would result in 0.1 Mbit instead of showing a failure. This is hopefully not the biggest problem.
Just tell me if you want it resubmitted against maint (Linux 4.1.x).
Kind regards, Sven
On 29/06/15 18:16, Sven Eckelmann wrote:
Only the configuration for users is not correctly stored. For example 4294967296Mbit would result in 0.1 Mbit instead of showing a failure. This is hopefully not the biggest problem.
Thanks for the explanation. I don't think we should consider this as a fix then - it looks rather uncritical and rare.
Just tell me if you want it resubmitted against maint (Linux 4.1.x).
No, thanks. This is not required at this point.
Cheers,
On Sunday, June 21, 2015 14:42:52 Sven Eckelmann wrote:
The TVLV for the gw_bandwidth stores everything as u32. But the gw_bandwidth reads the signed long which limits the maximum value to (2 ** 31 - 1) on systems with 4 byte long. Also the input value is always converted from either Mibit/s or Kibit/s to 100Kibit/s. This reduces the values even further when the user sets it via the default unit Kibit/s. It may even cause an integer overflow and end up with a value the user never intended.
Instead read the values as u64, check for possible overflows, do the unit adjustments and then reduce the size to u32.
Signed-off-by: Sven Eckelmann sven@narfation.org
v2:
- rebased on current master
net/batman-adv/gateway_common.c | 49 +++++++++++++++++++++++++++++++++++------ 1 file changed, 42 insertions(+), 7 deletions(-)
Applied in revision ca6b86f.
Thanks, Marek
On Sunday, June 21, 2015 14:42:49 Sven Eckelmann wrote:
Invalid speed settings by the user are currently acknowledged as correct but not stored. Instead the return of the store operation of the file "gw_bandwidth" should indicate that the given value is not acceptable.
Signed-off-by: Sven Eckelmann sven@narfation.org
v2:
- rebased on current master
- add missing header linux/errno.h
net/batman-adv/gateway_common.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Applied in revision aa203f7.
Thanks, Marek
b.a.t.m.a.n@lists.open-mesh.org
-
Antonio Quartulli -
Marek Lindner -
Sven Eckelmann