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] [day] [month] [year] [list]

Date: Mon, 8 May 2023 08:48:43 -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/8/23 03:40, Michal Kubecek wrote:
> On Sun, May 07, 2023 at 10:46:05PM -0400, Nicholas Vinson wrote:
>> 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.
> Please note that ethtool is not a utility for a general POSIX system.
> It is a very specific utility which works and makes sense only on Linux.
> That's why it can and does take many assumptions which are only
> guaranteed on Linux but may not be true on other POSIX systems.
>
>> 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.
> As I said before, I do not object to adding a sanity check of argc/argv,
> I only objected to adding random checks inside the parser code just to
> make static checkers happy. If you want to add (or strengthen) a sanity
> check right at the beginning of the program, before any parsing, I have
> no problem with that.

I am abandoning this patch in favor of a new one with the subject line 
"[PATCH ethtool] Fix argc and argp handling issues".

Thanks,

Nicholas Vinson


> Michal