On Saturday 16 May 2015 22:26:43 Ruben Wisniewski wrote:

Signed-off-by: Ruben Wisniewsi ruben@vfn-nrw.de

Same commit message/topic problems as in the other patch. The patch also doesn't apply on master/next.

Applying: gw-bandwidth: fix potential overflow on very large input values, limit them to 100 GBit/s fatal: corrupt patch at line 11 Patch failed at 0001 gw-bandwidth: fix potential overflow on very large input values, limit them to 100 GBit/s The copy of the patch that failed is found in: /home/sven/tmp/batman-adv/.git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort".

net/batman-adv/gateway_common.c | 6 ++++++ 1 file changed, 6 insertions(+)

diff --git a/net/batman-adv/gateway_common.c b/net/batman-adv/gateway_common.c index 39cf44c..6b0f4d3 100644 --- a/net/batman-adv/gateway_common.c +++ b/net/batman-adv/gateway_common.c @@ -71,10 +71,16 @@ static bool batadv_parse_gw_bandwidth(struct net_device *net_dev, char *buff, switch (bw_unit_type) { case BATADV_BW_UNIT_MBIT:

• /* limit input to 100 GBit/s */
  

• if (ldown > 100000)
  

• 	ldown = 100000
  

  *down = ldown * 10; break; case BATADV_BW_UNIT_KBIT: default:

• /* limit input to 100 GBit/s */
  

• if (ldown > 100000000)
  

• 	ldown = 100000000
  

  *down = ldown / 100; break; }

Wouldn't it be better to reject the input and inform the user about it with an error code (EINVAL or something like that)?

The value looks a little bit randomly chosen. I've looked a little bit into the "new' TLV code and it seems store store this value inside a be32 variable when it is sent over the wire. This be32 can store 4294967295. The unit it represents seems to be in 100 Kib/s. So you would end up with a max of ~409 Tibit/s.

Maybe you can emphasize more why 100 Gibit/s is a good choice.

Btw. am I the only which gets a little bit sad about the fact that this function uses kstrtol to parse the input and then nobody check if ldown or lup is negative? ... sorry, got a little bit sidetracked.

Also, why does this change not include the upload speed parsing part of this function?

Kind regards, Sven