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]

Message-ID: <8907c066-9ac9-8abc-eeff-078d0b0219de@gmail.com>
Date: Sun, 7 May 2023 22:46:05 -0400
From: Nicholas Vinson <nvinson234@...il.com>
To: Michal Kubecek <mkubecek@...e.cz>
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH ethtool 3/3] Fix potentinal null-pointer derference
 issues.

On 5/7/23 18:57, Michal Kubecek wrote:
> 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.

By convention and POSIX standard, the last argv[] member is always set 
to NULL and is accessed from main(int argc, char **argp) via argp[argc] 
(see 
https://pubs.opengroup.org/onlinepubs/9699919799/functions/execve.html). 
It's also possible for argc to be zero. In such a case, 
find_option(NULL) would get called.

However, after reviewing main(), I recommend changing:

         if (argc == 0)

                 exit_bad_args();

to

         if (argc <= 0 || !*argp)

                 exit_bad_args();

as this fixes the potential issue of main()'s argc being 0 (argc would 
be -1 at this point in such cases), and "!*argp" silences gcc's built-in 
analyzer (and should silence all other SA with respect to the reported 
issue) as the SA doesn't recognize that it would take a buggy execve 
implementation to allow argp to be NULL at this point ).

If you don't have any objections to this change, I can draft an updated 
patch to make this change.

Thanks,

Nicholas Vinson

>> @@ -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