netdev - Re: [PATCH ethtool v2 13/13] ethtool: fix bug and use standard string parsing

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: <20221208114849.xlanfoasf3ivzek2@lion.mk-sys.cz>
Date:   Thu, 8 Dec 2022 12:48:49 +0100
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Jesse Brandeburg <jesse.brandeburg@...el.com>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH ethtool v2 13/13] ethtool: fix bug and use standard
 string parsing

On Wed, Dec 07, 2022 at 05:11:22PM -0800, Jesse Brandeburg wrote:
> The parse_reset function was open-coding string parsing routines and can
> be converted to using standard routines. It was trying to be tricky and
> parse partial strings to make things like "mgmt" and "mgmt-shared"
> strings mostly use the same code. This is better done with strncmp().
> 
> This also fixes an array overflow bug where it's possible for the for
> loop to access past the end of the input array, which is bad.
> 
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@...el.com>
> ---
>  ethtool.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/ethtool.c b/ethtool.c
> index 4776afe89e23..d294b5f8d92a 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -5280,17 +5280,21 @@ static int do_get_phy_tunable(struct cmd_context *ctx)
>  static __u32 parse_reset(char *val, __u32 bitset, char *arg, __u32 *data)
>  {
>  	__u32 bitval = 0;
> -	int i;
> +	int vallen = strlen(val);
> +	int strret;
>  
>  	/* Check for component match */
> -	for (i = 0; val[i] != '\0'; i++)
> -		if (arg[i] != val[i])
> -			return 0;
> +	strret = strncmp(arg, val, vallen);
> +	if (strret < 0)
> +		return 0;

Shouldn't this be "if (strret)"? Unless I missed something, this would
lead to e.g. "zzzz-shared" being matched for "mgmt-shared" (with usual
locale) because you would get strret > 0 here and strcmp() == 0 below.

>  
> -	/* Check if component has -shared specified or not */
> -	if (arg[i] == '\0')
> +	/* if perfect match to val
> +	 * else
> +	 * Check if component has -shared specified or not
> +	 */
> +	if (strret == 0)
>  		bitval = bitset;

And this should be "if (!arg[vallen])", I believe.

Michal

> -	else if (!strcmp(arg+i, "-shared"))
> +	else if (!strcmp(arg + vallen, "-shared"))
>  		bitval = bitset << ETH_RESET_SHARED_SHIFT;
>  
>  	if (bitval) {
> -- 
> 2.31.1
> 

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)