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

[Nicholas Vinson <nvinson234@gmail.com>]

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