netdev - Re: [PATCH][v2] net: ethtool: not call vzalloc for zero sized memory request

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: <20190329092908.GK26076@unicorn.suse.cz>
Date:   Fri, 29 Mar 2019 10:29:08 +0100
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Li RongQing <lirongqing@...du.com>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH][v2] net: ethtool: not call vzalloc for zero sized memory
 request

On Fri, Mar 29, 2019 at 09:18:02AM +0800, Li RongQing wrote:
> NULL or ZERO_SIZE_PTR will be returned for zero sized memory
> request, and derefencing them will lead to a segfault
> 
> so it is unnecessory to call vzalloc for zero sized memory
> request and not call functions which maybe derefence the
> NULL allocated memory
> 
> this also fixes a possible memory leak if phy_ethtool_get_stats
> returns error, memory should be freed before exit
> 
> Signed-off-by: Li RongQing <lirongqing@...du.com>
> Reviewed-by: Wang Li <wangli39@...du.com>
> ---
> v1->v2: not call get_ethtool_stats if n_stats is 0 
> 
>  net/core/ethtool.c | 46 ++++++++++++++++++++++++++++++----------------
>  1 file changed, 30 insertions(+), 16 deletions(-)

This looks correct, I have just one minor comment below.

> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index b1eb32419732..36ed619faf36 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -1797,11 +1797,16 @@ static int ethtool_get_strings(struct net_device *dev, void __user *useraddr)
>  	WARN_ON_ONCE(!ret);
>  
>  	gstrings.len = ret;
> -	data = vzalloc(array_size(gstrings.len, ETH_GSTRING_LEN));
> -	if (gstrings.len && !data)
> -		return -ENOMEM;
>  
> -	__ethtool_get_strings(dev, gstrings.string_set, data);
> +	if (gstrings.len) {
> +		data = vzalloc(array_size(gstrings.len, ETH_GSTRING_LEN));
> +		if (!data)
> +			return -ENOMEM;
> +
> +		__ethtool_get_strings(dev, gstrings.string_set, data);
> +	} else {
> +		data = NULL;
> +	}
>  
>  	ret = -EFAULT;
>  	if (copy_to_user(useraddr, &gstrings, sizeof(gstrings)))

If gstrings.len is zero, there is no need to set data pointer as it's
not going to be used so that you could omit the else branch. It's the
same in the other two functions.

Michal