Re: [B.A.T.M.A.N.] [PATCH] gw-bandwidth: fix potential overflow on very large input values, limit them to 100 GBit/s

[Sven Eckelmann <sven@narfation.org>]

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

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

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