netdev - Re: [PATCH ethtool 3/3] Fix potentinal null-pointer derference issues.

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]

Date: Mon, 8 May 2023 00:57:52 +0200
From: Michal Kubecek <mkubecek@...e.cz>
To: Nicholas Vinson <nvinson234@...il.com>
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH ethtool 3/3] Fix potentinal null-pointer derference
 issues.

On Sun, Apr 30, 2023 at 06:50:52PM -0400, Nicholas Vinson wrote:
> Found via gcc -fanalyzer. Analyzer claims that it's possible certain
> functions may receive a NULL pointer when handling CLI arguments. Adding
> NULL pointer checks to correct the issues.
> 
> Signed-off-by: Nicholas Vinson <nvinson234@...il.com>

A similar theoretical issue was discussed recently:

  https://patchwork.kernel.org/project/netdevbpf/patch/20221208011122.2343363-8-jesse.brandeburg@intel.com/

My position is still the same: argv[] members cannot be actually null
unless there is a serious kernel bug (see the link above for an
explanation). I'm not opposed to doing a sanity check just in case but
if we do, I believe we should check the whole argv[] array right at the
beginning and be done with it rather than add specific checks to random
places inside parser code.

> @@ -6182,16 +6182,18 @@ static int find_option(char *arg)
>  	size_t len;
>  	int k;
>  
> -	for (k = 1; args[k].opts; k++) {
> -		opt = args[k].opts;
> -		for (;;) {
> -			len = strcspn(opt, "|");
> -			if (strncmp(arg, opt, len) == 0 && arg[len] == 0)
> -				return k;
> -
> -			if (opt[len] == 0)
> -				break;
> -			opt += len + 1;
> +	if (arg) {
> +		for (k = 1; args[k].opts; k++) {
> +			opt = args[k].opts;
> +			for (;;) {
> +				len = strcspn(opt, "|");
> +				if (strncmp(arg, opt, len) == 0 && arg[len] == 0)
> +					return k;
> +
> +				if (opt[len] == 0)
> +					break;
> +				opt += len + 1;
> +			}
>  		}
>  	}
>  

I would rather prefer a simple

	if (!arg)
		return -1;

to avoid closing almost all of the function body inside an if block.

Michal

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