[B.A.T.M.A.N.] [PATCH v2 1/4] batman-adv: Return EINVAL on invalid gw_bandwidth change

[B.A.T.M.A.N.] [PATCH v2 1/4] batman-adv: Return EINVAL on invalid gw_bandwidth change

[Sven Eckelmann]

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;
 }
 
-- 
2.1.4

[B.A.T.M.A.N.] [PATCH v2 2/4] batman-adv: Backport kstrtou64 for Linux < 2.6.39

[Sven Eckelmann]

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
 
-- 
2.1.4

Re: [B.A.T.M.A.N.] [PATCH v2 2/4] batman-adv: Backport kstrtou64 for Linux < 2.6.39

[Marek Lindner]

[-- Attachment #1: Type: text/plain, Size: 287 bytes --]

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[B.A.T.M.A.N.] [PATCH v2 3/4] batman-adv: Define (u|s)(8|16|32|64) limits for Linux < 3.14

[Sven Eckelmann]

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_ */
-- 
2.1.4

Re: [B.A.T.M.A.N.] [PATCH v2 3/4] batman-adv: Define (u|s)(8|16|32|64) limits for Linux < 3.14

[Marek Lindner]

[-- Attachment #1: Type: text/plain, Size: 292 bytes --]

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[B.A.T.M.A.N.] [PATCH v2 4/4] batman-adv: Fix gw_bandwidth calculation on 32 bit systems

[Sven Eckelmann]

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;
-- 
2.1.4

Re: [B.A.T.M.A.N.] [PATCH v2 4/4] batman-adv: Fix gw_bandwidth calculation on 32 bit systems

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 856 bytes --]



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?


-- 
Antonio Quartulli


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [B.A.T.M.A.N.] [PATCH v2 4/4] batman-adv: Fix gw_bandwidth calculation on 32 bit systems

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 1177 bytes --]

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [B.A.T.M.A.N.] [PATCH v2 4/4] batman-adv: Fix gw_bandwidth calculation on 32 bit systems

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 527 bytes --]

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,

-- 
Antonio Quartulli


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [B.A.T.M.A.N.] [PATCH v2 4/4] batman-adv: Fix gw_bandwidth calculation on 32 bit systems

[Marek Lindner]

[-- Attachment #1: Type: text/plain, Size: 917 bytes --]

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [B.A.T.M.A.N.] [PATCH v2 1/4] batman-adv: Return EINVAL on invalid gw_bandwidth change

[Marek Lindner]

[-- Attachment #1: Type: text/plain, Size: 556 bytes --]

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 473 bytes --]