netdev - Re: [PATCH ethtool 1/2] netlink: return -ENOMEM when calloc fails

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: <20200928154455.hi6767brao7p4ac5@lion.mk-sys.cz>
Date:   Mon, 28 Sep 2020 17:44:55 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Ivan Vecera <ivecera@...hat.com>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH ethtool 1/2] netlink: return -ENOMEM when calloc fails

On Thu, Sep 24, 2020 at 09:27:57PM +0200, Ivan Vecera wrote:
> Fixes: f2c17e107900 ("netlink: add netlink handler for gfeatures (-k)")
> 
> Cc: Michal Kubecek <mkubecek@...e.cz>
> Signed-off-by: Ivan Vecera <ivecera@...hat.com>
> ---
>  netlink/features.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/netlink/features.c b/netlink/features.c
> index 3f1240437350..b2cf57eea660 100644
> --- a/netlink/features.c
> +++ b/netlink/features.c
> @@ -112,16 +112,17 @@ int dump_features(const struct nlattr *const *tb,
>  	unsigned int *feature_flags = NULL;
>  	struct feature_results results;
>  	unsigned int i, j;
> -	int ret;
> +	int ret = 0;
>  
>  	ret = prepare_feature_results(tb, &results);
>  	if (ret < 0)
>  		return -EFAULT;
>  
> -	ret = -ENOMEM;
>  	feature_flags = calloc(results.count, sizeof(feature_flags[0]));
> -	if (!feature_flags)
> +	if (!feature_flags) {
> +		ret = -ENOMEM;
>  		goto out_free;
> +	}
>  
>  	/* map netdev features to legacy flags */
>  	for (i = 0; i < results.count; i++) {
> @@ -184,7 +185,7 @@ int dump_features(const struct nlattr *const *tb,
>  
>  out_free:
>  	free(feature_flags);
> -	return 0;
> +	return ret;
>  }
>  
>  int features_reply_cb(const struct nlmsghdr *nlhdr, void *data)
> -- 
> 2.26.2

The patch is correct but relying on ret staying zero through the whole
function is rather fragile (it could break when adding more checks in
the future) and it also isn't consistent with the way this is done in
other functions.

AFAICS you could omit the first hunk and just add "ret = 0" above the
out_free label.

Michal

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