netdev - Re: [PATCH ethtool v2 06/13] ethtool: fix uninitialized local variable use

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y5FGr9Tgmta0Qwpn@lunn.ch>
Date:   Thu, 8 Dec 2022 03:06:39 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Jesse Brandeburg <jesse.brandeburg@...el.com>
Cc:     mkubecek@...e.cz, netdev@...r.kernel.org
Subject: Re: [PATCH ethtool v2 06/13] ethtool: fix uninitialized local
 variable use

On Wed, Dec 07, 2022 at 05:11:15PM -0800, Jesse Brandeburg wrote:
> '$ scan-build make' reports:
> 
> netlink/parser.c:252:25: warning: The left operand of '*' is a garbage value [core.UndefinedBinaryOperatorResult]
>         cm = (uint32_t)(meters * 100 + 0.5);
>                         ~~~~~~ ^
> 
> This is a little more complicated than it seems, but for some
> unexplained reason, parse_float always returns integers but was declared
> to return a float.

Looks like i got that wrong. Duh!

> This is confusing at best. In the case of the error
> above, parse_float could conceivably return without initializing it's
> output variable, and because the function return variable was declared
> as float but downgraded to an int via assignment (-Wconversion anyone?)
> upon the return, the return value could be ignored.
> 
> To fix the bug above, declare an initial value for meters, and make sure
> that parse_float() always returns an integer value.
> 
> It would probably be even more ideal if parse_float always initialized
> it's output variables before even checking for input errors, but that's
> for some future day.

It is also following the pattern of other parse_foo functions. None of
them set the result in case of errors.

> 
> CC: Andrew Lunn <andrew@...n.ch>
> Fixes: 9561db9b76f4 ("Add cable test TDR support")
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@...el.com>
> ---
>  netlink/parser.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/netlink/parser.c b/netlink/parser.c
> index f982f229a040..70f451008eb4 100644
> --- a/netlink/parser.c
> +++ b/netlink/parser.c
> @@ -54,8 +54,7 @@ static bool __prefix_0x(const char *p)
>  	return p[0] == '0' && (p[1] == 'x' || p[1] == 'X');
>  }
>  
> -static float parse_float(const char *arg, float *result, float min,
> -			 float max)
> +static int parse_float(const char *arg, float *result, float min, float max)
>  {
>  	char *endptr;
>  	float val;
> @@ -237,7 +236,7 @@ int nl_parse_direct_m2cm(struct nl_context *nlctx, uint16_t type,
>  			 struct nl_msg_buff *msgbuff, void *dest)
>  {
>  	const char *arg = *nlctx->argp;
> -	float meters;
> +	float meters = 0;
>  	uint32_t cm;
>  	int ret;

Should this actually be 0.0? Otherwise if -Wconversion gets turned on,
you have an int being converted to a float?

Anyway:

Reviewed-by: Andrew Lunn <andrew@...n.ch>

    Andrew